Add feature to show users what global-usings are in scope at the top of a file. by CyrusNajmabadi · Pull Request #60893 · dotnet/roslyn

CyrusNajmabadi · 2022-04-21T23:25:07Z

Depends on #59533. So Draft PR for now.

I'm reusing 'inheritance' margin because (if you squint) you're "inheriting" these using-directives from the global scope.

Looks like this:

When clicking on that item, you get:

Clicking on any item takes you to the declaration location for it:

If you have multiple files bringing in directives, this shows up as:

Todo:

Tests

CyrusNajmabadi · 2022-04-28T21:31:38Z

src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService.cs

-            if (allDeclarationNodes.IsEmpty)
+
+            var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
+            var imports = syntaxFacts.GetImportsOfCompilationUnit(root);


will likely need to move this OOP.

CyrusNajmabadi · 2022-04-28T21:32:33Z

src/VisualStudio/Core/Def/InheritanceMargin/MarginGlyph/MemberMenuItemViewModel.cs

-            => new(
-                member.DisplayTexts.JoinText(),
-                member.Glyph.GetImageMoniker(),
-                InheritanceMarginHelpers.CreateModelsForTargetItems(member.TargetItems));


inlined to single caller.

src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginTag.cs

davidwengier

you're "inheriting" these using-directives

I'll remember this. Next time you tell me I put something in the wrong place, you just watch out!

davidwengier · 2022-04-29T02:16:56Z

src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService.cs

+                        // project level imports come first.
+                        (null, not null) => -1,
+                        (not null, null) => 1,
+                        // both are from different files.  Sort by file path first, then location in file if same file path.


I wonder if this distinction is useful, or if just sorting them all alphabetically is just as good?

Related: By "project level" do you mean the VB global imports that are specified on the command line? Because if so, isn't it impossible for "project level imports" and "global usings" to appear int he same list, since they're relevant to different languages?
Update: After looking at the rest of the PR, yes, the above is true. So I guess my comment is "this could potentially just be simplified to sort by namespace and nobody will care". Potentially.

I wonder if this distinction is useful, or if just sorting them all alphabetically is just as good?

I def like sorting as per the items in a file, as that makes it feel like you're getting a little view into that file :)

Related: By "project level" do you mean the VB global imports that are specified on the command line? Because if so, isn't it impossible for "project level imports" and "global usings" to appear int he same list, since they're relevant to different languages?

you are correct. the above is a little bit of paranoia... i'll see if i can comfortably clean it up though!

Cosifne · 2022-04-29T06:34:54Z

src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginHelpers.cs

                InheritanceRelationship.OverriddenMember => ServicesVSResources.Overridden_members,
                InheritanceRelationship.OverridingMember => ServicesVSResources.Overriding_members,
                InheritanceRelationship.ImplementingMember => ServicesVSResources.Implementing_members,
+                InheritanceRelationship.InheritedImport => item.DisplayTexts.JoinText(),


Does this need localization?

Nope! this is already localized.

Cosifne · 2022-04-29T07:04:57Z

src/EditorFeatures/Core/InheritanceMargin/InheritanceMarginItem.cs

            DisplayTexts = displayTexts;
            Glyph = glyph;
-            TargetItems = targetItems;
+            TargetItems = isOrdered ? targetItems : targetItems.OrderBy(item => item.DisplayName).ToImmutableArray();


Seems like a place might be benefited from our offline discussion. (Move all the data processing part to OOP)
I am fine with this approach now, but could you add a comment here to explain isOrdered would only be used for showing the global using?

yup. i will move this OOP and add the comment.

Cosifne · 2022-04-29T07:06:48Z

src/EditorFeatures/Core/InheritanceMargin/InheritanceMarginItem.cs

        /// </summary>
        public readonly int LineNumber;

+        public readonly string? TopLevelDisplayText;


Similar comments as other comment, could you add a comment here to explain this would only be used showing global usings?
I would help when later I start refactoring this

Cosifne · 2022-04-29T07:16:40Z

InheritanceMargin is controlled by this Option https://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices/InheritanceMargin/InheritanceMarginTaggerProvider.cs,100

Do you feel it would be fine to also let this feature controlled by this option?
Also the generation time of the tag would be put under https://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices/InheritanceMargin/InheritanceMarginTaggerProvider.cs,128

Cosifne · 2022-04-29T07:25:26Z

The overall approach LGTM. I guess there are still some in progress works so just ping me after it's done I can take a second look.

Two potential future items (not blocking issue for this features) in my mind:

I feel generally we are bringing more and more contents to inheritance margin. This would not be a big problem for this PR, because namespace generally won't stay on the same line with class/method/property. But we might want to look for some general approach to handle the overlapping problem.
We need a big refactoring to move all things except the ViewModel to OOP : )

CyrusNajmabadi · 2022-04-29T17:22:03Z

Do you feel it would be fine to also let this feature controlled by this option?

i think i'll add an option for this.

CyrusNajmabadi · 2022-04-29T18:33:05Z

src/EditorFeatures/Core/InheritanceMargin/AbstractInheritanceMarginService.cs

@@ -1,107 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.


this was moved down to Features. Unfortunatelly, github seems to view it as a delete :(

CyrusNajmabadi · 2022-04-29T18:33:58Z

src/Features/CSharp/Portable/InheritanceMargin/CSharpInheritanceMarginService.cs

+namespace Microsoft.CodeAnalysis.CSharp.InheritanceMargin
 {
    [ExportLanguageService(typeof(IInheritanceMarginService), LanguageNames.CSharp), Shared]
    internal class CSharpInheritanceMarginService : AbstractInheritanceMarginService


these types moved to Features level. That way they can also be used by the remote service, while still utilizing inheritance (instead of statics).

CyrusNajmabadi · 2022-04-29T18:34:33Z

src/Features/Core/Portable/DocumentIdSpan.cs

+    [DataContract]
    internal readonly struct DocumentIdSpan
    {
-        private readonly Workspace _workspace;


remove workspace. That allowed thsi type to become serializable, which made it so all the types in inheritance margin can be serializable themselves (no need for a duplicate hierarchy).

CyrusNajmabadi · 2022-04-29T18:40:19Z

src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService.cs

+        public async ValueTask<ImmutableArray<InheritanceMarginItem>> GetInheritanceMemberItemsAsync(
+            Document document,
+            TextSpan spanToSearch,
+            CancellationToken cancellationToken)


this was a move, but the logic here did change somewhat. namely that we can still call into OOP, even if we have no membersymbolkeys (since we may have global imports).

CyrusNajmabadi · 2022-04-29T18:41:06Z

src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService.cs

+        private async ValueTask<(Project remapped, SymbolKeyAndLineNumberArray symbolKeyAndLineNumbers)> GetMemberSymbolKeysAsync(
+            Document document,
+            TextSpan spanToSearch,
+            CancellationToken cancellationToken)


this was an extract method. however, isntead of calling into oop, it determines what data it wants to call into oop with.

CyrusNajmabadi · 2022-04-29T18:41:45Z

src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService_Helpers.cs

 namespace Microsoft.CodeAnalysis.InheritanceMargin
 {
-    internal static class InheritanceMarginServiceHelper
+    internal abstract partial class AbstractInheritanceMarginService


this became non-static and a partial part of AbstractInheritanceMarginService. taht way it can benefit from inheritance for when we want C# and VB to differ in behavior (for example, titles of htings).

CyrusNajmabadi · 2022-04-29T18:42:10Z

src/Features/Core/Portable/InheritanceMargin/AbstractInheritanceMarginService_Helpers.cs

+            TextSpan spanToSearch,
+            ArrayBuilder<InheritanceMarginItem> items,
+            CancellationToken cancellationToken)
+        {


this is new logic for this PR.

CyrusNajmabadi · 2022-04-29T18:43:04Z

src/Features/Core/Portable/InheritanceMargin/InheritanceMarginItem.cs

 namespace Microsoft.CodeAnalysis.InheritanceMargin
 {
+    [DataContract]
    internal readonly struct InheritanceMarginItem


made these types into message-pack compatible types. this allowes us to get rid of the SerializableXXX siblings.

…ag.cs Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

…i/roslyn into globalUsingsMargin

CyrusNajmabadi · 2022-04-29T19:28:06Z

@Cosifne this is ready for review.

CyrusNajmabadi added 4 commits April 21, 2022 14:55

Show information about global usings in the inheritance margin

14b1496

In progress

3f25f76

In progress

b2e10ba

In progress

aabb3a1

ghost added the Area-Compilers label Apr 21, 2022

Simplify

856072f

CyrusNajmabadi changed the title ~~WIP. Global usings.~~ Add feature to show users what global-usings are in scope at the top of a file. Apr 22, 2022

Better hover text

9043752

jcouv added Area-IDE and removed Area-Compilers labels Apr 22, 2022

CyrusNajmabadi added 2 commits April 28, 2022 10:04

Merge branch 'compilationImports' into globalUsingsMargin

e07ade1

Merge remote-tracking branch 'upstream/main' into globalUsingsMargin

b8e4d09

CyrusNajmabadi marked this pull request as ready for review April 28, 2022 21:30

CyrusNajmabadi requested a review from a team as a code owner April 28, 2022 21:30

CyrusNajmabadi requested a review from Cosifne April 28, 2022 21:30

CyrusNajmabadi commented Apr 28, 2022

View reviewed changes

jinujoseph added the Feature Request label Apr 28, 2022

JoeRobich reviewed Apr 28, 2022

View reviewed changes

src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginTag.cs Outdated Show resolved Hide resolved

davidwengier approved these changes Apr 29, 2022

View reviewed changes

Cosifne reviewed Apr 29, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into globalUsingsMargin

8d3337d

CyrusNajmabadi added 3 commits April 29, 2022 10:56

Move import computation to OOP

eb3bbfe

Move types down

8fb7f28

Loc strings

d267201

CyrusNajmabadi commented Apr 29, 2022

View reviewed changes

Remove

5d6e1fa

CyrusNajmabadi commented Apr 29, 2022

View reviewed changes

rename

0ad9260

CyrusNajmabadi commented Apr 29, 2022

View reviewed changes

CyrusNajmabadi and others added 8 commits April 29, 2022 11:44

Update src/VisualStudio/Core/Def/InheritanceMargin/InheritanceMarginT…

85c60a0

…ag.cs Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com>

Don't order in the view, order in the data model

a846dfc

Cleanup test

acca286

Merge branch 'globalUsingsMargin' of https://github.com/CyrusNajmabad…

8fa8d1c

…i/roslyn into globalUsingsMargin

Sort

dc2c35b

Add VB tests

d15e221

Add tests

1cf839d

Add option to enable/disable new feature

3386cc0

CyrusNajmabadi requested review from Cosifne and davidwengier April 29, 2022 19:27

CyrusNajmabadi added 2 commits April 29, 2022 12:28

Merge remote-tracking branch 'upstream/main' into globalUsingsMargin

a9cbd48

Fix

99f047f

CyrusNajmabadi requested a review from a team as a code owner April 29, 2022 19:32

CyrusNajmabadi merged commit 6d54bd9 into dotnet:main Apr 30, 2022

ghost added this to the Next milestone Apr 30, 2022

CyrusNajmabadi deleted the globalUsingsMargin branch April 30, 2022 02:03

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Cosifne mentioned this pull request Jul 5, 2022

Add an Integration test for 'Go to global usings' #62217

Merged

		@@ -1,107 +0,0 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

CyrusNajmabadi commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 29, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cosifne Apr 29, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Cosifne commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cosifne commented Apr 29, 2022

Uh oh!

CyrusNajmabadi commented Apr 29, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CyrusNajmabadi commented Apr 21, 2022 •

edited

Loading

davidwengier Apr 29, 2022 •

edited

Loading

Cosifne Apr 29, 2022 •

edited

Loading

Cosifne commented Apr 29, 2022 •

edited

Loading