Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Find implementation assemblies for reference assemblies in the .NET SDK #61526

Merged
merged 12 commits into from
Jun 8, 2022
Merged

Find implementation assemblies for reference assemblies in the .NET SDK #61526

merged 12 commits into from
Jun 8, 2022

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented May 26, 2022 •
edited
Loading

Part of #55834

This makes Source Link work for projects targeting .NET 6 if a matching runtime is installed for the targeted SDK. It does cool things like this:

Navigating to symbol 'System.Attribute.GetCustomAttribute(System.Reflection.Assembly, System.Type)' from 'System.Runtime'.
Symbol found in assembly path 'C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\6.0.4\ref\net6.0\System.Runtime.dll'
Symbol found in assembly path 'C:\Program Files\dotnet\shared\Microsoft.NETCore.App\6.0.4\System.Runtime.dll'
Symbol found in assembly path 'C:\Program Files\dotnet\shared\Microsoft.NETCore.App\6.0.4\System.Private.CoreLib.dll'
Found PDB on symbol server.
'/_/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs' found via SourceLink.

Still to do:

  • More tests that do type forwarding
  • .NET Framework support - look in GAC? some code in MAS might help here
  • .NET Maui (and other runtime packs) support - these need more XML file lookups
  • .NET Standard - No idea!

@tmat PTAL
FYI @chuckries @caslan @mikadumont

@davidwengier
Find implementation assemblies for reference assemblies in the .NET SDK
2956108 
There are two parts to this:

1. A heuristic approach to finding implementation DLLs from the dotnet\shared folder, when a symbol comes from a reference DLL in the dotnet\packs folder. This works for things like System.Console, whose reference comes from System.Console.dll, and implementation is in the corresponding System.Console.dll in the shared folder
2. Loading implementation DLLs and following the type forwarding chain to the correct assembly for types that have been forwarded. This works for things like System.Attribute, which is in reference assembly System.Runtime.dll, but the implementation assembly System.Runtime.dll forwards the type to System.Private.CoreLib.dll
@davidwengier davidwengier requested a review from a team as a code owner May 26, 2022 08:23
@davidwengier
Copy link
Contributor Author

davidwengier commented May 26, 2022 •
edited
Loading

This is one of those situations where I've been fighting code for 2 days, but now it's 10 minutes before dinner time and so I hastily put up a PR to get a dopamine hit.

All of that is to say don't be surprised if there is a glaring mistake. I also fully expect @tmat to tell me to just call the magical FindForwardedType method in S.R.Metadata or something :)

@runfoapp runfoapp bot mentioned this pull request May 26, 2022
Open
tmat
tmat reviewed May 27, 2022
View reviewed changes
src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceHelpers.cs Outdated
/// Uses various heuristics to try to find the implementation assembly for a reference assembly without
/// loading
/// </summary>
public static bool TryFindImplementationAssemblyPath(string referencedDllPath, [NotNullWhen(true)] out string? implementationDllPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryFindImplementationAssemblyPath

Shouldn't this be async and read files using async API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, why not ¯\_(ツ)_/¯

Though I wouldn't expect much difference. This only reads one XML file, and only the first line of that file. In fact, reading a file that is just <FileList FrameworkName="MyPack"> doesn't even throw an XmlException, because it doesn't read far enough to determine that it's invalid :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually.. because of the out parameter, this is a bit annoying. Is it really going to make any meaningful difference? Only the Read() call becomes ReadAsync(), there is nothing else that is async.

@tmat
Copy link
Member

tmat commented May 27, 2022

Reviewed iteration 3.

@davidwengier
Copy link
Contributor Author

Apologies for the diff, but I extracted some code from MetadataAsSourceHelpers to a new service so that the cache lifetime could be controlled. Also because I will do a follow up in future and use the same service for decompilation, so it allows for a shared cache.

@davidwengier
Copy link
Contributor Author

@tmat this is ready for another look. Will fix the other scenarios in follow ups, so this doesn't get too crowded.

tmat
tmat requested changes Jun 3, 2022
View reviewed changes
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

tmat
tmat reviewed Jun 6, 2022
View reviewed changes
src/Features/Core/Portable/PdbSourceDocument/ImplementationAssemblyLookupService.cs Outdated
{
// If there are no type forwards in this DLL, or not one for this type, then it means
// we've found the right DLL
if (typeForwards?.TryGetValue((typeSymbol.ContainingNamespace.MetadataName, typeSymbol.MetadataName), out var assemblyName) is null or false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is null or false

nit: != true

@davidwengier
Feedback
c760cf2
@davidwengier
Merge remote-tracking branch 'upstream/main' into SourceLinkForNet60
e545c74 
# Conflicts:
#	src/Features/Core/Portable/FeaturesResources.resx
@davidwengier davidwengier requested a review from tmat June 7, 2022 04:06
@davidwengier
Copy link
Contributor Author

@tmat anything else for this one? (other than fixing conflicts :P)

@davidwengier
Merge remote-tracking branch 'upstream/main' into SourceLinkForNet60
0d5e3ee 
# Conflicts:
#	src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs
#	src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs
@davidwengier davidwengier enabled auto-merge (squash) June 8, 2022 21:08
@davidwengier davidwengier merged commit 35b3d3f into dotnet:main Jun 8, 2022
@ghost ghost added this to the Next milestone Jun 8, 2022
@davidwengier davidwengier deleted the SourceLinkForNet60 branch June 9, 2022 00:51
@tmat tmat modified the milestones: Next, 17.3 Jun 16, 2022
@KieranDevvs KieranDevvs mentioned this pull request Jun 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmat tmat tmat approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
17.3
Development

Successfully merging this pull request may close these issues.
2 participants
@davidwengier @tmat