Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

Closed
wants to merge 22 commits into from
Closed

Share ISyntaxFacts/SyntaxGenerator with CodeStyle layer. #41376

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2af9d32
Share ISyntaxFacts with CodeStyle layer.
CyrusNajmabadi Feb 3, 2020
f2ab78f
Share more.
CyrusNajmabadi Feb 3, 2020
645f413
Add Kinds service.
CyrusNajmabadi Feb 3, 2020
fe84de2
Revert
CyrusNajmabadi Feb 3, 2020
2dcaa98
Moving more down
CyrusNajmabadi Feb 3, 2020
bb9b719
Move more down
CyrusNajmabadi Feb 3, 2020
56084e8
Share more
CyrusNajmabadi Feb 4, 2020
a4b4131
Share more
CyrusNajmabadi Feb 4, 2020
e037e13
Share more
CyrusNajmabadi Feb 4, 2020
9e537c7
Fix
CyrusNajmabadi Feb 4, 2020
4870d47
Fixes
CyrusNajmabadi Feb 4, 2020
64bb7a7
Fix
CyrusNajmabadi Feb 4, 2020
d506fb1
Move back
CyrusNajmabadi Feb 4, 2020
87cc476
Move back
CyrusNajmabadi Feb 4, 2020
6d1ddd6
Share VB code.
CyrusNajmabadi Feb 4, 2020
115863a
Share VB code.
CyrusNajmabadi Feb 4, 2020
d46d186
More VB sharing
CyrusNajmabadi Feb 4, 2020
bd2640d
Move files
CyrusNajmabadi Feb 4, 2020
07ded01
Move files
CyrusNajmabadi Feb 4, 2020
14a95f9
Merge remote-tracking branch 'upstream/master' into sharedSyntaxFacts
CyrusNajmabadi Feb 10, 2020
5e9a6ad
Fix
CyrusNajmabadi Feb 10, 2020
aa3ca17
Merge
CyrusNajmabadi Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions src/CodeStyle/CSharp/Analyzers/Microsoft.CodeAnalysis.CSharp.CodeStyle.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,31 @@
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.CSharp.CodeStyle.UnitTests" />
</ItemGroup>
<ItemGroup>
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Formatting\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" Link="Formatting\MemberDeclarationSyntaxExtensions_GetAttributes.cs" />
<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" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

<Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\CSharpSyntaxGenerator.cs" Link="CodeGeneration\CSharpSyntaxGenerator.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\CodeGeneration\ExpressionGenerator.cs" Link="CodeGeneration\ExpressionGenerator.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ArgumentSyntaxExtensions.cs" Link="Extensions\ArgumentSyntaxExtensions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\AssignmentExpressionSyntaxExtensions.cs" Link="Extensions\AssignmentExpressionSyntaxExtensions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Extensions\ContextQuery\SyntaxTokenExtensions_SharedWithCodeStyle.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ContextQuery\SyntaxTreeExtensions_Shared.cs" Link="Extensions\ContextQuery\SyntaxTreeExtensions_Shared.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.cs" Link="Extensions\DirectiveSyntaxExtensions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveInfo.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveInfo.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveSyntaxEqualityComparer.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveSyntaxEqualityComparer.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\DirectiveSyntaxExtensions.DirectiveWalker.cs" Link="Extensions\DirectiveSyntaxExtensions.DirectiveWalker.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ExpressionSyntaxExtensions_Shared.cs" Link="Extensions\ExpressionSyntaxExtensions_Shared.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs" Link="Extensions\ITypeSymbolExtensions.TypeSyntaxGeneratorVisitor.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\ITypeSymbolExtensions_Shared.cs" Link="Extensions\ITypeSymbolExtensions_Shared.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" Link="Extensions\MemberDeclarationSyntaxExtensions_GetAttributes.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\MemberDeclarationSyntaxExtensions_Shared.cs" Link="Extensions\MemberDeclarationSyntaxExtensions_Shared.cs" />
<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" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" Link="Extensions\SyntaxTokenExtensions_SharedWithCodeStyle.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxNodeExtensions.SingleLineRewriter.cs" Link="Extensions\SyntaxNodeExtensions.SingleLineRewriter.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTreeExtensions_Shared.cs" Link="Extensions\SyntaxTreeExtensions_Shared.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTriviaExtensions.cs" Link="Extensions\SyntaxTriviaExtensions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Extensions\SyntaxTriviaListExtensions.cs" Link="Extensions\SyntaxTriviaListExtensions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpFormattingOptions.cs" Link="Formatting\CSharpFormattingOptions.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpFormattingOptions.Parsers.cs" Link="Formatting\CSharpFormattingOptions.Parsers.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\CSharpSyntaxFormattingService.cs" Link="Formatting\CSharpSyntaxFormattingService.cs" />
Expand Down Expand Up @@ -56,7 +76,10 @@
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\SyntaxKindEx.cs" Link="Formatting\Rules\SyntaxKindEx.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\TokenBasedFormattingRule.cs" Link="Formatting\Rules\TokenBasedFormattingRule.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Formatting\Rules\WrappingFormattingRule.cs" Link="Formatting\Rules\WrappingFormattingRule.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Utilities\FormattingRangeHelper.cs" Link="Formatting\FormattingRangeHelper.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpDocumentationCommentService.cs" Link="LanguageServices\CSharpDocumentationCommentService.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpSyntaxFactsService.cs" Link="LanguageServices\CSharpSyntaxFactsService.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\LanguageServices\CSharpSyntaxKindsService.cs" Link="LanguageServices\CSharpSyntaxKindsService.cs" />
<Compile Include="..\..\..\Workspaces\CSharp\Portable\Utilities\FormattingRangeHelper.cs" Link="Utilities\FormattingRangeHelper.cs" />
</ItemGroup>
<ItemGroup>
<Compile Update="CSharpCodeStyleResources.Designer.cs">
Expand All @@ -76,6 +99,11 @@
<PublicAPI Include="PublicAPI.Shipped.txt" />
<PublicAPI Include="PublicAPI.Unshipped.txt" />
</ItemGroup>
<ItemGroup>
<Folder Include="Extensions\ContextQuery\" />
<Folder Include="LanguageServices\" />
<Folder Include="CodeGeneration\" />
</ItemGroup>
<Import Project="..\..\..\Workspaces\SharedUtilitiesAndExtensions\Compiler\CSharp\CSharpCompilerExtensions.projitems" Label="Shared" />
<Import Project="..\..\..\Analyzers\CSharp\Analyzers\CSharpAnalyzers.projitems" Label="Shared" />
</Project>
5 changes: 5 additions & 0 deletions src/CodeStyle/Core/Analyzers/Formatting/Formatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ namespace Microsoft.CodeAnalysis.Formatting
/// </summary>
internal static class Formatter
{
/// <summary>
/// The annotation used to mark portions of a syntax tree to be formatted.
/// </summary>
public static SyntaxAnnotation Annotation { get; } = new SyntaxAnnotation();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Gets the formatting rules that would be applied if left unspecified.
/// </summary>
Expand Down
Loading