Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude additive classifications from "/highlight" requests #1726

Merged
merged 2 commits into from Mar 10, 2020

Conversation

DasCleverle
Copy link
Contributor

Currently the behavior of the "/highlight" request is not deterministic for static symbols. This is because the Classifier class returns two ClassifiedSpans for static symbols (one with the actual kind and one with kind "static symbol"). And depending on which of the two spans is returned first in the result we get a different highlight span.

I propose a change where we remove these "additive" kinds from the result, only returning the actual kind of the span.

Fixes #1576

@nickspoons
Copy link
Member

I can confirm that this PR fixes #1576, thanks @DasCleverle!

I'm not sure if it is related but I notice that WriteLine has kind method name when it is contains anything valid but has kind identifier when it contains e.g. an undeclared variable:

Console.WriteLine("a string");
Console.WriteLine(5);
object o = "another string";
Console.WriteLine(o.ToString());
Console.WriteLine(undeclared_variable.ToString());

image

Is this another situation where there are multiple spans?

@DasCleverle
Copy link
Contributor Author

No, it seems that Roslyn can't determine the actual kind when there is a semantic error. This is what I got for the sample you provided:

image

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, this is a very good PR.
it's the same that Roslyn does internally https://github.com/dotnet/roslyn/blob/9ee3ac69f99cbcd645d494e0b0ee44616c1725e0/src/Workspaces/Core/Portable/Classification/ClassifierHelper.cs#L49-L55

(we might want to switch to their classifier in the future completely too, but that's a separate story)

@nickspoons
Copy link
Member

I don't think the CI errors have anything to do with the PR - can someone trigger them for a rebuild?

@dnfclas
Copy link

dnfclas commented Mar 9, 2020

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ DasCleverle sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@filipw filipw merged commit f0761dd into OmniSharp:master Mar 10, 2020
@DasOhmoff
Copy link

Thank you all very much for fixing this issue. It is very appreciated <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent /highlight Kinds
6 participants