[WIP] Fix brace matching for outer braces

[cartermp]

Fixes#3794

Not entirely sure if this is the right fix, but it certainly is a fix. It's really just the range.Contains() call that fixes this, though.

Needs test updates

[cartermp]

this doesn't appear to be true at all

[vasily-kirichenko]

Maybe a test or two should be added to https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/tests/unittests/BraceMatchingServiceTests.fs?

[saul]

Shouldn't this be range.End = caretPosition?

[cartermp]

That's what I originally thought, but it it results in the same behavior as #3794 (and the current implementation).

Changing to just range.Contains(caretPosition) fixes #3794 and also results the same behavior as the current implementation for #2092 (all cases except for let (| A9 |) and the caret being inside a brace pair result in highlighting).

Unfortunately, debugging this is a real mess. I think it has something to do with debugging CEs, but isPositionInRange is called twice as often as I would expect.

[cartermp]

Actually @saul back to this comment: #2092 (comment)

You fixed brace matching such that (given ^ is the caret position):

letx=(12^)

Will match those braces. I don't think that's the correct thing to do. Given this:

letx=((12)^)

It's not clear which brace should be highlighted. Roslyn throws out the outer pair and so the inner one is matched, but I'm not convinced that we should be giving them both pairs. Thoughts?

[saul]

I'm not entirely sure - does it really matter? I was just trying to fix the bug raised in #2092 which I agreed with. My change was to fix most of those issues, and to also fix the logic for auto-deindent (which I believe this PR breaks, looking at the tests)

[cartermp]

Not sure if it matters, but given that ambiguity I'm leaning towards not counting that case.

What is a bit baffling is that all the test cases in BraceMatchingBothSides_Bug2092 fail if the caret is at the very end of the last brace, but when I run the code outside the context of a test they highlight just fine. Debugging the test shows that span.Contains(caretPosition) clearly fails, but that doesn't appear to matter when the code is being run in the editor. This also breaks auto-deindent as well (for the same reason). I have no idea why this appears to fix the nested braces issue, nor why braces will match just fine with something like this:

letx=[1..10]^

And conversely, when I change it to span.Contains(caretPosition) || span.End = caretPosition, all the tests pass (including a new case: let a9 = (((12)))) and auto-deindent works, but nested braces fail when using the changes in the editor.

[cartermp]

When debugging the editor, we're getting called twice:

caretPosition is different the second time. No clue why that is.

[realvictorprm]

@cartermp is this still valid?

[cartermp]

Yes. The current brace matching implementation doesn't work because of some weirdness with Roslyn calling us twice and passing different values.

[TIHan]

@cartermp There is a reason why it is passing two different values. http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/BraceMatching/BraceHighlightingViewTaggerProvider.cs,77

This is intended behavior. To summarize, here is an example (C#):

()^()

It will check the right side for a starting brace; if there is, it will match. (position)
Then, it will check the left side for an ending brace; if there is, it will match. (position - 1)

I also want to note that IBraceMatcher is an internal API in Roslyn. In the future, we either implement our own version of all of this using IViewTaggerProvider or convince Roslyn to make this public.

[realvictorprm]

@TIHan if it's about reimplementing taggers I can help out here. Thanks to code lens I know too much about them.

[cartermp]

With regards to the API being internal - that's the case for, unfortunately, too many of the things we implement or override. The best thing is to help convince folks to make all the workspace APIs we deal with public so that we can get rid of the internalsvisibleto hack.

[realvictorprm]

Can the community help with convincing them? E.g. writing nice blog posts 😝?

[cartermp]

Not really, no. It's a big cost for a small team with a huge feature area to open them up right now. It affects more than just F#, though, so I'm hopeful we can get to it in 2018.

[cartermp]

Closing this, as there is likely to be more advanced logic required to fix this problem correctly.