Poly fill Index and Range types

[tarekgh]

Fixes #28285

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

[tarekgh]

CC @stephentoub @bartonjs @tannergooding @terrajobst @jaredpar

@ViktorHofer I appreciate if you can take a quick look too.

[ericstj]

LGTM, most comments here are just on style. Tried to look for precedent elsewhere in repo. If you take the suggestions please do the same in src project.

[terrajobst]

What's the reason to target .NET Core explicitly when we just give it the .NET Standard 2.1 assets?

[tarekgh]

What's the reason to target .NET Core explicitly when we just give it the .NET Standard 2.1 assets?

The existing type Base64Url poly fill ns2.1. Index & range type forward in ns2.1 target.

[terrajobst]

Gotcha, so .NET Core and .NET Standard are in fact not getting the same assets. I read the configuration wrong then.

[ViktorHofer]

Changes LGTM. I agree with @stephentoub that we should replace the existing usages of IncludeIndexRangeTypes.

[jkoritzinsky]

We need to make sure to ship the netstandard2.0 version of Microsoft.Bcl.Memory in the ref pack in the analyzers/cs folder, otherwise we'll break the ComInterfaceGenerator. Looking at the build artifacts, it's not being included currently.

[ViktorHofer]

This is a strange case. You probably need to add the BCL project name here

But then you also need to make sure that System.Memory.dll is present as this library depends on it. Redistributing System.Memory.dll which last shipped in 2022 from a corefx branch inside the Microsoft.NETCore.App.Ref targeting pack is problematic, to say the least. Unless we know that the compiler already includes System.Memory.dll when targeting .NET Framework, then it would be fine to omit the assembly (source generators are just compiler plugins).

I find it very weird that we now ship this Bcl assembly inside the Microsoft.NETCore.App.Ref targeting pack (inside the source gen folder) but given that source generators must target netstandard2.0, this might be OK. cc @jaredpar as this is related to your design doc about compilers shipping inside the SDK vs VS.

[tarekgh]

@jkoritzinsky @ViktorHofer do you know how I can add Microsoft.Bcl.Memory ns2.0 version to analyzers/dotnet/cs folder in the ref package? I am not seeing a straightforward way to do that.

This is a strange case. You probably need to add the BCL project name here:

This didn't work and I am not sure either if it will be correct. The list there is detect if it is analyzer and handle it accordingly. It is not right every project reference Microsoft.Bcl.Memory will be treated as analyzer.

[ViktorHofer]

I will take a look tomorrow.

[ViktorHofer]

Look at the ProjectReference above. You need to specify Pack and PackagePath so that the ProjectReference gets copied correctly.

Suggested change

	<ProjectReference Include="$(LibrariesProjectRoot)\Microsoft.Bcl.Memory\src\Microsoft.Bcl.Memory.csproj" />
	<ProjectReference Include="$(LibrariesProjectRoot)\Microsoft.Bcl.Memory\src\Microsoft.Bcl.Memory.csproj" Pack="true" PackagePath="analyzers/dotnet/cs" />

[tarekgh]

Let me try that. Thanks!

[tarekgh]

@jkoritzinsky @stephentoub after trying and talking to @ViktorHofer, looks it needs non-trivial change in the infra to allow bin-placing the Bcl library next to the source gen library in the ref pack. For now, I am going to revert the source gen changes and will create an issue to track the infrastructure work and to add the source gen dependency on the new Bcl library.

[tarekgh]

I created the issue #104308 to track the work need to be done.

[jaredpar]

@jaredpar as this is related to your dotnet/sdk#41790.

That doc only covers two items:

1. The version of the compiler used in builds.
2. How analyzers load in VS.

It wouldn't change anything related to how dependencies of analyzers load