Provide a better experience editing a file with merge-markers in it. by CyrusNajmabadi · Pull Request #15850 · dotnet/roslyn

CyrusNajmabadi · 2016-12-12T21:59:33Z

This is a port of an extremely popular feature we did in TypeScript.

Today, if you try to edit a file with git merge-markers in it, you get a pretty terrible experience. Because of the text like <<<<<< in the file, the parser gets very thrown off, and that impacts all subsequent features.

To alleviate that, we augmented the TypeScript lexer to understand and be resilient to these merge markers, effectively making them part of our syntax model. Here's what things look like with the same feature applied to C#:

Note the clear errors indicating that we detected merge-conflicts.

In this system the higher levels then treat the merge-conflicts effectively as comments. So we classify them as comments, and we better support the code between them. In fact, in the 'top' part of the merge, you get a full C# experience (i.e. completion, highlight references, formatting, etc.). In the bottom part, we lexically-classify, but that's about it. In effect, it's treated similarly to an 'excluded code' region, except that we lexically classify since we're much more certain that is contains code.

--

A future feature i would lke to add would be a lightbulb/code-action that then easily let's you say "pick mine/theirs" to resolve merge conflicts :)

CyrusNajmabadi · 2016-12-12T22:01:24Z

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

This is a new feature, and i don't think meets the bar for VS2017. However, this is a super nice thing to add and it makes the experience around editing merge-conflict files so much nicer than today. I would like to get this into post-dev15.

Note: tests are forthcoming. I just wanted to get the initial code in so people could take a look. This code is really a straight port of the exact same feature we did in TypeScript several years back.

CyrusNajmabadi · 2016-12-13T00:05:46Z

I also added this feature to make working with merge conflicts nicer: #15853

CyrusNajmabadi · 2016-12-13T21:29:25Z

I have done the VB side of this as well. So i would appreciate VB and C# eyes on this.

jcouv · 2017-02-14T18:11:49Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

 Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax.Type.get -> Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax
-Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax.WithIdentifier(Microsoft.CodeAnalysis.SyntaxToken identifier) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax
 Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax.Update(Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax type, Microsoft.CodeAnalysis.SyntaxToken identifier) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax
+Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax.WithIdentifier(Microsoft.CodeAnalysis.SyntaxToken identifier) -> Microsoft.CodeAnalysis.CSharp.Syntax.TupleElementSyntax


FYI @jaredpar @gafter for public API change.

jcouv · 2017-02-14T18:13:43Z

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


+        // All conflict markers consist of the same character repeated seven times.  If it is
+        // a <<<<<<< or >>>>>>> marker then it is also followed by a space.
+        private static readonly int s_conflictMarkerLength = "<<<<<<<".Length;


... = 7; seems simpler, especially since you have a comment already ;-)

I far prefer the "string".Length pattern. The code is then self-describing and good, regardless of the state of the comment.

jcouv · 2017-02-14T18:15:16Z

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

+            var text = TextWindow.Text;
+            if (position == 0 || SyntaxFacts.IsNewLine(text[position - 1]))
+            {
+                var firstCh = text[position];


firstCh [](start = 20, length = 7)

Maybe check or assert that firstCh is one of '<', '>' or '='?

i can do that.

jcouv · 2017-02-14T18:17:17Z

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

+            var startCh = this.TextWindow.PeekChar();
+
+            // First create a trivia from the start of this merge conflict marker to the
+            // end of line/file (whicever comes first).


whicever [](start = 33, length = 8)

s/whicever/whichever/

CyrusNajmabadi · 2017-02-17T00:36:02Z

Anything else @jcouv

jaredpar · 2017-02-17T00:41:26Z

We need to chat before merging this. It's essentially adding a feature to the compiler layer. That means it's putting us on the hook for future bug fixes, etc ...

CyrusNajmabadi · 2017-02-17T00:47:03Z

Sure. We can talk about this. I can def see how this is a feature (since there are API changes for this). However, the core idea here is that this is just better error recovery. Right now our error recovery is basically horrendous/non-existent. Leading to a fairly terrible user experience in a reasonably common editing case. As this is really just about error-recovery, we always reserve the right to change how we behave here (including removing all this code entirely if we want). We've always asserted that error files may change in terms of how our APIs present them (including at the syntactic level).

Also, from doing this in TS, the long term cost is effectively nothing. AFAIK there hasn't ever even been a single bug that needed to get fixed here. So, while it is a feature, it's an extremely low cost one. I'm hoping the bang/buck equation makes this an easy choice.

--

Also, from a selfish Roslyn perspective, i think this is quite helpful to the team. Working with unmerged files today is basically extremely unpleasant in the IDE. You really have no choice but to treat the file as a blob of text that you manipulate roughly, while giving up most features. This tiny feature suddenly makes it completely reasonable and effective to work with these files jsut as you would any other C#/VB file. This turned out to be quite nice for TS development when this was added.

gafter · 2017-02-17T01:03:54Z

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

+                    break;
+                }
+
+                if (ch == '>' && IsConflictMarkerTrivia())


Do you want to accept conflict markers that are anywhere but the beginning of a line? #Resolved

Ah, I see that is tested by IsConflictMarkerTrivia. #Resolved

IsConflictMarkerTrivia actually verifies that it's at the start of a line. Which is pretty much all that we want since htat's what you always get from Git.

CyrusNajmabadi · 2017-02-17T01:05:25Z

Also worth noting: this is effectively a straight port of the TS code I wrote for this.

CyrusNajmabadi · 2017-02-17T01:13:33Z

src/Compilers/VisualBasic/Portable/Errors/Errors.vb

        ERR_PublicSignNetModule = 37282
        ERR_BadAssemblyName = 37283

+        ERR_Merge_conflict_marker_encountered = 37282


clearly this needs to be updated.

jaredpar

We've discussed with @Pilchie and @CyrusNajmabadi. This is a feature the IDE will own within the compiler.

CyrusNajmabadi · 2017-02-23T01:42:54Z

@jaredpar Any other concerns you have? Or is this ready for review? Tnx!

jaredpar · 2017-02-23T01:43:39Z

@CyrusNajmabadi i'm always concerned 😄

Fine for review.

CyrusNajmabadi · 2017-02-23T01:44:50Z

Great :) Glad to work toward making you bald like me.

@dotnet/roslyn-compiler Please review when convenient (though ideally sooner rather than later) :)

CyrusNajmabadi · 2017-02-23T01:46:17Z

@dotnet/roslyn-ide Please review IDE sections as well. Thanks!

AdamSpeight2008 · 2017-02-23T11:09:26Z

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


+        // All conflict markers consist of the same character repeated seven times.  If it is
+        // a <<<<<<< or >>>>>>> marker then it is also followed by a space.
+        private static readonly int s_conflictMarkerLength = "<<<<<<<".Length;


Minor: I'd personally make the "<<<<<<<" a named constant. eg s_conflictMarker`

I'm not certain what that buys. It's a value only used in a single place. Why introduce an indirection when it is already self-describing?

Thought it was used else where, if it was it would make sense to share the content. So it had the same content.

jcouv

LGTM Thanks

CyrusNajmabadi · 2017-02-28T20:13:33Z

@gafter can you take a look?

@dotnet/roslyn-ide please review IDE parts.

gafter · 2017-02-28T20:52:51Z

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

                            return;
                        }
+
+                    case '>':


case [](start = 20, length = 4)

A valid program can have >>>>>>> at the beginning of a line. e.g.

List<List<List<List<List<List<List<int >>>>>>> x = null;

Are we intentionally breaking this "scenario"?

Yes. We would be breaking that scenario. Up to you guys if you think such a break could realistically happen. Note: the issue occurs for TS, and we never got a single report about that.

How about this idea: we stop recognizing >>>>>>> as a conflict marker, but continue having >>>>>>> end a skipped text section that is started with a =======. I think that would remove the compat break and still retain all the value of the feature. Does that work?

In reply to: 103549556 [](ancestors = 103549556)

To clarify, the idea is that the skipped text section would be recognized by the scanner.

In reply to: 103551073 [](ancestors = 103551073,103549556)

Ok. I think that is doable. Trying this out now.

CyrusNajmabadi · 2017-02-28T21:06:46Z

@gafter Checking to see how feasible that would be.

CyrusNajmabadi · 2017-02-28T21:30:22Z

@gafter I have made that change (and added a test for the large-generic parsing case as well).

AdamSpeight2008 · 2017-02-28T21:36:08Z

@CyrusNajmabadi What happens it the "markers" are not valid markers but actually text.

"
=======

>>>>>>> 
"

CyrusNajmabadi · 2017-02-28T21:49:45Z

@AdamSpeight2008 Nothing happens. This is about lexing trivia. The contents of a string are not trivia and are consumed by the normal string lexing routine.

gafter · 2017-03-01T01:32:29Z

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/LexicalTests.cs

+    void M()
+    {
+        var v = new List<List<List<List<List<List<List<int
+>>>>>>>


Please add a space after the last >.

AdamSpeight2008 · 2017-03-01T19:05:31Z

@CyrusNajmabadi Thank-You for clarifying, I'm still learning.

gafter

This is great, thanks!

CyrusNajmabadi · 2017-03-01T20:06:06Z

@AdamSpeight2008 We all are :) Glad to help.

CyrusNajmabadi · 2017-03-01T20:06:18Z

@gafter Will make that change and merge in.

CyrusNajmabadi · 2017-03-01T20:32:00Z

@dotnet/roslyn-ide Still need reviewers for the IDE sections.

dpoeschl

IDE portions look good.

dpoeschl · 2017-03-01T22:12:47Z

src/EditorFeatures/CSharpTest/Classification/SyntacticClassifierTests.cs

        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.Classification)]
+        public async Task TestConflictMarkers1()


Do we want a test here for the really, really generic case?

Sure. I can add that. It's more a test that nothing goes horribly wrong (like crashing/index-out-of-bounds) vs actually verifying that we classify correctly.

dpoeschl · 2017-03-01T22:18:13Z

src/Workspaces/VisualBasic/Portable/Classification/Worker.vb

+
+        Private Sub ClassifyDisabledText(trivia As SyntaxTrivia, triviaList As SyntaxTriviaList)
+            Dim index = triviaList.IndexOf(trivia)
+            If index >= 2 AndAlso


Is there a way to share this code?

Possibly now. It used to be not possible due to the different syntax models. Now that we've unified, tihs area is ripe for an overhaul. I'll add to my cleanup backlog.

… into mergeMarkers

cston · 2017-03-17T17:13:45Z

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

        ERR_ExpressionTreeContainsDiscard = 8207,
        #endregion more stragglers for C# 7
+
+        ERR_Merge_conflict_marker_encountered = 8300,


Please use mixed case: ERR_MergeConflictMarkerEncountered

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

gafter added Area-Compilers Feature Request labels Dec 12, 2016

gafter added this to the 2.1 milestone Dec 12, 2016

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

dafedbd

CyrusNajmabadi force-pushed the mergeMarkers branch from f775766 to dafedbd Compare December 13, 2016 21:47

CyrusNajmabadi mentioned this pull request Dec 13, 2016

Add code fix to help choose which piece to take when there is a merge conflict #15853

Merged

gafter self-assigned this Dec 14, 2016

Use a fresh error code.

c631783

jcouv self-assigned this Dec 29, 2016

jaredpar assigned AlekseyTs Feb 10, 2017

jcouv reviewed Feb 14, 2017

View reviewed changes

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

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

79138da

gafter reviewed Feb 17, 2017

View reviewed changes

CyrusNajmabadi commented Feb 17, 2017

View reviewed changes

Update error code.

8cee378

jaredpar reviewed Feb 23, 2017

View reviewed changes

AdamSpeight2008 reviewed Feb 23, 2017

View reviewed changes

jcouv approved these changes Feb 28, 2017

View reviewed changes

gafter reviewed Feb 28, 2017

View reviewed changes

CyrusNajmabadi added 2 commits February 28, 2017 13:07

Merge branch 'master' into mergeMarkers

ef8b1e2

Only accept >>>>>>> as a merge marker if it follows a ======= region.

2141286

gafter reviewed Mar 1, 2017

View reviewed changes

gafter approved these changes Mar 1, 2017

View reviewed changes

Update LexicalTests.cs

a4344b4

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

2e42b00

dpoeschl approved these changes Mar 1, 2017

View reviewed changes

CyrusNajmabadi added 2 commits March 1, 2017 14:21

Make code more consistent.

e55f763

Merge branch 'mergeMarkers' of https://github.com/CyrusNajmabadi/roslyn…

41250fe

… into mergeMarkers

CyrusNajmabadi merged commit 1b0cf5c into dotnet:master Mar 1, 2017

CyrusNajmabadi deleted the mergeMarkers branch March 1, 2017 22:59

cston reviewed Mar 17, 2017

View reviewed changes

jcouv mentioned this pull request Jul 7, 2022

Handle diff3 conflict markers #62391

Merged

Conversation

CyrusNajmabadi commented Dec 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Dec 12, 2016

Uh oh!

CyrusNajmabadi commented Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CyrusNajmabadi commented Feb 17, 2017

Uh oh!

jaredpar commented Feb 17, 2017

Uh oh!

CyrusNajmabadi commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 23, 2017

Uh oh!

jaredpar commented Feb 23, 2017

Uh oh!

CyrusNajmabadi commented Feb 23, 2017

Uh oh!

CyrusNajmabadi commented Feb 23, 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!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Feb 28, 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 Feb 28, 2017

Uh oh!

CyrusNajmabadi commented Dec 12, 2016 •

edited

Loading

CyrusNajmabadi commented Dec 13, 2016 •

edited

Loading

CyrusNajmabadi commented Feb 17, 2017 •

edited

Loading

gafter Feb 17, 2017 •

edited

Loading

gafter Feb 17, 2017 •

edited

Loading