Roslyn: CS8300 Merge conflict marker does not see markers inside of comments or #if

[binki]

Version Used:
2.6.0.62405 (4939752b) (vs-15.6.0 preview 1.0)

Steps to Reproduce:

1. Place /* on a line prior to an opening merge marker (<<<<<<< dest) or get a merge conflict inside of a comment from your favorite VCS. You might have something that looks like this:

public class Class1
{
    public void M()
    {
        /*
>>>>>>> dest
         * a thing
        */
=======
         * another thing
         */
>>>>>>> source
    }
}

Expected Behavior:

CS8300 is shown for the opening merge marker and shows a Quick Action with the option of accepting either dest or source.

Actual Behavior:

Merge marker errors are shown for the ======= and >>>>>>> source but not for the <<<<<<< dest. No Quick Actions are shown on the opening merge marker or subsequent merge markers.

[jcouv]

Tagging @CyrusNajmabadi @gafter as FYI

[gafter]

The text >>>>>>> dest is a valid line in a comment, and we do not want to change it into an error. The proposed behavior would be a breaking change and a violation of the language specification.

The best I can suggest is that the IDE recognize this situation and offer the fix. That is an IDE change.

[binki]

I noticed that this is also an issue for preprocessor directives:

public class Class1
{
    public void M()
    {
#if notreallydefined
>>>>>>> dest
        System.Console.WriteLine("a");
=======
        System.Console.WriteLine("b");
>>>>>>> source
#endif
    }
}

I get no help from codefixes for that merge conflict scenario.

[CyrusNajmabadi]

i agree with @gafter .

This can be done entirely in teh IDE, driven by an analyzer that just looks at the text of the file.

[svick]

@gafter @CyrusNajmabadi

The compiler already handles merge conflict markers in some cases. Are you sure that handling some cases in the compiler and other cases in the IDE is the best solution? That sounds like a recipe for all kinds of issues to me.

One possible approach would be that if the compiler finds the middle or closing merge conflict marker that's causing an error, it will look for the opening marker in unparsed code (comments, inactive branches of #ifs etc.). This will not create new errors in code that previously compiled successfully, so it wouldn't be a breaking change. Looking at already parsed code would likely decrease performance, but I think that might be okay in this specific case.

Going even further, the compiler could also detect merge conflict markers that don't cause errors and report them as weaker diagnostics (Info or Warning, possibly as part of a warning wave).

[CyrusNajmabadi]

One possible approach would be that if the compiler finds the middle or closing merge conflict marker that's causing an error,

I don't see how that would help with:

```c#
public class Class1
{
public void M()
{

if notreallydefined

dest

System.Console.WriteLine("a");

    System.Console.WriteLine("b");

source

endif

}

}
```

[CyrusNajmabadi]

That sounds like a recipe for all kinds of issues to me.

What sort of issues do you think that would cause?

[CyrusNajmabadi]

Are you sure that handling some cases in the compiler and other cases in the IDE is the best solution?

I think it's completely fine. The purpose of having compiler support was to make sure the compiler wasn't thrown off hugely by the presence of merge markers. That's all. i.e. before my change, having something >>>>>>> would make the parser go all haywire, which would make the parse tree bad, which would lead to tons of cascading errors, etc. etc. etc. After my change, you have a reasonable tree to work with.

Now, on the IDE side, you can just fix up the analyzer to do better in these sorts of corner cases.

For example, if you have:

```c#
public class Class1
{
public void M()
{
/*

dest
* a thing

*/

     * another thing
     */

source
}
}
```

You will have errors on hte ==== and >>>>> lines:

If the fixer sees those types of lines without corresponding <<<<< lines in the code (or other sorts of variations) it can just textually scan for such a pattern at the start of lines in the document, seeing if any are in a non-user-code section.

[svick]

@CyrusNajmabadi I think you're right. My assumption pretty much was that the goal was to produce a correct diagnostic for all merge conflict markers and so I was against splitting the logic for that.

But the actual goal is to produce a correct refactoring for all merge conflict markers. And that logic belongs into the IDE and using the compiler diagnostic as one of the inputs for the refactoring is fine.

[binki]

If think merge conflicts entirely inside of comments/#ifs are something that people would like to see a warning about during compilation even when not using the IDE (e.g., when invoking csc/msbuild directly). If someone is writing comments which intentionally have merge markers inside of them for, e.g., demonstration purposes, they can always use pragmas to suppress the warnings or entirely disable the analyzer (rule?) which adds those warnings. Is this something that can be considered as part of this issue? Wouldn’t this require compiler-level analyzers rather than IDE-level ones (and what are these IDE analyzers anyway? ;-)).

[CyrusNajmabadi]

Is this something that can be considered as part of this issue?

I would not, no.

If you want that behavior feel free to create an analyzer for this and make it available on Nuget. :)

[binki]

I found another case where the codefix does not show up properly in Visual Studio:

class X {
  void x() {
    var x = @"
<<<<<<< working copy
a";
=======
b";
>>>>>>> merge rev
  }
}

Does this fall under the same thing, is it different bug (new issue), or should it be dismissed?

[CyrusNajmabadi]

It's the same thing. The first <<<<<<<< is completely legal. It would be up to the IDE to support this. Note: supporting this should be really simple. I think the IDE would welcome PRs here.