Better handle surrounding directives when inlining a local variable. by CyrusNajmabadi · Pull Request #21472 · dotnet/roslyn

CyrusNajmabadi · 2017-08-12T20:09:15Z

Customer scenario

The Inline Temporary Variable code fix is applied to code containing preprocessor directives. The operation fails to consider the syntax requirements of the preprocessor directives, and the resulting file is no longer valid code.

Bugs this fixes

Fixes #21455

Workarounds, if any

None.

Risk

Low. Existing test behavior was not altered except where the previous code was obviously incorrect, and new tests were implemented to cover the new behavior.

Performance impact

Low. There are no key algorithm changes and the approach is consistent with other cases needing similar patterns.

Is this a regression from a previous update?

No.

Root cause analysis

Tests were configured to ignore trivia by default (including preprocessor directives), so the validation described in previous unit tests did not match the actual validation occurring. The test suite was corrected in #21439, leaving this bug as a follow-up work item.

How was the bug found?

Unit tests.

Test documentation updated?

No.

CyrusNajmabadi · 2017-08-12T20:41:46Z

Tagging @dotnet/roslyn-ide @dotnet/roslyn-compiler @mattwar

As part of this change, i've tweaked out SyntaxNode.RemoveNode works when removing a node from a separated list. Specifically, i've tweaked the heuristic that is used to decide which comma to remove from teh seprated list when removing the node. Previously, we would always remove the preceding comma if there was one. If there wasn't one, we'd remove hte following comma. This was suboptimal for some coding patterns. For example, if the user has:

SomeMethod(a, // comment about a
           b, // some comment about b
           c // come comment about c
);

Then previously removing 'b' would leave you with:

SomeMethod(a, // some comment about b
           c // come comment about c
);

Now it gives you the expected

SomeMethod(a, // comment about a
           c // come comment about c
);

sharwell · 2017-08-12T20:49:47Z

@CyrusNajmabadi I don't see any difference between your before/after examples in the previous comment.

sharwell · 2017-08-12T20:51:16Z

src/EditorFeatures/CSharpTest/CodeActions/InlineTemporary/InlineTemporaryTests.cs

 {
        int y,
+#if true
+


💡 This blank line should be removed

I can live with this behavior. Critical part is not leaving dangling preprocessor directives.

CyrusNajmabadi · 2017-08-12T21:40:36Z

@sharwell

I don't see any difference between your before/after examples in the previous comment.

I've highlighted the relevant part with asterisk here:

SomeMethod(a, // comment about **b**
           c // come comment about c
);

Now it gives you the expected

SomeMethod(a, // comment about **a**
           c // come comment about c
);

sharwell · 2017-08-12T21:50:22Z

@CyrusNajmabadi Can you verify that the comment preservation works with leading comments too:

SomeMethod(/*arg1:*/ a,
    /*arg2:*/ b,
    /*arg3:*/ c);

I'd rather leave the extraneous comment on a blank line than have cases where a comment is incorrectly removed. In other words, the following outcome in your original example would be either acceptable or preferred:

SomeMethod(a, // comment about a
           // some comment about b
           c // come comment about c
);

sharwell · 2017-08-12T21:52:01Z

Also test the following:

SomeMethod(// comment about a
           a,
           // some comment about b
           b,
           // some comment about c
           c);

sharwell · 2017-08-12T21:54:51Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeRemover.cs

                return list;
            }

+            private bool ContainsEndOfLine(SyntaxTriviaList triviaList)


💡 Several other kinds of trivia have a newline built-in. StyleCop Analyzers uses a HasBuiltinEndLine helper method for this function.

I considered the cases here, but felt they were not suitably interesting or important enough to handle right now.

CyrusNajmabadi · 2017-08-14T17:18:58Z

Sure, will add additional tests.

CyrusNajmabadi · 2017-08-14T17:46:33Z

Added tests.

CyrusNajmabadi · 2017-08-17T19:41:05Z

@dotnet/roslyn-compiler @mattwar Please review the changes (and tests) to 'RemoveNode'. Thanks!

cston · 2017-08-17T19:59:23Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeRemover.cs

                return list;
            }

+            private bool ContainsEndOfLine(SyntaxTriviaList triviaList)


Method can be static or perhaps an instance method on SyntaxTriviaList. Or alternatively, replace with triviaList.IndexOf(SyntaxKind.EndOfLineTrivia) >= 0.

cston · 2017-08-17T20:01:40Z

src/Compilers/VisualBasic/Portable/Syntax/SyntaxNodeRemover.vb

@@ -199,6 +227,16 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax
                Return list
            End Function


Can VisitList be shared between C# and VB?

Yup. I think i can move to a shared helper.

Ok. I looked into this, and we definitely can't share the entire method. First, it accesses a lot of instance state. And we can't make the REmover classes shared either (because htey need to respectively subclass either CSharpSyntaxRewriter and VisualBasicSyntaxRewriter).

However, i can try to share some smaller pieces of logic.

CyrusNajmabadi · 2017-08-18T19:04:27Z

@cston Another look plz?

cston · 2017-08-18T19:21:52Z

src/Compilers/Core/Portable/Syntax/CommonSyntaxNodeRemover.cs

+            // If there is no next comma, or the next comma is not on the 
+            // same line, then just remove the preceding comma if there is 
+            // one.  If there is no next or previous comma there's nothing
+            // in the list that needs to be fixed up.


The comment and the name of this class are a little confusing because the methods are only classifying separators not removing any nodes.

CyrusNajmabadi · 2017-08-18T19:23:19Z

@dotnet/roslyn-compiler @mattwar i need a second pair of eyes when you get a chance. thanks!

CyrusNajmabadi · 2017-08-24T18:40:11Z

Need another @dotnet/roslyn-compiler review. Thanks!

CyrusNajmabadi · 2017-09-22T22:19:23Z

@dotnet/roslyn-compiler @mattwar i need a second pair of eyes when you get a chance. thanks!

CyrusNajmabadi · 2017-10-04T18:25:35Z

@dotnet/roslyn-compiler @mattwar i need a second pair of eyes when you get a chance. thanks!

mattwar · 2017-10-04T19:33:06Z

What about:

SomeMethod( /* A */ a, /* B */ b, /* C */ c);

Removing b and trailing comma gives me:

SomeMethod( /* A */ a, /* B */ c);

Because the comments are trailing the tokens.

Though this pattern is probably less likely to be used than the end-of-line comments.

CyrusNajmabadi · 2017-10-06T22:45:24Z

@mattwar That case works properly (and has tests). Specifically, in your example of SomeMethod( /* A */ a, /* B */ b, /* C */ c); the comma following 'b' is not considered to the be the one "belonging" to 'b'. We only consider the comma to belong to it if it is followed by a newline.

So, in this case, we'll do the right thing and will make SomeMethod( /* A */ a, /* C */ c);

CyrusNajmabadi · 2017-10-25T19:56:19Z

@dotnet/roslyn-compiler Are you ok with just Chuck and Matt's reviews?

CyrusNajmabadi · 2017-11-01T22:17:48Z

@agocke Can you take a look? Thanks!

CyrusNajmabadi · 2018-01-16T20:57:56Z

@sharwell can you, or someone on @dotnet/roslyn-ide help get this in? Thanks!

jinujoseph · 2018-01-23T02:06:34Z

@jcouv can you get a second review for this from compiler side

jcouv · 2018-01-23T05:45:07Z

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeRemover.cs

-                            if (alternate.Count > 0 && alternate[alternate.Count - 1].IsToken)
+                            CommonSyntaxNodeRemover.GetSeparatorInfo(
+                                withSeps, i, (int)SyntaxKind.EndOfLineTrivia,
+                                out var nextTokenIsSeparator, out var nextSeparatorBelongsToNode);


Please spell out bool types

jcouv

LGTM Thanks!

jcouv · 2018-01-23T05:53:06Z

src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxVisitor.cs


 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Syntax;


Can be removed?

sharwell · 2018-01-23T19:44:59Z

@jcouv Updated for your two changes

jcouv · 2018-01-23T20:53:50Z

@sharwell FYI The PR is all green, if you're ready for ask-mode approval (probably needs template).

sharwell · 2018-01-23T21:04:38Z

@Pilchie for ask mode

Pilchie · 2018-01-23T21:28:22Z

To comment - this change missed the cutoff for 15.6 Escrow. Given the narrowness of the scenario, we're going to just take it for 15.7 instead.

Pilchie · 2018-01-31T16:29:53Z

Approved - sorry for the delay

dnfclas added the cla-already-signed label Aug 12, 2017

CyrusNajmabadi force-pushed the inlineTempDirectives branch from 2b4ec39 to 2e8b0b6 Compare August 12, 2017 20:12

CyrusNajmabadi added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 12, 2017

CyrusNajmabadi force-pushed the inlineTempDirectives branch 2 times, most recently from 431fa4c to 52c908f Compare August 12, 2017 20:39

CyrusNajmabadi removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 12, 2017

CyrusNajmabadi force-pushed the inlineTempDirectives branch from 52c908f to 5a6071b Compare August 12, 2017 20:42

Better handle surrounding directives when inlining a local variable.

7188a08

CyrusNajmabadi force-pushed the inlineTempDirectives branch from 5a6071b to 7188a08 Compare August 12, 2017 20:43

sharwell reviewed Aug 12, 2017

View reviewed changes

Add tests.

fe633dc

cston reviewed Aug 17, 2017

View reviewed changes

CyrusNajmabadi added 2 commits August 17, 2017 13:30

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

5a9100a

Share code between VB and C#.

fff9abe

cston reviewed Aug 18, 2017

View reviewed changes

cston approved these changes Aug 18, 2017

View reviewed changes

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

e8d5cad

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

db53b50

mattwar approved these changes Oct 4, 2017

View reviewed changes

jcouv reviewed Jan 23, 2018

View reviewed changes

jcouv approved these changes Jan 23, 2018

View reviewed changes

jcouv reviewed Jan 23, 2018

View reviewed changes

sharwell changed the base branch from master to dev15.6.x January 23, 2018 19:37

Cleanup from code review

287472e

CyrusNajmabadi requested review from a team as code owners January 23, 2018 19:44

sharwell approved these changes Jan 23, 2018

View reviewed changes

sharwell added this to the 15.6 milestone Jan 23, 2018

sharwell changed the base branch from dev15.6.x to dev15.7.x January 23, 2018 21:19

sharwell modified the milestones: 15.6, 15.7 Jan 23, 2018

Pilchie added the Approved to merge label Jan 31, 2018

sharwell merged commit 63eb3cb into dotnet:dev15.7.x Jan 31, 2018

CyrusNajmabadi deleted the inlineTempDirectives branch January 25, 2020 00:45

		@@ -199,6 +227,16 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax
		Return list
		End Function

Conversation

CyrusNajmabadi commented Aug 12, 2017 • edited by sharwell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

CyrusNajmabadi commented Aug 12, 2017

Uh oh!

sharwell commented Aug 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 12, 2017

Uh oh!

sharwell commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 14, 2017

Uh oh!

CyrusNajmabadi commented Aug 14, 2017

Uh oh!

CyrusNajmabadi commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 18, 2017

Uh oh!

CyrusNajmabadi commented Aug 24, 2017

Uh oh!

CyrusNajmabadi commented Sep 22, 2017

Uh oh!

CyrusNajmabadi commented Oct 4, 2017

Uh oh!

mattwar commented Oct 4, 2017

Uh oh!

CyrusNajmabadi commented Oct 6, 2017

Uh oh!

CyrusNajmabadi commented Oct 25, 2017

Uh oh!

CyrusNajmabadi commented Nov 1, 2017

Uh oh!

CyrusNajmabadi commented Jan 16, 2018

Uh oh!

jinujoseph commented Jan 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 12, 2017 •

edited by sharwell

Loading

sharwell commented Aug 12, 2017 •

edited

Loading

sharwell commented Aug 12, 2017 •

edited

Loading