Refactor BlockStructure service and providers by mavasani · Pull Request #49264 · dotnet/roslyn

mavasani · 2020-11-10T16:50:30Z

This change refactors ~~and moves~~ the BlockStructure service and providers ~~to the shared Workspace utilities layer~~. The core APIs are now based off SyntaxTree instead of Document to allow the service to be used from an analyzer context, which is required for #49197.

This change refactors and moves the BlockStructure service and providers to the shared Workspace utilities layer. The core APIs are now based off SyntaxTree instead of Document to allow the service to be used from an analyzer context, which is required for dotnet#49197.

mavasani · 2020-11-10T16:53:01Z

...paces/SharedUtilitiesAndExtensions/Compiler/Core/Services/Structure/BlockStructureHelpers.cs

 namespace Microsoft.CodeAnalysis.Structure
 {
-    internal abstract class BlockStructureServiceWithProviders : BlockStructureService
+    internal static class BlockStructureHelpers


BlockStructureServiceWithProviders was split to move the core structure creation APIs to BlockStructureHelpers helper class. All the code shown as deleted in this file is just in a separate file BlockStructureServiceWithProviders.cs

mavasani · 2020-11-10T16:54:42Z

...esAndExtensions/Compiler/VisualBasic/Services/Structure/VisualBasicBlockStructureProvider.vb

 Imports Microsoft.CodeAnalysis.Structure
 Imports Microsoft.CodeAnalysis.VisualBasic.Syntax

 Namespace Microsoft.CodeAnalysis.VisualBasic.Structure
-    <ExportLanguageServiceFactory(GetType(BlockStructureService), LanguageNames.VisualBasic), [Shared]>


All deleted code just moved to a separate file.

mavasani · 2020-11-10T16:55:27Z

...ndExtensions/Workspace/Core/LanguageServices/Structure/BlockStructureServiceWithProviders.cs

+
+namespace Microsoft.CodeAnalysis.Structure
+{
+    internal abstract class BlockStructureServiceWithProviders : BlockStructureService


All existing code, except CreateContextAsync at the end of this file.

mavasani · 2020-11-10T16:55:42Z

...tiesAndExtensions/Workspace/VisualBasic/LanguageServices/VisualBasicBlockStructureService.vb

+
+Namespace Microsoft.CodeAnalysis.VisualBasic.Structure
+    <ExportLanguageServiceFactory(GetType(BlockStructureService), LanguageNames.VisualBasic), [Shared]>
+    Friend Class VisualBasicBlockStructureServiceFactory


All existing code.

CyrusNajmabadi · 2020-11-10T17:41:22Z

This change refactors and moves the BlockStructure service and providers to the shared Workspace utilities layer

can we move to Features level instead? Does this need to be in workspace layer itself?

CyrusNajmabadi · 2020-11-10T17:43:52Z

...haredUtilitiesAndExtensions/Compiler/Core/Services/Structure/BlockStructureOptionProvider.cs

+
+        public bool IsMetadataAsSource { get; }
+
+        public T GetOption<T>(PerLanguageOption2<T> option, string language)


i'm curious what other options we need. should we just have a BlockStructureOptions that contains the bools that we need to look at while processing?

CyrusNajmabadi · 2020-11-10T17:56:06Z

Just to check, but TS/JS will still work with this right? We still have the same plugin architecture for them there? And only the C#/VB impl now defers to helpers which can now be shared?

mavasani · 2020-11-10T18:01:01Z

Just to check, but TS/JS will still work with this right? We still have the same plugin architecture for them there? And only the C#/VB impl now defers to helpers which can now be shared?

Yes, nothing changes for any languages with this PR. Only thing this PR does is now we can potentially invoke BlockStructureHelpers.GetStructure from analyzer layer (whenever we convert LSIF generator to an analyzer) for C# and VB.

mavasani · 2020-11-10T18:07:10Z

can we move to Features level instead? Does this need to be in workspace layer its

Yes, as we discussed offline, I'll do the following:

Add a new shared Features layer that gets included in Features projects and CodeStyle projects
Open a follow-up design issue to convert all the IDE shared projects into separate utility projects with public APIs with weak public API support. These projects will have no IVTs and will be used by Workspaces/Features and CodeStyle Analyzers/Fixes respectively. This would likely need RPS approval upfront as we will be loading bunch of additional assemblies on VS startup (WorkspaceUtilities, FeaturesUtilities, CSharpWorkspaceUtilities (and/or BasicWorkspaceUtilities) and CSharpFeaturesUtilities (and/or BasicFeaturesUtilities).

mavasani · 2020-11-11T16:47:41Z

@CyrusNajmabadi I will do the refactoring to move the shared functionality into a separate shared project with a follow up PR. It also gives us time to ensure everyone on the team is fine with it (have started an email thread with the IDE team to get consensus). I will like to merge this PR to unblock #49197

sharwell · 2020-11-11T17:32:34Z

Note that the block structure service is obsolete and we have a long-standing request to change to the new APIs. I forget the name of the new API so I was not able to find the tracking issue in search.

CyrusNajmabadi · 2020-11-11T18:06:29Z

Note that the block structure service is obsolete and we have a long-standing request to change to the new APIs

The block structure stuff is our internal API for ths. It still remains valid (and independent of any host). There is a request to hook this up to the new API on the editor side. However, something blocking that on our side has been a request or us to tie information to view-oriented coordinates. Something which we haven't wanted to do as we want our APIs to be view independent.

mavasani · 2020-11-13T16:09:57Z

@CyrusNajmabadi I have reverted all the file moves to shared layer - all block structure code now lies in the Features layer itself.

ghost · 2020-11-13T16:10:27Z

Hello @mavasani!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

mavasani commented Nov 10, 2020

View reviewed changes

CyrusNajmabadi reviewed Nov 10, 2020

View reviewed changes

CyrusNajmabadi approved these changes Nov 10, 2020

View reviewed changes

Fix tests

cb49d0a

mavasani marked this pull request as ready for review November 11, 2020 16:47

mavasani requested a review from a team as a code owner November 11, 2020 16:47

mavasani added 4 commits November 13, 2020 08:02

Move block structure code back to Features layer

a36daf3

Revert file

4790c53

Remove blank line

df564e6

Revert pragma suppression

5277ab7

mavasani added the Area-IDE label Nov 13, 2020

mavasani added this to the 16.9 milestone Nov 13, 2020

mavasani added the auto-merge label Nov 13, 2020

ghost approved these changes Nov 13, 2020

View reviewed changes

ghost merged commit 75b0f7b into dotnet:master Nov 13, 2020

ghost modified the milestones: 16.9, Next Nov 13, 2020

mavasani deleted the RefactorBlockStructureService branch November 13, 2020 17:41

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

This pull request was closed.


		public bool IsMetadataAsSource { get; }

		public T GetOption<T>(PerLanguageOption2<T> option, string language)

Conversation

mavasani commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavasani Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

mavasani Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 10, 2020

Uh oh!

CyrusNajmabadi Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 10, 2020

Uh oh!

mavasani commented Nov 10, 2020

Uh oh!

mavasani commented Nov 10, 2020

Uh oh!

mavasani commented Nov 11, 2020

Uh oh!

sharwell commented Nov 11, 2020

Uh oh!

CyrusNajmabadi commented Nov 11, 2020

Uh oh!

mavasani commented Nov 13, 2020

Uh oh!

ghost commented Nov 13, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mavasani commented Nov 10, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.