Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

CyrusNajmabadi · 2020-02-03T22:48:39Z

No description provided.

CyrusNajmabadi · 2020-02-03T22:48:56Z

Tagging @mavasani . This is the approach i'm talking about. Will continue trying to make this fully fleshed out.

mavasani · 2020-02-03T23:00:36Z

@CyrusNajmabadi I think you also need to link in the implementation types and the extension methods used by the implementations, which means ensuring that the transitive closure of APIs invoked from syntax facts service or its C# and VB implementations are linked into CodeStyle layer + none of these have any Workspace dependencies, otherwise they will fail at runtime from command line.

Btw, I don't like all the explicit linked files in code style projects, and am proposing switching to shared projects in #41363, but we unfortunately did not get to discussing that approach in today's design meeting. For example, if you checkout my branch from that PR, you can just drag and drop ISyntaxFactsService and all its implementations from WorkspacesExtensions.shproj to CompilersExtension.shproj and go from there to move rest of the accessed extensions into CompilersExtension.shproj.

CyrusNajmabadi · 2020-02-03T23:07:30Z

@CyrusNajmabadi I think you also need to link in the implementation types and the extension methods used by the implementations, which means ensuring that the transitive closure of APIs invoked from syntax facts service or its C# and VB implementations are linked into CodeStyle layer + none of these have any Workspace dependencies, otherwise they will fail at runtime from command line.

Yup. It's a WIP. Will be updating as a go along.

Btw, I don't like all the explicit linked files in code style projects, and am proposing switching to shared projects

That woudl also work for me. How the files come in is a separate concern. I just want to make sure all the code is 'workspace clean'. :)

CyrusNajmabadi · 2020-02-03T23:07:38Z

if you want, we can also gitter chat.

sharwell · 2020-02-03T23:32:19Z

src/Workspaces/Core/Portable/Simplification/Simplifier.cs

-        /// The annotation the reducer uses to identify sub trees to be reduced.
-        /// The Expand operations add this annotation to nodes so that the Reduce operations later find them.
-        /// </summary>
-        public static SyntaxAnnotation Annotation { get; } = new SyntaxAnnotation();


😕 This is a public API. Why are we duplicating it?

CyrusNajmabadi · 2020-02-04T00:54:38Z

Ok. C# seems done-ish. Working on VB side now.

CyrusNajmabadi · 2020-02-04T00:58:01Z

src/CodeStyle/Core/Analyzers/Formatting/Formatter.cs

+        /// <summary>
+        /// The annotation used to mark portions of a syntax tree to be formatted.
+        /// </summary>
+        public static SyntaxAnnotation Annotation { get; } = new SyntaxAnnotation();


note: i'm taking this approach because we have code that i'm sharing that does thing like add these annotations. I could attempt to eitehr not share that code (which would mean ore shuffling around) or #ifdef it. I chose to do this for expediency as i don't really see any issue with annotations being added.

CyrusNajmabadi · 2020-02-04T00:58:55Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

@@ -333,13 +333,13 @@ public static bool IsStaticallyDeclared(SyntaxToken token)
                parentNode = parentNode!.Parent!.Parent;

                // Check if this is a field constant declaration 
-                if (parentNode.GetModifiers().Any(SyntaxKind.ConstKeyword))
+                if (parentNode!.GetModifiers().Any(SyntaxKind.ConstKeyword))


GetModifiers moved around nullable contexts. wasn't sure the best thing do to here. However, given the heavy use of ! already, this seemed like an acceptable quickpatch.

specifically, i didn't want to change runtime behavior. so i just suppressed to keep things safe.

CyrusNajmabadi · 2020-02-04T00:59:14Z

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/SyntaxTreeExtensions_Shared.cs

+        private static SyntaxToken FindTokenOnLeftOfNode(SyntaxNode node)
+        {
+            return node.FindTokenOnLeftOfPosition(node.SpanStart);
+        }


code was moved without touching semantics.

CyrusNajmabadi · 2020-02-04T00:59:58Z

src/Workspaces/CSharp/Portable/Extensions/ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs

@@ -88,6 +88,7 @@ public override TypeSyntax VisitArrayType(IArrayTypeSymbol symbol)

                        break;
                    }
+#endif


this is one area i did need to change code. That's beacuse .NullableAnnotation doesn't seem to be avialable in this layer yet. i assume we would have to wait to be able to reference a more recent compiler version.

if we move forward with this PR we should have a tracking issue to remove these.

CyrusNajmabadi · 2020-02-04T01:06:12Z

src/Workspaces/CSharp/Portable/Extensions/SyntaxTokenExtensions.cs

-        public static bool IntersectsWith(this SyntaxToken token, int position)
-        {
-            return token.Span.IntersectsWith(position);
-        }


note: i unified this with VB and moved to common layer. there is nothing language specific with this particular extension.

CyrusNajmabadi · 2020-02-04T03:10:49Z

This is ready for review @mavasani @sharwell.

CyrusNajmabadi · 2020-02-10T21:47:34Z

src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs

-        [ImportingConstructor]
-        public CSharpSyntaxGenerator()
-        {
-        }


note that this is moved into a separate part. This allows hte data-only portion of this type (i.e. all of it by the mef stuff) to be used as a singleton in the codestyle layer. The workspcae layer then languageservice-izes it so it can be imported from that point upwards.

CyrusNajmabadi · 2020-02-10T21:49:02Z

src/Workspaces/CSharp/Portable/Extensions/ContextQuery/SyntaxTreeExtensions_Shared.cs

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


all the files intended to be shared end with the _Shared.cs suffix. I believe these should all be pure splits into partial types, and pure moves of the code.

CyrusNajmabadi · 2020-02-10T21:54:31Z

@sharwell @mavasani this is ready for re-review. This change moves (CSharp|VB)SyntaxGenerator, SyntaxFacts, SyntaxKinds and associated helper classes/extensions down into the CodeStyle layer to be used as statics/singletons.

99.9% of hte change is purely mechanical moves of code into files that can then be shared between the two layers. There is a tiny amount of changing that is very narrowly scoped to make this possible:

ifdefing code out that requires the latest nullable apis to be available from the compiler. We should make a tracking bug to remove these ifdefs.
breaking out the mef portion of code and the non-mef portion. The mef portoin continues to live at the workspaces layer (and eventually the codestyle.fixer layer), but will not be in the codestyle.analyzer layer.

CyrusNajmabadi · 2020-02-10T21:54:54Z

I've merged all the latest changes we've all made into this. So this can go in safely into master.

Ths change alone will drop #41363 down by a third :)

mavasani · 2020-02-10T22:04:32Z

src/CodeStyle/CSharp/Analyzers/Microsoft.CodeAnalysis.CSharp.CodeStyle.csproj

-    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" Link="Formatting\SyntaxNodeExtensions_SharedWithCodeStyle.cs" />
-    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Formatting\SyntaxTokenExtensions_SharedWithCodeStyle.cs" />
-    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTriviaExtensions.cs" Link="Formatting\SyntaxTriviaExtensions.cs" />
+    <Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpFlagsEnumGenerator.cs" Link="CodeGeneration\CSharpFlagsEnumGenerator.cs" />


We want all of these now to happen via the newly added shared projects, not with linked files.

Sounds good. Let me figure out how to do that. thanks!

Should we do the split first, the move all the files over into the shared projects? Looking at the projects now, it still looks like they use linked files. Seems like we can two phase this.

benefit of that approach is that the sharedproject pr should have no actual code changes, just project structure changes.

You should be able just create a folder named "Extensions" in the required shared projects under: https://github.com/dotnet/roslyn/tree/master/src/Workspaces/SharedUtilitiesAndExtensions. At this point you don't even need to name the files in shared projects end with _Shared or _SharedWithCodeStyle suffix.

My refactoring PR already removes all the linked files from CodeStyle layer projects, so it might be easier if that went in?

Let me update my PR with the latest sources from master, and you can see if it makes sense.

mavasani · 2020-02-10T22:09:10Z

src/CodeStyle/CSharp/Analyzers/Microsoft.CodeAnalysis.CSharp.CodeStyle.csproj

+    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\StatementSyntaxExtensions.cs" Link="Extensions\StatementSyntaxExtensions.cs" />
+    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\StringExtensions.cs" Link="Extensions\StringExtensions.cs" />
+    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxListExtensions.cs" Link="Extensions\SyntaxListExtensions.cs" />
+    <Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" Link="Extensions\SyntaxNodeExtensions_SharedWithCodeStyle.cs" />


These will certainly clash with my refactoring PR as I have split and moved bunch of these into the shared layer as well. One of us will have to deal with merge conflicts in few of files being added to shared layer now.

Yes. I think that is def the case. But ideally you shouldn't need to split/move these down yourself. the only splitting/moving would be for some extensions that weren't captured in this that are used by some other features?

but in tha tcase, i would argue again that they shouldn't be in that big PR. we should pull them out (like the unnecessary-imports part) so that none of that is in the big PR).

but in tha tcase, i would argue again that they shouldn't be in that big PR. we should pull them out

The refactoring PR is now just doing this exact thing: just splitting and moving down extensions into shared layers. Your PR here is doing so for SyntaxGenerator and SyntaxFacts and my PR is doing so for other features. I think the key is we should avoid these efforts in parallel as there will be overlap that leads to big merge conflicts. I would recommend waiting on this PR till the primary refactoring PR goes in.

Note: it may be hte case that that's what is going on in that PR... but it's also 8k more adds and 3k more deletes. So it's mixed in with a bunch of other stuff. We might want to consider how this could just go in, and then have the remaining stuff be teh final bit we need to review in isolation.

mavasani · 2020-02-28T03:02:17Z

@CyrusNajmabadi Are you planning to continue with this work? Otherwise, I can take this up following the same model that you started with here.

CyrusNajmabadi · 2020-02-28T03:10:47Z

@mavasani i think trying to merge this would be a bit too painful at this point. I'm ok with you taking this :)

Share ISyntaxFacts with CodeStyle layer.

2af9d32

CyrusNajmabadi requested a review from a team as a code owner February 3, 2020 22:48

CyrusNajmabadi added 3 commits February 3, 2020 14:55

Share more.

f2ab78f

Add Kinds service.

645f413

Revert

fe84de2

Moving more down

2dcaa98

sharwell reviewed Feb 3, 2020

View reviewed changes

CyrusNajmabadi added 5 commits February 3, 2020 15:36

Move more down

bb9b719

Share more

56084e8

Share more

a4b4131

Share more

e037e13

Fix

9e537c7

CyrusNajmabadi commented Feb 4, 2020

View reviewed changes

CyrusNajmabadi added 2 commits February 3, 2020 17:02

Fixes

4870d47

Fix

64bb7a7

CyrusNajmabadi commented Feb 4, 2020

View reviewed changes

Move back

d506fb1

CyrusNajmabadi requested a review from a team as a code owner February 4, 2020 01:09

CyrusNajmabadi added 4 commits February 3, 2020 17:09

Move back

87cc476

Share VB code.

6d1ddd6

Share VB code.

115863a

More VB sharing

d46d186

CyrusNajmabadi added 2 commits February 3, 2020 19:14

Move files

bd2640d

Move files

07ded01

CyrusNajmabadi changed the title ~~WIP: Share ISyntaxFacts with CodeStyle layer.~~ Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. Feb 4, 2020

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Feb 4, 2020

jinujoseph assigned sharwell Feb 4, 2020

CyrusNajmabadi mentioned this pull request Feb 7, 2020

Port few IDE analyzers/fixers to shared layer to be enabled in CodeStyle NuGet package #41363

Closed

CyrusNajmabadi added 2 commits February 10, 2020 13:42

Merge remote-tracking branch 'upstream/master' into sharedSyntaxFacts

14a95f9

Fix

5e9a6ad

CyrusNajmabadi commented Feb 10, 2020

View reviewed changes

Merge

aa3ca17

mavasani reviewed Feb 10, 2020

View reviewed changes

CyrusNajmabadi closed this Feb 28, 2020

CyrusNajmabadi deleted the sharedSyntaxFacts branch April 11, 2021 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

CyrusNajmabadi commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

mavasani commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

sharwell Feb 3, 2020

CyrusNajmabadi Feb 3, 2020

CyrusNajmabadi commented Feb 4, 2020

CyrusNajmabadi Feb 4, 2020

CyrusNajmabadi Feb 4, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi Feb 4, 2020

CyrusNajmabadi Feb 4, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi Feb 4, 2020

CyrusNajmabadi commented Feb 4, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi commented Feb 10, 2020

CyrusNajmabadi commented Feb 10, 2020 •

edited

Loading

mavasani Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

mavasani Feb 10, 2020

mavasani Feb 10, 2020

mavasani Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

mavasani Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

mavasani Feb 10, 2020

CyrusNajmabadi Feb 10, 2020

mavasani commented Feb 28, 2020

CyrusNajmabadi commented Feb 28, 2020

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

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

Conversation

CyrusNajmabadi commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

mavasani commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

CyrusNajmabadi commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 10, 2020

CyrusNajmabadi commented Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani commented Feb 28, 2020

CyrusNajmabadi commented Feb 28, 2020

CyrusNajmabadi commented Feb 10, 2020 •

edited

Loading