Locate implementations for reference assemblies using the GAC by sharwell · Pull Request #24291 · dotnet/roslyn

sharwell · 2018-01-17T21:13:53Z

This pull request partially implements the "locate the runtime assembly" feature described in #24349, specifically for cases where the metadata reference resolves to a reference assembly (i.e. an assembly which does not contain IL for method bodies). Reference assemblies do not produce meaningful results when decompiled, and currently all references to the .NET Framework assemblies fall into this category. For common assemblies already located in the GAC, including the .NET Framework assemblies, this pull request allows the Navigate to Decompiled Sources feature to provide meaningful results.

Fixes #24185 (other related limitations will become separate bugs)

Customer scenario

A user attempts to use the Navigate to Decompiled Sources feature to navigate to a member defined as part of the .NET Framework. The feature does not work as expected, because the decompiled source file shows the code for a reference assembly, which does not have any contents in the bodies of members.

Bugs this fixes

Fixes #24185

Workarounds, if any

None.

Risk

Low. This only impacts the Navigate to Decompiled Sources feature, which is disabled by default and only exposed as an experimental option in the Advanced page of C# settings.

Performance impact

This code is only in effect on the code paths where it is required.

Is this a regression from a previous update?

No.

Root cause analysis

Known limitation of a new feature.

How was the bug found?

Known limitation of a new feature.

Test documentation updated?

Yes.

tmat · 2018-01-19T21:11:09Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

I don't think we should be resolving or loading assemblies in devenv process.

Is there something in .Net, the project system, VS, dtar (etc. etc.) that would be able to do this mapping for us?

tmat · 2018-01-19T21:12:01Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

We shouldn't report NFW for load failures.

❓ I was hoping it would provide insight into cases where Navigate to Decompiled Sources doesn't work properly, in order to prioritize future improvements. In light of this, can you confirm that this edge case should be removed from the telemetry consideration?

sharwell · 2018-01-21T20:25:26Z

@tmat What if we change this to only consider assemblies that are already loaded in the IDE process? In other words, remove the fallback call to Assembly.ReflectionOnlyLoad(fullAssemblyName).

tmat · 2018-01-22T02:44:45Z

@sharwell I don't like this approach at all. Customers will get different behavior based on what's loaded in devenv process and what not. That's totally based on what VS features and extension the user have used prior to triggering Go To Disassembly. This would make the feature work seemingly non-deterministically.

sharwell · 2018-01-28T01:09:43Z

@tmat I tried to replace the current logic with GacFileResolver but found that the code for the resolvers is in the Scripting assembly, which EditorFeatures does not reference. How would you recommend I proceed?

sharwell · 2018-01-29T14:56:39Z

@tmat replied to my question above with the following:

Link files from src\Compilers\Shared\GlobalAssemblyCacheHelpers – start with GlobalAssemblyCache.cs, ClrGlobalAssemblyCache.cs and MonoGlobalAssemblyCache.cs. They might need some more.
GacFileResolver implements a specific resolver compiler abstraction that isn’t needed in your scenario.

I implemented the suggested approach, and this should be ready for review again.

…ding path

…f GlobalAssemblyCache

tmat · 2018-02-02T20:58:04Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

-            var reference = compilation.GetMetadataReference(symbol.ContainingAssembly);
+
+            string assemblyLocation = null;
+            var isReferenceAssembly = symbol.ContainingAssembly.GetAttributes().Any(attribute => attribute.AttributeClass.Name == nameof(ReferenceAssemblyAttribute));


ReferenceAssemblyAttribute [](start = 137, length = 26)

We should check for namespace as well.

tmat · 2018-02-02T20:59:46Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

+                    var fullAssemblyName = symbol.ContainingAssembly.Identity.GetDisplayName();
+                    GlobalAssemblyCache.Instance.ResolvePartialName(fullAssemblyName, out assemblyLocation, preferredCulture: CultureInfo.CurrentCulture);
+                }
+                catch (Exception e) when (FatalError.ReportWithoutCrashUnlessCanceled(e))


ReportWithoutCrashUnlessCanceled [](start = 53, length = 32)

Nit: the code is not cancellable, calling ReportWithoutCrash is sufficient

tmat · 2018-02-02T21:03:23Z

src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs

+            if (assemblyLocation == null)
+            {
+                var reference = compilation.GetMetadataReference(symbol.ContainingAssembly);
+                assemblyLocation = reference.Display;


reference.Display [](start = 34, length = 18)

Display is only meant to be used in messages for the user.
To get the path of the assembly use (reference as PortableExecutableReference)?.FilePath and handle case when the result is null.

handle case when the result is null

What would an appropriate error handling strategy be here?

I guess we should report that we can't navigate to the symbol.

In reply to: 165761856 [](ancestors = 165761856)

tmat

🕐

tmat · 2018-02-02T21:22:08Z

                    useDecompiler = !symbol.ContainingAssembly.GetAttributes().Any(attribute => attribute.AttributeClass.Name == nameof(SuppressIldasmAttribute));

Should also check namespace

Refers to: src/EditorFeatures/Core/Implementation/MetadataAsSource/MetadataAsSourceFileService.cs:122 in 4b1853f. [](commit_id = 4b1853f, deletion_comment = False)

* Check fully-qualified names for SuppressIldasmAttribute and ReferenceAssemblyAttribute * Use correct reference location, or fail decompilation if it's not available

sharwell · 2018-02-02T21:58:41Z

@tmat Feedback should be addressed now! 😄

tmat

sharwell · 2018-02-02T23:37:41Z

Requesting reviews from @dotnet/roslyn-compiler for the pragma change

Requesting reviews from @dotnet/roslyn-ide for behavior

jcouv

Compiler portion LGTM

natidea · 2018-02-05T20:14:27Z

Approved to merge via https://devdiv.visualstudio.com/DevDiv/_workitems/edit/562652

dpoeschl

👍

jasonmalinowski

Oops I had some pending questions that I never submitted the review on. I approve of the change, treat these as "Jason wants to learn something new" questions.

jasonmalinowski · 2018-02-03T18:16:09Z

src/EditorFeatures/Core/EditorFeatures.csproj

+    <Compile Include="..\..\Compilers\Shared\GlobalAssemblyCacheHelpers\ClrGlobalAssemblyCache.cs" Link="Shared\GlobalAssemblyCacheHelpers\ClrGlobalAssemblyCache.cs" />
+    <Compile Include="..\..\Compilers\Shared\GlobalAssemblyCacheHelpers\FusionAssemblyIdentity.cs" Link="Shared\GlobalAssemblyCacheHelpers\FusionAssemblyIdentity.cs" />
+    <Compile Include="..\..\Compilers\Shared\GlobalAssemblyCacheHelpers\GlobalAssemblyCache.cs" Link="Shared\GlobalAssemblyCacheHelpers\GlobalAssemblyCache.cs" />
+    <Compile Include="..\..\Compilers\Shared\GlobalAssemblyCacheHelpers\MonoGlobalAssemblyCache.cs" Link="Shared\GlobalAssemblyCacheHelpers\MonoGlobalAssemblyCache.cs" />


Hmm, how does this work with cross-plat? There's some runtime sniffing to use the right one at runtime?

jasonmalinowski · 2018-02-06T04:47:12Z

src/Compilers/Shared/GlobalAssemblyCacheHelpers/FusionAssemblyIdentity.cs

                    throw new ArgumentException(Scripting.ScriptingResources.InvalidCharactersInAssemblyName, nameof(name));

-#elif WORKSPACE_DESKTOP
+#elif WORKSPACE_DESKTOP || EDITOR_FEATURES


@tmat How do we eventually detangle this linked file mess?

sharwell requested a review from a team as a code owner January 17, 2018 21:13

sharwell force-pushed the ilspy-ref-assem branch from 6e37099 to c0ea68d Compare January 17, 2018 21:27

sharwell requested a review from tmat January 19, 2018 19:54

tmat reviewed Jan 19, 2018

View reviewed changes

sharwell mentioned this pull request Jan 23, 2018

Navigate to decompiled sources: Reference assemblies are not supported #24185

Closed

sharwell requested a review from a team as a code owner January 29, 2018 14:55

sharwell changed the title ~~Locate implementations for reference assemblies using the process binding path~~ Locate implementations for reference assemblies using the GAC Jan 29, 2018

sharwell requested a review from a team as a code owner January 29, 2018 16:13

sharwell added 3 commits February 2, 2018 07:16

Locate implementations for reference assemblies using the process bin…

8f0699a

…ding path

Use GlobalAssemblyCache helper to locate assemblies directly in the GAC

0c80fd7

Update InteractiveEditorFeatures to account for a second definition o…

4b1853f

…f GlobalAssemblyCache

sharwell force-pushed the ilspy-ref-assem branch from d890379 to 4b1853f Compare February 2, 2018 13:17

tmat reviewed Feb 2, 2018

View reviewed changes

tmat requested changes Feb 2, 2018

View reviewed changes

Cleanup based on code review feedback

d7df94e

* Check fully-qualified names for SuppressIldasmAttribute and ReferenceAssemblyAttribute * Use correct reference location, or fail decompilation if it's not available

tmat approved these changes Feb 2, 2018

View reviewed changes

sharwell added this to the 15.6 milestone Feb 2, 2018

sharwell added the Urgency-Now label Feb 2, 2018

sharwell self-assigned this Feb 2, 2018

jcouv approved these changes Feb 5, 2018

View reviewed changes

jinujoseph added the Needs Shiproom Approval label Feb 5, 2018

natidea added Approved to merge and removed Needs Shiproom Approval labels Feb 5, 2018

dpoeschl approved these changes Feb 5, 2018

View reviewed changes

sharwell merged commit 79e7182 into dotnet:dev15.6.x Feb 5, 2018

sharwell deleted the ilspy-ref-assem branch February 5, 2018 22:09

jasonmalinowski reviewed Feb 6, 2018

View reviewed changes

Conversation

sharwell commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Jan 21, 2018

Uh oh!

tmat commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Jan 28, 2018

Uh oh!

sharwell commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat left a comment

Choose a reason for hiding this comment

Uh oh!

tmat commented Feb 2, 2018

Uh oh!

sharwell commented Feb 2, 2018

Uh oh!

tmat left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented Feb 2, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

natidea commented Feb 5, 2018

Uh oh!

dpoeschl left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

sharwell commented Jan 17, 2018 •

edited

Loading

CyrusNajmabadi Jan 19, 2018 •

edited

Loading

tmat Jan 19, 2018 •

edited

Loading

sharwell Jan 21, 2018 •

edited

Loading

tmat commented Jan 22, 2018 •

edited

Loading

tmat Feb 2, 2018 •

edited

Loading