Various navigation bar bugs

[saul]

I've found a few issues with the navigation bar implementation on master:

1. Properties with a getter and setter are shown twice (once for the setter, once for the getter)
2. Private values are shown as methods as opposed to private fields
3. Methods do not show the parameters list as they do in C#

Demonstration of issue 1

Demonstration of issue 2

Issue 3: example of parameter list as in C#

/cc @vasily-kirichenko

[cartermp]

Confirmed all three. I think that the first two are bugs. The latter is more of a feature request IMO.

[cartermp]

I noticed another interesting issue. Given the following code:

type MyClass() =
    let mutable value = "hello"

    let tupleize x y = (x, y)

    member this.TurnIntoTuple(x: string, y: int) = tupleize x y

    member private this.Whatever() = ()

    member this.MyProperty
        with get() = value
        and set(s) = value <- s

value is being treated as a method, and tupleize is being treated as a constant. That classification is being assigned in ServiceNavigation.fs.

For example, tupleize matches this case, which causes it to show up as a constant: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/vs/ServiceNavigation.fs#L138

[saul]

Out of interest - do we not have any tests for NavigationImpl? If not it seems like a simple thing to write tests for.

[vasily-kirichenko]

We have a single test in VFPT https://github.com/fsprojects/VisualFSharpPowerTools/blob/master/tests/FSharp.Editing.Tests/NavigableItemsCollectorTests.fs#L46, I think it'd be great to port it to this repo and add some more tests.

[cartermp]

Agreed, this could be quite helpful.

[vasily-kirichenko]

Ah, sorry, wrong test in my message. We reuse the implementation that's available in FCS (was written by @7sharp9 if I'm correct) and is used in XS. So, no tests I think.

About three duplicates for properties, it's expected because AST contains one node for property itself, one for getter and one for setter. I think this piece of code may be helpful in fixing the bug: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/vs/ServiceParseTreeWalk.fs#L418-L454

About method signatures, it's not a simple thing to implement. We could try to use quick info (toolpips) for this, as we did in VFPT for Quick Info Panel, but what we did there was just getting the first line from the tooltip + some simple filtering. However, tooltip require type check results while Nav Bar uses parse results, which are orders or magnitude faster to get. So, tooltips are not an option.

About the glyphs, it should be fixed.

[cartermp]

@vasily-kirichenko Hmmm, interesting. I wonder if there's a way to special-case the fact that the AST has a node for both the getter and setter just for the navigation service?

[saul]

Maybe take a look at refactoring Tests.LanguageService.NavigationBar.fs? They look relevant and have been disabled since Roslyn move.

[vasily-kirichenko]

@saul The old navbar calls the same ast traversing code https://github.com/Microsoft/visualfsharp/blob/efde2a8758b3f35e6637cd1d63d7aa370129d2af/src/fsharp/vs/ServiceNavigation.fs#L403