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

Better classification: such colors, much wow #9511

Merged
merged 16 commits into from
Jun 23, 2020

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 19, 2020

This all started because I wanted to fix #4618 and then fix #6117

Firstly, this PR produces a lot more distinct classification data than before. This can allow a much finer degree of control over what an editor chooses to do with it, such as coloring. It is done in a way that matches how we compute FSharpGlyph data.

For VS specifically, we do centralize a lot of these classification types to more common Roslyn classifications. That said, a lot have been updated so that we can have some Fancy Colors.

Currently, to get these Fancy Colors in VS, you'll need to toggle a setting int he C# settings. I will endeavor to submit a PR to Roslyn to move this to a more central location, or have a similar setting specifically for F#.

image

Once this is turned on, you'll see the pretty colors.

Structs are distinct from classes. Methods and properties are colored distinctly:

image

This applies to records and DUs too:

image

image

Anonymous record labels are now classified:

image

Functions, methods, constructors are colored similarly, unless they are record labels. Note the distinction between constructor invocation and qualifying something with the class name:

image

image

Disposable types and bindings are classified (color TBD, I picked Colors.Tomato because the name seemed funny to me):

image

image

(note that the this was colored, but if you mark it as a discard then it won't be)

image

Here's what a lot of different things (generics, class, structs vs classes, UoM, functions, type abbreviations, exceptions) look like together:

image
image

And here's what it looks like in some larger, more involved/complicated parts of the F# compiler codebase:

image

image

image

THIS ALSO FIXED SOME STUFF

Indexers now don't color the . like it was a class

image
image

Multiple active patterns can still be marked as "unused" and the ranges for that are still bonkers, but at least it doesn't show random white characters anymore:

Before:

image

After:

image

BUT GENERIC PARAMETERS CAN STILL BE WEIRD

@cartermp cartermp changed the title [WIP] Such colors, much wow Better classification: such colors, much wow Jun 19, 2020
[
FSharpClassificationTypes.MutableVar, (Color.FromRgb(160uy, 128uy, 0uy), Color.FromRgb(255uy, 210uy, 28uy))
FSharpClassificationTypes.Printf, (Color.FromRgb(43uy, 145uy, 175uy), Color.FromRgb(78uy, 220uy, 176uy))
FSharpClassificationTypes.Disposable, (Colors.Tomato, Colors.Tomato)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't be the actual color picked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also lmao "picked" because it's a tomato

Copy link
Contributor

Choose a reason for hiding this comment

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

aww why not keep tomato = disposable

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I already love Tomato, IDisposable is scary, and tomatos are explosive, I like the combo :)

@charlesroddie
Copy link
Contributor

charlesroddie commented Jun 19, 2020

This implements #4618 . So 👍. Tomatoes are disposable so it's a sensible choice.

@Krzysztof-Cieslak
Copy link
Contributor

One thing that VSCode does in its semantic highlighting API is separating token type and modifiers - each token can be classified as a single type and has zero or more modifiers.

From VSCode docs:

Standard semantic token types:

  • namespace
  • type, class, enum, interface, struct, typeParameter
  • parameter, variable, property, enumMember, event
  • function, member, macro
  • label
  • comment, string, keyword, number, regexp, operator

Standard semantic token modifiers:

  • declaration
  • readonly, static, deprecated, abstract
  • async, modification, documentation, defaultLibrary

This means instead of having DisposableValue and DisposableType or Mutable... things like disposable or mutable would be a modifier. I wonder if it would be worth exploring a similar approach.

@cartermp
Copy link
Contributor Author

Alternative: Locals colorized, properties (and property-like things) are not

image

image

Top-level declarations (such as those in scripts, or defined in modules) are not colored:

image

image

Copy link
Member

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

This looks straight forward to me. I like the additional semantic classification types.

Copy link
Contributor Author

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

There's still some additional design considerations left:

  • Work through a better color (that meets accessibility standards) for disposables
  • Consider going back to the old behavior for constructors where they looked like the types they were constructing
  • Consider renaming ReferenceType and ValueType to Class and Struct

src/fsharp/service/SemanticClassification.fs Outdated Show resolved Hide resolved
src/fsharp/service/SemanticClassification.fs Outdated Show resolved Hide resolved
@cartermp
Copy link
Contributor Author

Okay, decided to revert the "color ctors as methods" approach. They're classified as specific kinds of constructors, but the VS coloring will use the current approach of coloring the constructors like the type:

image

All other considerations I had are now resolved. The big remaining things are:

  1. Figure out an accessible color for disposables (will need to talk to UX folks at Microsoft for this)
  2. Figure out how to move the current setting that handles this in Roslyn to be something we can expose in F# settings, and not let the C# one override anything we do. Will need to talk to some Roslyn folks for that.

@abelbraaksma
Copy link
Contributor

the VS coloring will use the current approach of coloring the constructors like the type:

Does this only apply for specific ctors with new, or also "ctors as (hof) function"? I.e. let c = Classy() or let f = Classy in f()?

@jmarolf
Copy link

jmarolf commented Jun 22, 2020

image

@cartermp
Copy link
Contributor Author

@abelbraaksma classification depends on name resolution, so the answer is "whatever the compiler resolves it to be". That kind of "constructor" isn't a constructor, but a function that returns an instance of something. So the function is resolved as a function, which leads to getting classified as such. It's no different than a function which constructs a new disposable via object expressions.

@cartermp
Copy link
Contributor Author

From the VSCode spec, the following are covered by this API:

  • namespace
  • class
  • enum
  • interface
  • struct
  • typeParameter
  • property
  • event
  • function
  • member
  • argument lavel
  • comment
  • string literal
  • keyword
  • numeric literal
  • operator
  • variable (in F#, this will be "LocalVaue" since it's non-top-level declarations)

The following are not:

  • type - unclear how this would matter for F#, since F# types are classified but ultimately are either classes or structs
  • parameter - distinguishing parameters from non-top-level values could probably be done by looking at the enclosing type and inspecting how it's defined. Not sure how to do that, and such distinctions are intentionally not drawn in the F# type system anyways
  • macro - F# does not support macros
  • regexp - F# doesn't distinguish regexp strings from normal strings. There is a Roslyn component we could use in VS, not sure if that's sharable with other tools.

@abelbraaksma
Copy link
Contributor

That kind of "constructor" isn't a constructor, but a function that returns an instance of something. So the function is resolved as a function, which leads to getting classified as such.

Got it, makes sense. I was mainly just curious. Coloring a "ctor as a function" as a function, because it's a function, seems pretty ok to me.

I really like this pr, it's absolutely awesome!

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 23, 2020

type - unclear how this would matter for F#, since F# types are classified but ultimately are either classes or structs

Do you mean "type alias" here? Cause I can see some benefit in a (slightly) different color to distinguish type X = 'a -> 'b when X is used. Now I often find myself hovering over it, or F 12-to-definition, to find out if it's a true type or not (it sometimes matters, esp in deconstruction syntax).

Though in practice I guess that's pretty rare, since usually you don't specify the type anyway.

@cartermp cartermp merged commit 4456e0a into dotnet:master Jun 23, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* First go at updated classifications

* More complete classification

* More accurate classification that roughly matches glyph computations

* Proper measure classification and tests

* remove ze comments

* Add clarifying comment

* Distinguish property setter args from named argument labels

* Color local values, don't color properties and property-like things that way

* Dont't do the dumb

* We can't distinguish between params and locals right now

* Updates per feedback from myself

* do discards right

* Accessible colors for disposables + some fixes

* Remove exports for things we don't do anymore

* Softer green

* Reduce diff
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* First go at updated classifications

* More complete classification

* More accurate classification that roughly matches glyph computations

* Proper measure classification and tests

* remove ze comments

* Add clarifying comment

* Distinguish property setter args from named argument labels

* Color local values, don't color properties and property-like things that way

* Dont't do the dumb

* We can't distinguish between params and locals right now

* Updates per feedback from myself

* do discards right

* Accessible colors for disposables + some fixes

* Remove exports for things we don't do anymore

* Softer green

* Reduce diff
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* First go at updated classifications

* More complete classification

* More accurate classification that roughly matches glyph computations

* Proper measure classification and tests

* remove ze comments

* Add clarifying comment

* Distinguish property setter args from named argument labels

* Color local values, don't color properties and property-like things that way

* Dont't do the dumb

* We can't distinguish between params and locals right now

* Updates per feedback from myself

* do discards right

* Accessible colors for disposables + some fixes

* Remove exports for things we don't do anymore

* Softer green

* Reduce diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants