Initial fix for issue "VB Formatting of LineContinuation Wrong after _ ' Comment" by paul1956 · Pull Request #54559 · dotnet/roslyn

paul1956 · 2021-07-02T03:52:09Z

Issue #36891 There are some VB constants in C# Abstract function that needs to be addressed and C# code needs to be cleaned up and not look like it was written by VB programmer. All "Line Continuation" Tests pass. I await all tests to run.

… function that needs to be addressed and C# code needs to be cleaned up and not look like it was written by VB programmer.

…a is EOL.

sharwell · 2021-07-02T04:29:06Z

src/Workspaces/CoreTest/CodeCleanup/NormalizeModifiersOrOperatorsTests.cs

    Private _
        Shared _
- _
+               _


😕 It seems like this one should have been removed altogether

Suggested change

_

@sharwell They were not in the original design even before I added "comments" after Line Continuation all I did was move the underscore to a more appropriate place. Many of the tests expect the same results with and without the comments. You can see the original behavior on line 724 before my change.

<Theory, Trait(Traits.Feature, Traits.Features.Formatting)> <InlineData("_")> <InlineData("_ ' Comment")>

Makes sense. We should file a follow-up issue to prevent the extraneous continuation as a separate change.

@sharwell Agreed, I think it is rare someone would actually do it without a comment but I can enter an issue. It will require duplicating all the tests which today use InlineData.

@sharwell This might be better handled by IDE2000 and/or RS0101 and the code fixes for them as a separate PR. Today I can type lots of blank lines and nothing stops me but I do get the two warmings above.

src/EditorFeatures/VisualBasicTest/CodeActions/InlineTemporary/InlineTemporaryTests.vb

src/Workspaces/CoreTest/CodeCleanup/NormalizeModifiersOrOperatorsTests.cs

src/Workspaces/CoreTest/CodeCleanup/RemoveUnnecessaryLineContinuationTests.cs

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs

...kspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/LineColumnRule.cs

src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

…/InlineTemporaryTests.vb Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

…orsTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

…nuationTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

… and in C# return false for first and correct value for second but its not used.

...aredUtilitiesAndExtensions/Compiler/CSharp/Formatting/Engine/Trivia/CSharpTriviaFormatter.cs

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/TriviaDataFactory.Analyzer.vb

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs

CyrusNajmabadi · 2021-07-02T18:45:17Z

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs


            var list = new TriviaList(this.Token1.TrailingTrivia, this.Token2.LeadingTrivia);
+            var i = -1;
+            var previousLineColumn = new LineColumn();


can you doc what this is for?

💭 It feels like this local variable is hard-coding logic that is designed to be provided by implementations of GetLineColumnRuleBetween and/or GetAdjustSpacesOperation (which are called by FormatFirstTriviaAndWhitespaceAfter below). If GetLineColumnRuleBetween/GetAdjustSpacesOperation return the correct value, then FormatFirstTriviaAndWhitespaceAfter will also return the correct value and it looks like all of the changes in this file become unnecessary (some of the logic would move into the space adjustment rule, but would no longer be applied in such a general location).

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs

paul1956 · 2021-07-25T23:37:18Z

@jinujoseph Ping

sharwell · 2021-07-26T14:46:01Z

Design review conclusion: the formatting of continuation characters demonstrated by this PR is acceptable.

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs

...kspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/LineColumnRule.cs

sharwell · 2021-07-26T15:05:20Z

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/TriviaDataFactory.Analyzer.vb

+                    result.HasTabAfterSpace = False
+                    result.Space = 0
+                    result.Tab = 0
+                    result.TreatAsElastic = result.TreatAsElastic Or trivia.IsElastic()


❔ What happens if these three lines are left in their original location?

@sharwell 4 tests fail for example ColonToken_If with whitepace NOT removed

Expected: ··· If True Then\r\n End If\r\n End Sub\r\nEnd Class Actual: ··· If True Then \r\n End If\r\n End Sub\r\nEnd Class

sharwell · 2021-07-26T15:16:26Z

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs


            var list = new TriviaList(this.Token1.TrailingTrivia, this.Token2.LeadingTrivia);
+            var i = -1;
+            var previousLineColumn = new LineColumn();


💭 It feels like this local variable is hard-coding logic that is designed to be provided by implementations of GetLineColumnRuleBetween and/or GetAdjustSpacesOperation (which are called by FormatFirstTriviaAndWhitespaceAfter below). If GetLineColumnRuleBetween/GetAdjustSpacesOperation return the correct value, then FormatFirstTriviaAndWhitespaceAfter will also return the correct value and it looks like all of the changes in this file become unnecessary (some of the logic would move into the space adjustment rule, but would no longer be applied in such a general location).

paul1956 · 2021-07-26T17:26:00Z

@sharwell Regarding "local variable is hard-coding", I don't see how those 2 functions have to context to understand that " _ ' comment" has special meaning. The way they are written forces 2 extra spaces on every line. They only look at 2 trivia and I need to look at 4 or more.

paul1956 · 2021-08-13T20:18:38Z

ping

paul1956 · 2021-08-20T16:06:53Z

Ping @sharwell @CyrusNajmabadi

paul1956 · 2021-09-11T22:15:36Z

@sharwell Is there anything else I need to do to move this forward?

paul1956 · 2021-10-18T23:11:30Z

Ping
anything else needed to move forward?

paul1956 · 2021-11-08T16:27:30Z

Ping @sharwell @CyrusNajmabadi what is needed to finish this?

sharwell · 2021-11-08T16:40:52Z

@paul1956 it looks like a few unit tests are failing. The integration test failures have been resolved separately and should not affect future builds from this branch.

Edit: I pushed e628e3b to fix these issues.

paul1956 · 2021-11-08T17:39:48Z

@sharwell thanks

sharwell · 2021-11-08T20:58:43Z

🎉 I'm so happy this finally passed/merged

Thanks @paul1956 !

paul1956 · 2021-11-08T20:59:57Z

Thanks for all your help. Any idea when I might see it in VS?

sharwell · 2021-11-08T23:38:09Z

Visual Studio 2022 version 17.1 Preview 2

Initial fix for issue "VB Formatting of LineContinuation Wrong after _ ' Comment"

Initial fix for Issue 36891. This are some VB constants a C# Abstract…

2a0c94d

… function that needs to be addressed and C# code needs to be cleaned up and not look like it was written by VB programmer.

paul1956 requested a review from a team as a code owner July 2, 2021 03:52

ghost added the Area-IDE label Jul 2, 2021

Remove redundant code, and avoid exta tests and assignment when trivi…

e5b46a5

…a is EOL.

sharwell reviewed Jul 2, 2021

View reviewed changes

sharwell added the Need Design Review The end user experience design needs to be reviewed and approved. label Jul 2, 2021

paul1956 and others added 11 commits July 1, 2021 23:54

Update src/EditorFeatures/VisualBasicTest/CodeActions/InlineTemporary…

95ff685

…/InlineTemporaryTests.vb Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/CoreTest/CodeCleanup/NormalizeModifiersOrOperat…

249f7f2

…orsTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/CoreTest/CodeCleanup/RemoveUnnecessaryLineConti…

d03b703

…nuationTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/CoreTest/CodeCleanup/RemoveUnnecessaryLineConti…

3cf8a3c

…nuationTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/CoreTest/CodeCleanup/RemoveUnnecessaryLineConti…

4b8ead6

…nuationTests.cs Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

5a7c294

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/VisualBasicTest/Formatting/FormattingTests.vb

18ee3e7

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

Remove _PC from test names

dbdba56

add missing 4 to name

d5e4cdb

Add IsComment and IsLineContinuation to Interface and implement in VB…

1d2ac5f

… and in C# return false for first and correct value for second but its not used.

Allow C# IsComment to Thrown if that is a better design

a33ee62

Youssef1313 reviewed Jul 2, 2021

View reviewed changes

...aredUtilitiesAndExtensions/Compiler/CSharp/Formatting/Engine/Trivia/CSharpTriviaFormatter.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 2, 2021

View reviewed changes

...aredUtilitiesAndExtensions/Compiler/CSharp/Formatting/Engine/Trivia/CSharpTriviaFormatter.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 2, 2021

View reviewed changes

src/Workspaces/VisualBasic/Portable/Formatting/Engine/Trivia/TriviaDataFactory.Analyzer.vb Outdated Show resolved Hide resolved

sharwell reviewed Jul 2, 2021

View reviewed changes

...haredUtilitiesAndExtensions/Compiler/Core/Formatting/TriviaEngine/AbstractTriviaFormatter.cs Outdated Show resolved Hide resolved