Add transitive CompilerServices.Unsafe dependency

[ViktorHofer]

for System.Memory and System.Threading.Tasks.Extensions package
references. Adding the dependency as a P2P to prefer the live asset
over the prebuilt.

Fixes #40208

I attempted to automate this but didn't find a viable solution that would cover all cases. We can revisit this to add protection later.

[Dotnet-GitSync-Bot]

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

[ericstj]

I'm surprised to see Unsafe show up in reference assemblies. I didn't think it had any exchange types.

[ericstj]

Also it looks like all these paths are wrong since they omitted Runtime from System.Runtime.CompilerServices.Unsafe

[ViktorHofer]

I'm surprised to see Unsafe show up in reference assemblies. I didn't think it had any exchange types.

I'm unsure what you mean by that. It's coming in as a transitive dependency from System.Memory.

[ericstj]

unsure what you mean by that

I don't think any reference assembly in repo should ever have a reference to System.Runtime.CompilerServices.Unsafe since it contains a single static type which will never appear in any other assembly's surface area.

[ViktorHofer]

If I add a PackageReference to System.Memory or System.Threading.Tasks.Extensions, the reference to System.Runtime.CompilerServices.Unsafe is added transitively. This is already happening in master today. How would I express that a PackageReference should not pass its dependency to the compiler?

[ericstj]

Looks System.Diagnostics.DiagnosticSource.Tests on.NETFramework are encountering legit failures:

System.TypeInitializationException: The type initializer for 'PerTypeValues`1' threw an exception. ---> System.IO.FileLoadException: Could not load file or assembly 'System.Runtime.CompilerServices.Unsafe, Version=4.0.4.1, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
   at System.SpanHelpers.PerTypeValues`1.MeasureArrayAdjustment()
   at System.SpanHelpers.PerTypeValues`1..cctor()

[ViktorHofer]

It looks for S.R.CompilerServices.Unsafe 4.0.4.1 which is in the 4.5.3 version of the package. We put the 5.0.0 asset into the output directory and no binding redirects are present, hence the TypeLoadException. @ericstj if we want to use the live asset we need the binding redirect, right?

[ericstj]

if we want to use the live asset we need the binding redirect, right?

Correct. It may also be indicative of a problem our customers might encounter with the package, since that also references latest Unsafe: https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/5.0.0-preview.8.20407.11

Have a look at the test's RAR run to ensure that it sees both System.Memory with Unsafe reference and the direct reference to the live Unsafe dll.

[ViktorHofer]

The binding redirect isn't generated because of _FindDependencies set to false:

runtime/eng/references.targets

Lines 3 to 4 in 2ba6515

	<!-- Disable RAR from transitively discovering dependencies for References -->
	<_FindDependencies>false</_FindDependencies>

. As soon as I remove that, RAR sees the conflict:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <runtime>
    <UseRandomizedStringHashAlgorithm enabled="1" />
    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="System.Runtime.CompilerServices.Unsafe" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
        <bindingRedirect oldVersion="0.0.0.0-5.0.0.0" newVersion="5.0.0.0" />
      </dependentAssembly>
    </assemblyBinding>
  </runtime>
</configuration>

When I changed our references and RefPath I considered removing that variable but I wasn't entirely sure what it is for. Good that I now know its intent.

[ericstj]

I suspect that flag was there to ensure refpath + simple-name references was always WYSIWYG. You might try checking after removing it that we still force libraries to declare their reference closure and they don't automatically get transitive references because RAR sees them. If there's a problem consider a condition on that flag and only apply it in cases where we are forcing granular references.

[ViktorHofer]

The only difference that I can tell is that RAR finds the dependencies and stores them in the ReferenceDependencyPaths item which is only used in GenerateDepsFile wix/clickonce/etc but not in csc. What's your recommendation @ericstj? Keep the property but only for granular references or remove it completely? Unsure if dependencies inside the deps.json files are a problem (afaik we don't use the deps.json files of individual libraries).

[ericstj]

If the flag doesn’t impact references nor copy local I am fine removing. Glad to eliminate private property usage.

[ericstj]

Lgtm assuming green ci