Add code fix to help choose which piece to take when there is a merge conflict by CyrusNajmabadi · Pull Request #15853 · dotnet/roslyn

CyrusNajmabadi · 2016-12-13T00:04:22Z

Looks like this:

… conflict

dpoeschl · 2016-12-13T22:16:11Z

@CyrusNajmabadi The title of this PR is misleading. There's a non-trivial amount of compiler changes too.

CyrusNajmabadi · 2016-12-13T22:25:40Z

This is a branch off of #15850 . That PR contains the compiler changes, and i'm working through getting that reviewed.

This PR was around the CodeFixProvider. It would not go in without the other PR being reviewed and going in first.

dpoeschl · 2016-12-13T21:50:29Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  <data name="ERR_OpenEndedComment" xml:space="preserve">
    <value>End-of-file found, '*/' expected</value>
  </data>
+  <data name="ERR_Merge_conflict_marker_encountered" xml:space="preserve">


Can you push the resource down and reuse it instead of duplicating it between C# and VB?

No. The compilers don't work that way wrt resources.

dpoeschl · 2016-12-13T21:52:10Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

        ERR_PPDefFollowsToken = 1032,
        //ERR_TooManyLines = 1033,      unused in Roslyn.
-        //ERR_LineTooLong = 1034,       unused in Roslyn.
+        ERR_Merge_conflict_marker_encountered = 1034,


CS1034 already has meaning, so I wouldn't re-use it like this. https://msdn.microsoft.com/en-us/library/7bb77w7x(v=vs.90).aspx

dpoeschl · 2016-12-13T22:11:47Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

+            return triviaList;
+        }
+
+        private void LexConflictMarkerEndOfLine(ref SyntaxListBuilder triviaList)


Maybe name this LexConflictMarkerEndLines?
But I don't understand why more than one newline after <<<<<<< would be trailing trivia on it. Any subsequent newlines feel like they belong to the first token in that half of the diff (so that a codefix taking everything in the "mine" side wouldn't also need to worry about extra newlines attached to the <<<<<<< line -- they could just take everything in that half of the diff and know that it's correct.

Agree, and that is the general heuristic for newline trivia - it is generally attached to the next token, except the first newline which ends a line.

dpoeschl · 2016-12-13T22:14:45Z

src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb

+               ch <> "_"c AndAlso
+               ch <> "R"c AndAlso
+               ch <> "r"c AndAlso
+               ch <> "<"c AndAlso


The comment above is now incorrect.

dpoeschl · 2016-12-13T22:18:35Z

src/Compilers/VisualBasic/Test/Syntax/QuickTokenTableTests.vb

-        ShouldBeGoodQuickToken("ab$=")
-        ShouldBeGoodQuickToken("  a  =")
-        ShouldBeGoodQuickToken("  =a")
+        ShouldBeGoodQuickToken("a,")


I don't understand these changes.

dpoeschl · 2016-12-13T22:26:06Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+            while (true)
+            {
+                token = token.GetNextToken(includeZeroWidth: true);
+                if (token.RawKind == 0)


A comment explaining the magic number would be nice

dpoeschl · 2016-12-13T22:32:43Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+                }
+
+                var index = GetEqualsConflictMarkerIndex(text, token);
+                if (index >= 0)


Verifying my understanding of the compiler side: the <<<<<<< is only treated as a conflict marker if there exists a ======= and a >>>>>>> to match, right? Otherwise this index would always be -1 and you'd be stuck in a loop either forever or until GetNextToken fails, I'm not sure which.

and you'd be stuck in a loop either forever or until GetNextToken fails, I'm not sure which.

You're in the loop until there are no more tokens in the file, in which case the above 'if' check breaks out.

Yep. Thanks.

dpoeschl · 2016-12-13T22:34:17Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+                    if (index + 3 < token.LeadingTrivia.Count)
+                    {
+                        var equalsTrivia = leadingTrivia[index];
+                        var endOfLineTrivia = leadingTrivia[index + 1];


If this is actually all of the endlines, then pluralizing here would be better.

dpoeschl · 2016-12-13T22:39:11Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+            return -1;
+        }
+
+        private SyntaxTrivia GetEqualsConflictMarker(SyntaxToken token)


What's going on here? I don't see any callers.

dpoeschl · 2016-12-13T22:42:37Z

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

+
+        private void ClassifyConflictMarker(SyntaxTrivia trivia)
+        {
+            AddClassification(trivia, ClassificationTypeNames.Comment);


Should we introduce a new color here? I might want to make my conflict markers bright red. 😄

dpoeschl · 2016-12-13T22:46:52Z

In general, it would be nice to see more tests of "broken" scenarios. Almost conflict resolution code, but without the =======, or without the >>>>>>>, or whatever.

gafter · 2016-12-14T20:56:09Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

        ERR_PPDefFollowsToken = 1032,
        //ERR_TooManyLines = 1033,      unused in Roslyn.
-        //ERR_LineTooLong = 1034,       unused in Roslyn.
+        ERR_Merge_conflict_marker_encountered = 1034,


New error codes should please be moved near then end. They are currently approximately segregated by the version of the compiler in which they were introduced.

Ah, will do.

CyrusNajmabadi · 2016-12-14T23:55:48Z

retest windows_release_unit64_prtest please

DustinCampbell · 2016-12-21T08:56:55Z

This is very cool! Should we get the right folks to review the API additions, or wait on that until it's closer to being ready?

DanielRosenwasser · 2016-12-23T03:22:04Z

src/Features/Core/Portable/ConflictMarkerResolution/AbstractConflictMarkerCodeFixProvider.cs

+
+            var startTrivia = root.FindTrivia(context.Span.Start);
+
+            if (!IsConflictMarker(text, startTrivia, '<'))


So this only provides a fix on the <<<<<<<, but not the other markers?

Currently yes.

Pilchie · 2017-02-24T01:41:43Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

            }

-            return start < TextWindow.Position;
+            return start < TextWindow.Position; 


Nit: Extra space.

Pilchie · 2017-02-24T01:45:40Z

src/Compilers/CSharp/Portable/Parser/Lexer.cs

+            return triviaList;
+        }
+
+        private void LexConflictMarkerEndOfLine(ref SyntaxListBuilder triviaList)


Agree, and that is the general heuristic for newline trivia - it is generally attached to the next token, except the first newline which ends a line.

dnfclas added the cla-already-signed label Dec 13, 2016

CyrusNajmabadi changed the title ~~Add code fix to help choose which piece to take when there is a merge conflict." [mergeMarkerAnalyzer ae14e1d] Add code~~ Add code fix to help choose which piece to take when there is a merge conflict Dec 13, 2016

CyrusNajmabadi mentioned this pull request Dec 13, 2016

Provide a better experience editing a file with merge-markers in it. #15850

Merged

Add code fix to help choose which piece to take when there is a merge…

a34c2d4

… conflict

CyrusNajmabadi force-pushed the mergeMarkerAnalyzer branch from 1432ba2 to a34c2d4 Compare December 13, 2016 21:49

dpoeschl reviewed Dec 13, 2016

View reviewed changes

gafter reviewed Dec 14, 2016

View reviewed changes

CyrusNajmabadi added 2 commits December 14, 2016 13:25

Use a fresh error code.

2152ec3

Merge branch 'mergeMarkers' into mergeMarkerAnalyzer

f3e70da

Pilchie added the Area-IDE label Dec 15, 2016

DanielRosenwasser reviewed Dec 23, 2016

View reviewed changes

Pilchie added this to the 2.1 milestone Jan 9, 2017

jcouv added the Area-Compilers label Feb 14, 2017

agocke changed the base branch from post-dev15 to master February 14, 2017 23:28

Merge branch 'mergeMarkers' into mergeMarkerAnalyzer

4d427c3

Pilchie approved these changes Feb 24, 2017

View reviewed changes

CyrusNajmabadi added 4 commits March 1, 2017 14:26

Merge branch 'mergeMarkers' into mergeMarkerAnalyzer

4b3f002

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

608bb76

Update tests.

d460ea4

Fix code.

1c86412

CyrusNajmabadi merged commit ef7e35e into dotnet:master Mar 2, 2017

CyrusNajmabadi deleted the mergeMarkerAnalyzer branch March 2, 2017 03:06


		var startTrivia = root.FindTrivia(context.Span.Start);

		if (!IsConflictMarker(text, startTrivia, '<'))

Conversation

CyrusNajmabadi commented Dec 13, 2016

Uh oh!

dpoeschl commented Dec 13, 2016

Uh oh!

CyrusNajmabadi commented Dec 13, 2016

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!

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!

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!

dpoeschl commented Dec 13, 2016

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 Dec 14, 2016

Uh oh!

DustinCampbell commented Dec 21, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants