Improved User Experience with "Navigate to decompiled sources" #29149

siegfriedpammer · 2018-08-07T21:43:57Z

This pull request is intended as suggestion and should serve as basis for further discussion of the "Navigate to decompiled sources" feature.

This pull request implements the following changes:

Move ICSharpCode.Decompiler reference to Microsoft.CodeAnalysis.CSharp.EditorFeatures
Add Microsoft.CodeAnalysis.Editor.IDecompiledSourceService
Add basic implementation of the IDecompiledSourceService for C#: CSharpDecompiledSourceService
Move decompilation feature from MetadataAsSourceFileService to CSharpDecompiledSourceService
Do not show legal notice if decompilation is not available
Use "metadata-as-source" style for documentation comments
Format the decompiled code using the Roslyn formatter

Why

Currently ICSharpCode.Decompiler only supports C# and the "Navigation to decompiled sources" feature is only listed on the C#-specific options page.
I think that offering decompiled C# source code to VB users with VB syntax highlighting and broken navigation is even worse than the problems outlined in Decompilation does not support "collapse to definitions" #26436.
Creating a separate language service for decompilation enables us to further develop the feature with full access to C# language infrastructure. (this also addresses Go To Disassembly #23430 (comment) and Go To Disassembly #23430 (review))
Being able to use the Roslyn C# formatter is the best fix for Decompiler doesn't respect my code style settings #25007 and Decompilation feature does not respect editor line endings #28276 because it will take a long time for us to be able to reimplement all the formatting options in our codebase. And that time is better spent with implementing support for other language features and/or improving decompilation quality.

Points for further discussion

How should we improve the folding experience for the decompiled member bodies? I have been experimenting with AbstractSyntaxNodeStructureProvider to further improve the experience by completely hiding the source code when the source file is first opened. However I am not sure how to implement this, because there can only be one fold per line.
I want to mention Non-public members and types not highlighted and navigable in decompiled view #26989 here again: For navigation in decompiled sources to work perfectly, the "MetadataAsSource" workspace would have to be extended to include internal/private symbols as well. Maybe we should think about creating a separate workspace for DecompiledSource?
Implementation of VB support:
- Currently we have no resources for this, because we would have to reimplement the semantic analysis for VB on our end from scratch. We did this for C#, because we had most of it already done before Roslyn was a thing. This takes time that is better spent with improving the existing decompiler.
- The only way I can ever see this going forward is to able to use Roslyn, however that would require the decompiler to get InternalsVisibleTo access to Roslyn internals. For details see our analysis in Migrate to Roslyn typesystem? icsharpcode/ILSpy#896 especially Migrate to Roslyn typesystem? icsharpcode/ILSpy#896 (comment).

…arp.EditorFeatures * Add Microsoft.CodeAnalysis.Editor.IDecompiledSourceService * Add basic implementation of the IDecompiledSourceService for C#: CSharpDecompiledSourceService * Move decompilation feature from MetadataAsSourceFileService to CSharpDecompiledSourceService

…ts in decompilation.

siegfriedpammer · 2018-08-07T21:49:58Z

src/EditorFeatures/CSharp/DecompiledSource/DocCommentConverter.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.DecompiledSource
+{
+    // This is a copy of CSharpMetadataAsSourceService.DocCommentConverter


I know that this is bad, because it essentially is code duplication, but I am not sure whether it's possible to make the CSharpMetadataAsSourceService.DocCommentConverter accessible for the new CSharpDecompiledSourceService.

Feel free to just move the code around in features to make it more accessible; that's totally fine!

I moved it to Microsoft.CodeAnalysis.CSharp.DocumentationComments in MS.CA.CS.Features. I hope that's a suitable location.

CyrusNajmabadi · 2018-08-08T17:23:16Z

Use "metadata-as-source" style for documentation comments
Format the decompiled code using the Roslyn formatter

Wow. Thanks so much! :) <3

src/EditorFeatures/CSharp/DecompiledSource/AssemblyResolver.cs

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceService.cs

CyrusNajmabadi · 2018-08-08T17:25:12Z

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceService.cs

+using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.DecompiledSource


technically, does this need to live at the editor layer? are there any actual editor dependencies? could this live at the C# workspace layer?

➡️ Will file as a follow-up issue.

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceService.cs

CyrusNajmabadi · 2018-08-08T17:34:19Z

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceService.cs

+            }
+            while (ns != null && !ns.IsGlobalNamespace);
+
+            return string.Join(".", stack);


nit. my preference would be that we just use a SymbolDisplayFormat here. But i may be wrong given that you're using the .MetadataName. Is that necessary? can you document why?

The method is called GetFullReflectionName.... I wonder, isn't that enough documentation? As for the reason: Well... the decompiler API expects the reflection name. :)

CyrusNajmabadi · 2018-08-08T17:34:50Z

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceServiceFactory.cs

+    [ExportLanguageServiceFactory(typeof(IDecompiledSourceService), LanguageNames.CSharp), Shared]
+    internal partial class CSharpDecompiledSourceServiceFactory : ILanguageServiceFactory
+    {
+        public ILanguageService CreateLanguageService(HostLanguageServices provider)


@sharwell has some patterns he'd like you do use here. Sam, can you point him at the current pattern for mef types wrt Obsolete and whatnot?

➡️ Will file as a follow-up issue.

src/EditorFeatures/Core/IDecompiledSourceService.cs

sharwell · 2018-08-08T17:36:36Z

I think that offering decompiled C# source code to VB users with VB syntax highlighting and broken navigation

This was fixed in #27940, specifically 5a4b1ac.

siegfriedpammer · 2018-08-08T17:38:33Z

This was fixed in #27940, specifically 5a4b1ac.

Well, I was able to reproduce this bug locally, right before removing the feature for VB. I used the master branch. Is the fix not on the master branch?

edit: I see it is... strange...

siegfriedpammer · 2018-08-08T17:41:02Z

Is there anything wrong with disabling the feature for VB? I am open to discussion. However, I would really like to move the feature to a language-specific part of Roslyn.

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

src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioSymbolNavigationService.cs

sharwell · 2018-08-08T17:44:09Z

Is there anything wrong with disabling the feature for VB? I am open to discussion. However, I would really like to move the feature to a language-specific part of Roslyn.

I will check. It seems the main benefit of moving it is the formatting support.

CyrusNajmabadi · 2018-08-08T17:45:45Z

How should we improve the folding experience for the decompiled member bodies? I have been experimenting with AbstractSyntaxNodeStructureProvider to further improve the experience by completely hiding the source code when the source file is first opened. However I am not sure how to implement this, because there can only be one fold per line.

I'm not sure i understand your last statement. Why would it matter here?

Note: it's unclear to me why the StructureProviders are not working. If this document was properly created and added to the MetadataAsSource workspace, then we should be activating and lighting up the appropriate structure/outlining helpers. You shouldn't need to do anything.

siegfriedpammer · 2018-08-08T17:50:56Z

I'm not sure i understand your last statement. Why would it matter here?

The UI only allows for one plus/minus sign per line. That space is currently occupied by the fold for the documentation comment (it extends to the first keyword of the method declaration). So there is no more room.

Note: it's unclear to me why the StructureProviders are not working. If this document was properly created and added to the MetadataAsSource workspace, then we should be activating and lighting up the appropriate structure/outlining helpers. You shouldn't need to do anything.

roslyn/src/Features/Core/Portable/Structure/Syntax/AbstractSyntaxNodeStructureProvider.cs

Lines 49 to 53 in 312b115

    
           protected virtual bool SupportedInWorkspaceKind(string kind) 
        
           { 
        
               // We have other outliners specific to Metadata-as-Source. 
        
               return kind != WorkspaceKind.MetadataAsSource; 
        
           }

That's why...

siegfriedpammer · 2018-08-08T17:52:58Z

Is there anything wrong with disabling the feature for VB? I am open to discussion. However, I would really like to move the feature to a language-specific part of Roslyn.

I will check. It seems the main benefit of moving it is the formatting support.

Thanks!

CyrusNajmabadi · 2018-08-08T18:01:31Z

That's why

But we also have:

roslyn/src/Features/CSharp/Portable/Structure/Providers/MetadataAsSource/AbstractMetadataAsSourceStructureProvider.cs

Lines 51 to 54 in 312b115

    
           protected override bool SupportedInWorkspaceKind(string kind) 
        
           { 
        
               return kind == WorkspaceKind.MetadataAsSource; 
        
           }

:)

So that should be working here. that was the point of these structure providers. :)

CyrusNajmabadi · 2018-08-08T18:06:35Z

Oh, i think i get what you're saying. Effectively, for decompiled source, we want both the MetadataAsSource view for the comments on a member (that way, someone can collapse everything, and the view is very much like MAS). However, you also want to be able to collapse substructure.

There are a few ways we could solve this. One would be to create a new workspace kind for this. i.e. DecompiledSource instead of MetadataAsSource. The other would be to have all the statement/expression structure providers work in a MetadataAsSource workspace. For normal MAS, this won't matter (since you don't get statements/expressions). But for DecompiledSource, it would light up structure outlining inside methods.

siegfriedpammer · 2018-08-08T18:08:13Z

that was the point of these structure providers. :)

As far as I understood the implementation, the MAS structure providers exist only to make the comments collapsible right to the start of the declaration and not only to the end of the comment line.

CyrusNajmabadi · 2018-08-08T18:13:24Z

As far as I understood the implementation, the MAS structure providers exist only to make the comments collapsible right to the start of the declaration and not only to the end of the comment line.

Rigth. I misunderstood what you were saying. I thought you were saying that that functionality wasn't working. Which would def seem like a bug.

It sounds like you're saying that that functionality works, but we also want to be able to collapse the substructure of a method. in that case, we likely need to light up certain other structure providers so they work in DecompiledSource.

--

There's also the user-experience issue of: If i collapse the comments and i collapse an entire method body, i'll end up with effectively:

/* ...  */ public void Foo() /* ... */

How does a user effectively interact with this? Technically, with a mouse, i think they can expand either section by double clicking on it. but if they interact just with the margin + then they won't be able to expand one of these (at least, according to you). That's a rather fascinating issue, and something we should noodle on :)

Perhaps we can get the editor to treat multiple region s on hte same line as being controlled by a single glyph. So if you expand, you expand both. If you collapse, you collapse both.

…onComments

sharwell · 2018-10-30T21:00:15Z

This went to a design review today, and we determined that the proposed changes are a clear improvement for the C# developer's experience, and the VB side can be resolved later either by allowing decompilation in those cases or by implementing the more complete experience described in #24349.

sharwell · 2018-11-01T16:30:38Z

src/EditorFeatures/CSharp/DecompiledSource/AssemblyResolver.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.DecompiledSource
+{
+    internal class AssemblyResolver : IAssemblyResolver


📝 This was moved here without changes from MetadataAsSourceFileService.cs

sharwell · 2018-11-01T16:31:05Z

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceService.cs

+using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.DecompiledSource


➡️ Will file as a follow-up issue.

sharwell · 2018-11-01T16:31:54Z

src/EditorFeatures/CSharp/DecompiledSource/CSharpDecompiledSourceServiceFactory.cs

+    [ExportLanguageServiceFactory(typeof(IDecompiledSourceService), LanguageNames.CSharp), Shared]
+    internal partial class CSharpDecompiledSourceServiceFactory : ILanguageServiceFactory
+    {
+        public ILanguageService CreateLanguageService(HostLanguageServices provider)


➡️ Will file as a follow-up issue.

siegfriedpammer added 5 commits August 7, 2018 21:48

Do not show legal notice if decompilation is not available.

bbca32a

Move AssemblyResolver to separate file.

2d2abcd

dotnet#26436: Use "metadata-as-source" style for documentation commen…

5e9c2a9

…ts in decompilation.

Apply formatter to decompiled code.

6c1cc56

siegfriedpammer requested a review from a team as a code owner August 7, 2018 21:43

siegfriedpammer commented Aug 7, 2018

View reviewed changes

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Aug 8, 2018

jinujoseph added this to the 16.0 milestone Aug 8, 2018

jinujoseph assigned sharwell Aug 8, 2018