Do not require fixing a fixed field of a ref struct

[gafter]

From #1502

A fixed statement should not be required on a fixed field of a ref struct, because it is never moveable.

Note for future LDM consideration: there is a bug tail involved with any fixed change. While this is probably not worth it on it's own merits, if we have other fixed work that result in refactoring this would be worth it at that point.

[4creators]

Encountered this problem multiple times while working with HW intrinsics.

[jaredpar]

This is very related to #1697

[tannergooding]

Should this just be, don't requiring pinning when taking the address of a ref struct or its fields?

The entire ref struct is fixed and should be safe to take the address of (not just this and not just fixed fields).

[gafter]

@tannergooding I think you’re right

[ltrzesniewski]

@tannergooding I didn't talk about fields in #1697 but I probably should have. I think it should be safe to take both the address of the ref struct and of its unmanaged fields.

[kkokosa]

It seems I've already proposed a more general question regarding all fields in #1564

[tannergooding]

@gafter, does this need to go to LDM or is it already covered by the languge spec?

The fix, overall, looks rather trivial. From what I was able to determine, this ultimately comes down to what we return from IsMoveableVariable (http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/Binder/Binder_Operators.cs,bfd68ac15f2dece4) and the fix would be to check if expr.Type.IsRefLikeType (or a similar check, depending on the expression kind)....

[tannergooding]

I guess this does need to go to LDM, since it could be a breaking change...

Normally, the compiler reports ERR_FixedNotNeeded or ERR_FixedNeeded. ref structs should fall under the former, but currently are treated as the latter.

If this was simply changed to not require fixed, than any existing code which is fixing a ref struct would now fail to compile as ERR_FixedNotNeeded would be reported. So, it would need to be that:

• We take a breaking change and have people fix up their code
• We don't take the change because it is breaking
• We take the change, but allow fixing or not so that existing code continues to work
• Some other alternative not listed

[tannergooding]

I've prototyped something that looks to work here: tannergooding/roslyn@f31dbcd

It goes with the first option (which is most likely not what LDM will decide given the compat bar) and I'm sure I missed some edge cases, but I'll add more tests to find out.

[PathogenDavid]

• We take the change, but allow fixing or not so that existing code continues to work

I think this solution along with adding a warning to a warning wave might be the best solution to add this without breaking compatibility. (An error would be ideal, but LDM decided against error waves.)

That and also probably omit pinned flag on the corresponding local when the unnecessary fixed statement is used.

[jaredpar]

We take a breaking change and have people fix up their code

Don't think that is a realistic option here. Code which is unsafe is more likely to be legacy than not. There is plenty in Visual Studio for example that hasn't been touched in a decade. Asking developers to change all of that up, particularly because it's an area which can have very subtle bugs, just to upgrade the compiler is not realistic.

We take the change, but allow fixing or not so that existing code continues to work

Much better 😄

[tannergooding]

Just commenting here as I always need to go looking for it again.

The LDM last reviewed this here: https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-10-14.md#do-not-require-fixing-a-fixed-field-of-a-ref-struct and its on the long term backlog