Arcade: GenFacades should not use IL-rewriting (breaks SourceLink)

[ericstj]

We should re-implement GenFacades to not rewrite IL.

See https://github.com/dotnet/buildtools/issues/705, https://github.com/dotnet/corefx/issues/33987, https://github.com/dotnet/arcade/issues/1574

Rather than rewrite IL, we should have a step that runs before compile and does the following:

1. Inputs: reference assembly, @(ReferencePath), @(Compile)
2. Outputs: intermediate source file containing type-forwards
3. Examines the reference assembly for all type-defs => R (can be done using Roslyn or S.R.M).
4. Loads @(Compile) source into Roslyn to determine type-defs in implementation => I
5. Candidate typeforwards C = R - I
6. Optionally probe @(ReferencePaths) to locate the targets for typeforwards to determine which forwards can be satisfied, and which will need disambiguation with SeedPreferences. We could ignore this and just let the compiler fail.
7. To implement SeedType preference we allow for this to be passed in. When passed in, we'll add an additional copy of that reference, aliased, and emit the type forward using that alias.
8. To implement 'allow missing types' we could either force the project to specify these as an input and then just let the compiler fail if the type is missing, or we could have the check implemented in GenFacades if we do the thing I discuss in 6 above.
9. Emit type-forwards to an intermediate file.

We won't be able to implement ForceZeroVersionSeeds in this technique. If that's still required it can be done in a seperate task that rewrites only the Assembly refs emitted by the compiler.

To implement reference facades we need something slightly different. These have a different mode, they start source that defines the entire reference assembly and subtract the types which can be forwarded. We could just keep these using the existing tools, since they don't need to have PDBs as they are reference assemblies, but I'll spec out what it would look like to implement this behavior as well. Inputs would be similar to above process. Instead of producing a new source file, we'd produce a replacement source file that preserves the content for types that cannot be forwarded and adds type-forwards for things that can be forwarded.

[danmosemsft]

We need this to get correct symbols (and source link at all) for any rewritten assembllies, as noted in https://github.com/dotnet/buildtools/issues/1896

That's probably always been broken, so I'm surprised that it has not caused lots of complaints.

[ViktorHofer]

We definitely need this for 3.0 but I'm not sure how to correctly track this here because of the missing milestone.

[ericstj]

Maybe a companion bug in CoreFx that tracks the scenario that requires this work? That way we can test the exact thing we want to work and make sure there aren't any other causes that break symbols source linking.

[danmosemsft]

@Anipik this is the primary issue we need fixed. Detailed breakdown above by @ericstj .

[Anipik]

The list of partial facade project is

• [ ] System.Collections.ConcurrentSystem.Buffers
• [x] System.Collections
• [ ] System.Diagnostics.StackTrace
• [x] System.Diagnostics.Tools
• [x] System.IO.FileSystem
• [ ] System.Memory
• [ ] System.Numerics.Vectors
• [ ] System.Reflection.TypeExtensions
• [ ] System.Runtime.InteropServices
• [x] System.Runtime
• [x] System.Runtime.Extensions
• [ ] System.Security.Principal
• [ ] System.Runtime.InteropServices.WindowsRuntime
• [ ] System.Threading.Overlapped
• [ ] System.Threading.Thread
• [ ] System.Threading
• [ ] System.Xml.XPath.XDocument
• [ ] Microsoft.Win32.Registry.AccessControl
• [ ] System.Collections.NonGeneric
• [ ] System.ComponentModel.TypeConverter
• [ ] System.Security.Cryptography.Primitives
• [ ] System.Transactions.Local
• [x] System.Security.Permissions

I have written the tool for this. In order to verify that the new tool is working fine, i am comparing tools output to already running genfacades output. I have done this exercise for two of the assemblies. i will
verify it by running on couple of more assemblies before opening a pr in arcade repo.

The code for the new tools is https://github.com/Anipik/publicSurfaceArea

The only limitation is that it wont be able to pick the existing typeforwards in the assembly. Shouldnt be a problem in our use case scenario

[ericstj]

This is a reasonable start: we'll need to be able to handle an explicit list of files as well as #defines since that's how these projects actually build. That should just be some simple plumbing into the task. How were you envisioning getting the information from the reference projects?

[Anipik]

we'll need to be able to handle an explicit list of files

The version that I wrote for arcade takes in just the list of files.

defines since that's how these projects actually build

I didn't thought of this subcase. but I will come up with something for this.

How were you envisioning getting the information from the reference projects ?

I am sticking to the current method to extract the types from reference assembly https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.GenFacades/GenFacades.cs#L59 and reference projects https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.GenFacades/GenFacades.cs#L84.

[ericstj]

I see, got it!

The only limitation is that it wont be able to pick the existing typeforwards in the assembly. Shouldnt be a problem in our use case scenario

There should be a way to get these out of roslyn too. Test out what happens when someone does this so it isn't something bad like silently allowing duplicate type-forward attributes.

[Anipik]

There should be a way to get these out of roslyn too. Test out what happens when someone does this so it isn't something bad like silently allowing duplicate type-forward attributes.

So these are treated as attributes by the C# analyser and I was able to parse them accordingly

defines since that's how these projects actually build

there is CsharpOptions available in which we can provide the list of constants. By default all constants are treated as false

[Anipik]

Will close it after i have verified the sourceLink stuff

[ViktorHofer]

@Anipik implementation assemblies which are partial facades now contain source link information?

[Anipik]

yes @ViktorHofer they do contain sourcelink for all the files except the autogenerated files. (which are SR.cs and *.notSupported.cs). I have verified this by enabling the sourcelink for the local pdbs. I will verify with the official pdbs as well once they are build with my change

[ViktorHofer]

Sourcelink isn't enabled locally by default, I assume you changed that locally?

[Anipik]

You need to pass /p:EnableSourceLink=true in order to have pdbs with urls.
do we want to enable it by default ? i was not aware of that. I can put up the pr if that's the case.

[ViktorHofer]

https://github.com/dotnet/corefx/pull/34900 please not 😂

just wanted to make sure that you validated correctly locally.

[Anipik]

hahahah okay :P