From 245b05b170f8556544eb3a0101f3eb08a692e5eb Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Fri, 22 Nov 2024 10:35:52 -0600 Subject: [PATCH 1/2] Figuring out a way to get syntax tree validation happy when we add a trailing comma before a trailing comment closes #1388 --- Src/CSharpier.Benchmarks/Program.cs | 1 + Src/CSharpier.Cli/CommandLineFormatter.cs | 1 + .../Controllers/FormatController.cs | 1 + .../CommandLineFormatterTests.cs | 20 +++++++++++++ .../FormattingTests/BaseTest.cs | 1 + Src/CSharpier.Tests/Samples/Samples.cs | 1 + .../SyntaxNodeComparerTests.cs | 28 ++++++++++++++++++- Src/CSharpier/CSharpFormatter.cs | 3 ++ Src/CSharpier/CodeFormatterResult.cs | 1 + Src/CSharpier/SyntaxNodeComparer.cs | 10 +++++-- .../SyntaxPrinter/FormattingContext.cs | 4 +++ Src/CSharpier/SyntaxPrinter/Token.cs | 1 + 12 files changed, 69 insertions(+), 3 deletions(-) diff --git a/Src/CSharpier.Benchmarks/Program.cs b/Src/CSharpier.Benchmarks/Program.cs index 08bab192a..d487de6a6 100644 --- a/Src/CSharpier.Benchmarks/Program.cs +++ b/Src/CSharpier.Benchmarks/Program.cs @@ -24,6 +24,7 @@ public void Default_SyntaxNodeComparer() this.code, false, false, + false, SourceCodeKind.Regular, CancellationToken.None ); diff --git a/Src/CSharpier.Cli/CommandLineFormatter.cs b/Src/CSharpier.Cli/CommandLineFormatter.cs index 1b80ef17c..e56c050ab 100644 --- a/Src/CSharpier.Cli/CommandLineFormatter.cs +++ b/Src/CSharpier.Cli/CommandLineFormatter.cs @@ -441,6 +441,7 @@ CancellationToken cancellationToken codeFormattingResult.Code, codeFormattingResult.ReorderedModifiers, codeFormattingResult.ReorderedUsingsWithDisabledText, + codeFormattingResult.MovedTrailingTrivia, sourceCodeKind, cancellationToken ); diff --git a/Src/CSharpier.Playground/Controllers/FormatController.cs b/Src/CSharpier.Playground/Controllers/FormatController.cs index 07a74a617..1464b15c1 100644 --- a/Src/CSharpier.Playground/Controllers/FormatController.cs +++ b/Src/CSharpier.Playground/Controllers/FormatController.cs @@ -80,6 +80,7 @@ CancellationToken cancellationToken result.Code, result.ReorderedModifiers, result.ReorderedUsingsWithDisabledText, + result.MovedTrailingTrivia, sourceCodeKind, cancellationToken ); diff --git a/Src/CSharpier.Tests/CommandLineFormatterTests.cs b/Src/CSharpier.Tests/CommandLineFormatterTests.cs index 94c5748dc..f517e29d1 100644 --- a/Src/CSharpier.Tests/CommandLineFormatterTests.cs +++ b/Src/CSharpier.Tests/CommandLineFormatterTests.cs @@ -679,6 +679,26 @@ public void File_With_Reorder_Modifiers_In_If_Directive_Should_Pass_Validation(s result.OutputLines.First().Should().StartWith("Formatted 1 files in"); } + [Test] + public void File_With_Added_Trailing_Comma_Before_Comment_Should_Pass_Validation() + { + var context = new TestContext(); + + var fileContents = """ + var someObject = new SomeObject() + { + Property1 = 1, + Property2 = 2 // Trailing Comment + }; + """; + context.WhenAFileExists("file1.cs", fileContents); + + var result = this.Format(context); + + result.ErrorOutputLines.Should().BeEmpty(); + result.OutputLines.First().Should().StartWith("Formatted 1 files in"); + } + [TestCase(".csharpierrc")] [TestCase(".csharpierrc.json")] [TestCase(".csharpierrc.yaml")] diff --git a/Src/CSharpier.Tests/FormattingTests/BaseTest.cs b/Src/CSharpier.Tests/FormattingTests/BaseTest.cs index 5c6ad7da6..cbb8f8d30 100644 --- a/Src/CSharpier.Tests/FormattingTests/BaseTest.cs +++ b/Src/CSharpier.Tests/FormattingTests/BaseTest.cs @@ -63,6 +63,7 @@ protected async Task RunTest(string fileName, string fileExtension, bool useTabs normalizedCode, false, false, + false, SourceCodeKind.Regular, CancellationToken.None ); diff --git a/Src/CSharpier.Tests/Samples/Samples.cs b/Src/CSharpier.Tests/Samples/Samples.cs index 8cc6e5f76..a2c587fe5 100644 --- a/Src/CSharpier.Tests/Samples/Samples.cs +++ b/Src/CSharpier.Tests/Samples/Samples.cs @@ -43,6 +43,7 @@ public static async Task RunTest(string fileName) result.Code, false, false, + false, SourceCodeKind.Regular, CancellationToken.None ); diff --git a/Src/CSharpier.Tests/SyntaxNodeComparerTests.cs b/Src/CSharpier.Tests/SyntaxNodeComparerTests.cs index b4ae576e2..343e0fb71 100644 --- a/Src/CSharpier.Tests/SyntaxNodeComparerTests.cs +++ b/Src/CSharpier.Tests/SyntaxNodeComparerTests.cs @@ -874,6 +874,30 @@ public enum Enum result.Should().BeEmpty(); } + [Test] + public void CollectionExpression_Works_With_Adding_Comma_Before_Comment() + { + var left = """ + var someObject = new SomeObject() + { + Property1 = 1, + Property2 = 2 // Trailing Comment + }; + """; + + var right = """ + var someObject = new SomeObject() + { + Property1 = 1, + Property2 = 2, // Trailing Comment + }; + """; + + var result = CompareSource(left, right, movedTrailingTrivia: true); + + result.Should().BeEmpty(); + } + private static void ResultShouldBe(string actual, string expected) { actual.ReplaceLineEndings().Should().Be(expected.ReplaceLineEndings()); @@ -882,7 +906,8 @@ private static void ResultShouldBe(string actual, string expected) private static string CompareSource( string left, string right, - bool reorderedUsingsWithDisabledText = false + bool reorderedUsingsWithDisabledText = false, + bool movedTrailingTrivia = false ) { var result = new SyntaxNodeComparer( @@ -890,6 +915,7 @@ private static string CompareSource( right, false, reorderedUsingsWithDisabledText, + movedTrailingTrivia, SourceCodeKind.Regular, CancellationToken.None ).CompareSource(); diff --git a/Src/CSharpier/CSharpFormatter.cs b/Src/CSharpier/CSharpFormatter.cs index 46ad87820..b2c5e0d0e 100644 --- a/Src/CSharpier/CSharpFormatter.cs +++ b/Src/CSharpier/CSharpFormatter.cs @@ -130,6 +130,7 @@ bool TryGetCompilationFailure(out CodeFormatterResult compilationResult) var formattedCode = DocPrinter.DocPrinter.Print(document, printerOptions, lineEnding); var reorderedModifiers = formattingContext.ReorderedModifiers; var reorderedUsingsWithDisabledText = formattingContext.ReorderedUsingsWithDisabledText; + var movedTrailingTrivia = formattingContext.MovedTrailingTrivia; foreach (var symbolSet in PreprocessorSymbols.GetSets(syntaxTree)) { @@ -155,6 +156,7 @@ await syntaxTree.GetRootAsync(cancellationToken), reorderedUsingsWithDisabledText = reorderedUsingsWithDisabledText || formattingContext2.ReorderedUsingsWithDisabledText; + movedTrailingTrivia = movedTrailingTrivia || formattingContext2.MovedTrailingTrivia; } return new CodeFormatterResult @@ -166,6 +168,7 @@ await syntaxTree.GetRootAsync(cancellationToken), AST = printerOptions.IncludeAST ? PrintAST(rootNode) : string.Empty, ReorderedModifiers = reorderedModifiers, ReorderedUsingsWithDisabledText = reorderedUsingsWithDisabledText, + MovedTrailingTrivia = movedTrailingTrivia, }; } catch (InTooDeepException) diff --git a/Src/CSharpier/CodeFormatterResult.cs b/Src/CSharpier/CodeFormatterResult.cs index 9baea0e02..253cf151c 100644 --- a/Src/CSharpier/CodeFormatterResult.cs +++ b/Src/CSharpier/CodeFormatterResult.cs @@ -16,4 +16,5 @@ internal CodeFormatterResult() { } internal bool ReorderedModifiers { get; init; } internal bool ReorderedUsingsWithDisabledText { get; init; } + internal bool MovedTrailingTrivia { get; init; } } diff --git a/Src/CSharpier/SyntaxNodeComparer.cs b/Src/CSharpier/SyntaxNodeComparer.cs index cd135280f..55e472dee 100644 --- a/Src/CSharpier/SyntaxNodeComparer.cs +++ b/Src/CSharpier/SyntaxNodeComparer.cs @@ -10,6 +10,7 @@ internal partial class SyntaxNodeComparer protected SyntaxTree NewSyntaxTree { get; } protected bool ReorderedModifiers { get; } protected bool ReorderedUsingsWithDisabledText { get; } + protected bool MovedTrailingTrivia { get; } private static readonly CompareResult Equal = new(); @@ -18,6 +19,7 @@ public SyntaxNodeComparer( string newSourceCode, bool reorderedModifiers, bool reorderedUsingsWithDisabledText, + bool movedTrailingTrivia, SourceCodeKind sourceCodeKind, CancellationToken cancellationToken ) @@ -26,6 +28,7 @@ CancellationToken cancellationToken this.NewSourceCode = newSourceCode; this.ReorderedModifiers = reorderedModifiers; this.ReorderedUsingsWithDisabledText = reorderedUsingsWithDisabledText; + this.MovedTrailingTrivia = movedTrailingTrivia; var cSharpParseOptions = new CSharpParseOptions( CSharpFormatter.LanguageVersion, @@ -129,7 +132,8 @@ SyntaxNode formattedStart this.formattedStack.Push((formattedStart, formattedStart)); while (this.originalStack.Count > 0) { - var result = this.Compare(this.originalStack.Pop(), this.formattedStack.Pop()); + var valueTuple = this.originalStack.Pop(); + var result = this.Compare(valueTuple, this.formattedStack.Pop()); if (result.IsInvalid) { return result; @@ -274,7 +278,9 @@ originalToken.Parent is InterpolatedStringExpressionSyntax return result; } - var result2 = this.Compare(originalToken.TrailingTrivia, formattedToken.TrailingTrivia); + var result2 = this.MovedTrailingTrivia + ? Equal + : this.Compare(originalToken.TrailingTrivia, formattedToken.TrailingTrivia); return result2.IsInvalid ? result2 : Equal; } diff --git a/Src/CSharpier/SyntaxPrinter/FormattingContext.cs b/Src/CSharpier/SyntaxPrinter/FormattingContext.cs index d33e304b0..a6e6a56a8 100644 --- a/Src/CSharpier/SyntaxPrinter/FormattingContext.cs +++ b/Src/CSharpier/SyntaxPrinter/FormattingContext.cs @@ -22,6 +22,10 @@ internal class FormattingContext // we also need to keep track if we move around usings with disabledText public bool ReorderedUsingsWithDisabledText { get; set; } + // when adding a trailing comma in front of a trailing comment it is very hard to determine how to compare + // that trailing comment, so just ignore all trailing trivia + public bool MovedTrailingTrivia { get; set; } + public TrailingCommaContext? TrailingComma { get; set; } public FormattingContext WithSkipNextLeadingTrivia() diff --git a/Src/CSharpier/SyntaxPrinter/Token.cs b/Src/CSharpier/SyntaxPrinter/Token.cs index c8fa0c54f..38f29245d 100644 --- a/Src/CSharpier/SyntaxPrinter/Token.cs +++ b/Src/CSharpier/SyntaxPrinter/Token.cs @@ -132,6 +132,7 @@ context.TrailingComma is not null ) { docs.Add(context.TrailingComma.PrintedTrailingComma); + context.MovedTrailingTrivia = true; context.TrailingComma = null; } From c9374b55104168ff52ad86f11bebc071924e35ce Mon Sep 17 00:00:00 2001 From: Bela VanderVoort Date: Fri, 22 Nov 2024 10:38:25 -0600 Subject: [PATCH 2/2] self code review --- Src/CSharpier/SyntaxNodeComparer.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Src/CSharpier/SyntaxNodeComparer.cs b/Src/CSharpier/SyntaxNodeComparer.cs index 55e472dee..b4c143d1a 100644 --- a/Src/CSharpier/SyntaxNodeComparer.cs +++ b/Src/CSharpier/SyntaxNodeComparer.cs @@ -132,8 +132,7 @@ SyntaxNode formattedStart this.formattedStack.Push((formattedStart, formattedStart)); while (this.originalStack.Count > 0) { - var valueTuple = this.originalStack.Pop(); - var result = this.Compare(valueTuple, this.formattedStack.Pop()); + var result = this.Compare(this.originalStack.Pop(), this.formattedStack.Pop()); if (result.IsInvalid) { return result;