dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

Roslyn allows pointers to `Span<T>` if runtime assemblies are referenced directly #58040

Open PathogenDavid opened 3 years ago

PathogenDavid commented 3 years ago

(It could be argued this is the runtime's fault, but it's not clear to me where the fix would end up being -- assuming this should be fixed at all. I'm 80% reporting this issue because this probably isn't the first time I've done this and probably won't be the last and I need something that comes up when I search CS0208.)

Version Used:

Steps to Reproduce:

  1. Create a new C# project
  2. Enable unsafe blocks
  3. Write Span<char>* elements = stackalloc Span<char>[16]; somewhere in an unsafe context.
  4. Observe there is an error.
  5. Add the following to your csproj (substituting the path as appropriate):
  <ItemGroup>
    <Reference Include="C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.9\System.*.dll" />
  </ItemGroup>

Expected Behavior: A compiler error should occur:

error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('Span<char>')

Actual Behavior: Code compiles and runs just fine.

In other Roslyn-powered tools

Obviously the above <Reference ...> item is extremely atypical of a modern .NET project, but what is more common is how this issue surfaces in various Roslyn-powered tools since they seem to love referencing the runtime assemblies directly.

If you try the same on SharpLab you will not experience a compiler error.

Similarly, it works in the C# Interactive in Visual Studio (in both 17.0.0p3.1 and 16.11.1):

image

As well as CSharpRepl:

image

The underlying cause

The reason this happens is the actual implementation of Span<T> is technically unmanaged because it is just a pair of ByReference<T> and int. (The runtime has a special understanding of it, but as far as Roslyn is concerned ByReference<T> is just a struct with a single IntPtr field.)

This is not normally an issue because generally speaking the compiler is invoked with the runtime's reference assemblies, and the reference assembly replaces the ByReference<T> field with an object dummy. (My understanding is this is special-cased here in IsOrContainsReferenceType -- For context as to why private fields become dummies see https://github.com/dotnet/runtime/issues/16402.)

Possible fixes

  1. Roslyn should understand System.ByReference<T> to be object-like.
  2. Roslyn should understand that pointers to System.Span<T>, System.ReadOnlySpan<T>, and System.TypeReference shouldn't be possible.
  3. The runtime should modify ByReference<T> to use object instead of IntPtr for the placeholder field. (Although I imagine there's a reason it doesn't.)
  4. The runtime reference assembly should not use a dummy object field. (Probably not a good idea since you can use this to create non-obvious GC holes.)
  5. Roslyn should add the most obscure warning in existence in the next warning wave.
  6. Do nothing since technically all of the above would be a breaking change.
  7. All three of the above Roslyn-powered tools should stop compiling against the actual runtime assemblies.
  8. I should be banished to the shadow realm for attempting to stackalloc an array of spans. (I swear it was kosher in context!)

(Personally I am partial to option 8.)

jaredpar commented 3 years ago

Obviously the above <Reference ...> item is extremely atypical of a modern .NET project, but what is more common is how this issue surfaces in various Roslyn-powered tools since they seem to love referencing the runtime assemblies directly.

It's more than atypical though, it's generally not supported. The reference assemblies are meant for compiling .NET programs, not the runtime assemblies. Compiling against the runtime assemblies is subject to a number of different problems (including breaks between versions).

If the runtime team wants to support this then they need to change the definition such that Span<T> is recognized as a pointer illegal type. It's possible we end up agreeing that roslyn should just inherently recognize it as such but at the moment that is not the case. We depend on the runtime team to define the type in such a way that it's recognized as non-pointer legal.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.