use SymbolDisplayFormat in TypeLookup

[filipw]

Instead of manually composing type information, use SymbolDisplayFormat to format the response according to the predefined rules:

• default SymbolDisplayFormat.FullyQualifiedFormat but without global:: for regular C# syntax
• SymbolDisplayFormat.FullyQualifiedFormatbut without global:: and without namespace for script/interactive syntax

Fixes dotnet/vscode-csharp#394 (currently OmniSharp doesn't show type lookup information for nested types correctly)

[DustinCampbell]

Eventually, we'll just use this API: dotnet/roslyn#13623.

[DustinCampbell]

BTW, here's what we use in Roslyn: CSharpSymbolDisplayService.SymbolDescriptionBuilder.s_MinimallyQualifiedFormat.

For hover Quick Info tips, we almost always minimally-qualify by called ISymbol.ToMinimalDisplayParts.

[filipw]

However MinimallyQualifiedFormat skips namespace / containing type information. For example calling symbol.ToMinimalDisplay... on Foo.Bar.Xyz returns just Xyz, is that the intention? Because OmniSharp has shown fully qualified info

[DustinCampbell]

Note that this endpoint gets called for a lot more than just types. The hoverProvider in VS Code which just calls it with any old document and position. With your change, I suspect that a lot of things will be fully-qualified that weren't in the past. For example, if you hover over Console.WriteLine(), I bet you'd get System.Console.WriteLine()

[DustinCampbell]

One other thing: minimal qualification only skips namespace / containing type information if you're in the same type and namespace. Foo.Bar.Xyz would return Foo.Bar.Xyz if it's referenced in a different type. Showing all of the information inside the same type/namespace is redudant. However, minimal qualification really only works if you call ToMinimalDisplayString. The MinimallyQualifiedFormat doesn't really do much on its own. See how it's used: http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/SymbolDisplay/SymbolDisplay.cs,93

[filipw]

i've rebased this onto the tip of dev.

Can we take this change in? It doesn't change any existing OmniSharp behavior or format (still shows fully qualified tooltips as always) - except it correctly shows the names of nested classes now.

The verbosity around SymbolDisplayFormat is related to just that.

[DustinCampbell]

@filipw: Could you at least add some unit tests that ensure that OmniSharp behavior didn't change for other kinds of symbols? Also, it really might be worth a change like so to ensure that your change is only for named types and doesn't affect any other kinds of symbols (like methods, events, properties, parameters, local variables, etc., etc.):

if (symbol.Kind == SymbolKind.NamedType)
{
    if (document.SourceCodeKind == SourceCodeKind.Regular && !symbol.ContainingNamespace.IsGlobalNamespace)
    {
        response.Type = symbol.ToDisplayString(DefaultFormat);
    }
    else
    {
        response.Type = symbol.ToDisplayString(SimpleFormat);
    }
}
else
{
    response.Type = symbol.ToDisplayString(SymbolDisplayFormat.SymbolDisplayFormat.MinimallyQualifiedFormat);
}

[DustinCampbell]

At the very least, more comprehensive unit tests would be good. I really don't want to take a regression on hover tips into VS Code when it could have been avoided.

[DustinCampbell]

You changed this but there aren't any tests for it. All of the tests are for types, but this endpoint clearly handles all sorts of symbols.

[DustinCampbell]

E.g. what happens if you hover over WriteLine in Console.WriteLine(42) with this change?

[DustinCampbell]

FWIW, since we've got the semanticModel and the position, it's probably a better experience to do:

response.Type = symbol.ToMinimalDisplayString(semanticModel, position);

[DustinCampbell]

I think the problem is that the if only checks named types, in regular source code where the global namespace is null. That means that anything else, method symbols, parameters symbols, local symbols, etc. -- or named types contained in script -- are all treated the same.

I think the if needs another clause. Something like this:

if (symbol.Kind == SymbolKind.NamedType)
{
    if (document.SourceCodeKind == SourceCodeKind.Regular && !symbol.ContainingNamespace.IsGlobalNamespace)
    {
        response.Type = symbol.ToDisplayString(DefaultFormat);
    }
    else
    {
        response.Type = symbol.ToDisplayString(SimpleFormat);
    }
}
else
{
    response.Type = symbol.ToMinimalDisplayString(semanticModel, position);
}

[filipw]

thanks for the feedback!

I think it's a great suggestion to add more comprehensive unit tests - regardless of whether we merge anything proposed here or not, the tests will at least be useful for any potential changes in the future to avoid regressions.

I'll do that.

[DustinCampbell]

I was looking at doing that too -- in case you were busy. 😄

[DustinCampbell]

Note that this endpoint gets called for a lot more than just types. The hoverProvider in VS Code which just calls it with any old document and position. With your change, I suspect that a lot of things will be fully-qualified that weren't in the past. For example, if you hover over Console.WriteLine(), I bet you'd get System.Console.WriteLine()

[DustinCampbell]

One other thing: minimal qualification only skips namespace / containing type information if you're in the same type and namespace. Foo.Bar.Xyz would return Foo.Bar.Xyz if it's referenced in a different type. Showing all of the information inside the same type/namespace is redudant. However, minimal qualification really only works if you call ToMinimalDisplayString. The MinimallyQualifiedFormat doesn't really do much on its own. See how it's used: http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/SymbolDisplay/SymbolDisplay.cs,93

[filipw]

I have simplified the logic and pushed a total of 15 tests for different variants of hints.
As a bonus, this PR now fixes dotnet/vscode-csharp#250 as well (OmniSharp will now have the same behavior as Visual Studio).

I have run the tests against old implementation of TypeLookup and the only only difference in output is related to nested types (the original purpose of this PR) and dotnet/vscode-csharp#250.

Actually, there is one more "byproduct" of this change and I wanted to highlight it to make sure this makes sense.
When looking at a type hint for something like IDictionary<string, IEnumerable<int>>, at the moment OmniSharp would respond with System.Collections.Generic.IDictionary<string, IEnumerable<int>>. This is a bit halfway IMHO and inconsistent with other type hint OmniSharp shows.

This change would make this type hint System.Collections.Generic.IDictionary<System.String, System.Collections.Generic.IEnumerable<System.Int32>> which to me makes more sense.

[filipw]

I think down the road it would be nicer to have different output for generic types, like VS does:

But I assume dotnet/roslyn#13623 will take care of that, so no point in rolling out anything by hand?

[DustinCampbell]

Nice! And the code is simpler too!

[DustinCampbell]

@filipw: Yes, we'll get that with the Roslyn API. It's several months off but it will pretty much remove any code we have in this endpoint and add support for generic type maps (as you showed), proper XML documentation display (with minimally-qualified crefs), and all of the other subtleties in C#'s Quick Info tool tips.

It'll even give us the anonymous type map displays, which are very tricky to get right.