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

Tag items in tooltips consistenly with respect to document classification (such colors, much wow) #9563

Merged
merged 61 commits into from
Nov 17, 2020

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 25, 2020

Fixes #5385 and various other inconsistencies. Also fixes #9561 and fixes #6160

WE GOT US SOME MORE COLORS FOLKS

Value types are now colored in tooltips (as they are in source). Parameters and properties (and record labels) are colored just like parameters in source:

image

image

Methods have the same coloring as methods in source:

image

Functions have the same coloring as functions in source:

image

Methods and constructors follow the + N overloads paradigm and properties with get/set aren't shown twice anymore:

image

Generic type instantiations in tooltips show the correct color for their type kind:

image

.NET-defined types now also have the same fidelity as F#-defined types w.r.t colorization:

image

Another example:

image

@cartermp
Copy link
Contributor Author

cartermp commented Jun 25, 2020

Ugh, aligning QuickInfo data with classification data is going to be challenging. But it would fix at least one additional bug in this repo. The QuickInfo data is ultimately computed here: https://github.com/dotnet/fsharp/blob/master/src/fsharp/symbols/SymbolHelpers.fs#L1041

So this could be adjusted (and what it calls) to be similar to Semantic Classification. Or a bigger refactoring could be done where everything ultimately points to the same "classification" routine. Will need to think about this one a bit, since the needs of "classifying" things in a tooltip could be different from the needs of classifying a document. Aligning these things would mean a pretty significant enhancement to the layout type, which is used in FSharp.Core pretty printing and FSI pretty printing.

I'm definitely not interested in a complete revamp like how Roslyn does things though, since that would be an enormous change.

@cartermp

This comment has been minimized.

@cartermp

This comment has been minimized.

@@ -64,7 +64,25 @@ module internal PrintUtilities =
| (x :: rest) -> [ resultFunction x (layoutFunction x -- leftL (tagText (match rest.Length with 1 -> FSComp.SR.nicePrintOtherOverloads1() | n -> FSComp.SR.nicePrintOtherOverloadsN(n)))) ]
| _ -> []

let layoutTyconRefImpl isAttribute (denv: DisplayEnv) (tcref: TyconRef) =
let layoutTyconRefImpl isAttribute (denv: DisplayEnv) (tcref: TyconRef) =
let tagEntityRefName (xref: EntityRef) name =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

necessary?

@cartermp cartermp changed the title Better QuickInfo colors Tag items in tooltips consistency with respect to document classification (such colors, much wow) Jun 28, 2020
@cartermp cartermp changed the title Tag items in tooltips consistency with respect to document classification (such colors, much wow) [WIP] Tag items in tooltips consistency with respect to document classification (such colors, much wow) Jun 30, 2020
@cartermp

This comment has been minimized.

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

cartermp commented Jul 2, 2020

Further checklist:

  • Update old VS tests
  • Suppress non-immediate vals/static vals
  • More signature generation woes
  • Figure out why the .cctor is still written with --sig
  • review enums
  • handle delegates right
  • Empty types should not show =
  • Add struct attribute to objects that are structs
  • Add measure attribute to nonempty UoM
  • Show or suppress interface impls on types? System.Collection.Generic.List<_> now shows all interface impls...
  • Do we do with get when it's only a getter, or leave that all off?
  • DontShowCompilerGenNames01 signature (member z.M2 ((x, y) : int * string) = ignore) doesn't show tupling when hovering over T, parentheses are missing in SignatureWithOptionalArgs01
  • Type layout is used by --sig and this code breaks all of that with the overload flattening for members, probably other things too - must investigate
  • Member accessibility is lost (InternalAccessibility02) - specifically, private is now missing

@cartermp cartermp changed the title [WIP] Tag items in tooltips consistency with respect to document classification (such colors, much wow) [WIP] Tag items in tooltips consistenly with respect to document classification (such colors, much wow) Jul 11, 2020
@cartermp
Copy link
Contributor Author

#6160 is also fixed here

@cartermp cartermp changed the title [WIP] Tag items in tooltips consistenly with respect to document classification (such colors, much wow) Tag items in tooltips consistenly with respect to document classification (such colors, much wow) Jul 13, 2020
@abelbraaksma
Copy link
Contributor

I didn't test my bug report against a build with the changes of this PR, so it's possible it has already been tackled here. Be as it may, I think it's at least related: #9771 (in short: certain constraints like enum, comparison, not struct etc are not colored blue).

@cartermp
Copy link
Contributor Author

Left a comment there; this PR doesn't do anything to help the situation. Name resolution lacks the ability to properly distinguish or at least emit data that distinguishes the cases you've noted there.

@cartermp cartermp requested a review from TIHan August 13, 2020 01:46
@brettfo brettfo changed the base branch from master to main August 19, 2020 20:01
@cartermp cartermp force-pushed the better-quickinfo-colors branch 2 times, most recently from 90e29ac to e54f9f1 Compare August 26, 2020 21:50
@cartermp cartermp closed this Aug 29, 2020
@cartermp cartermp reopened this Sep 1, 2020
@cartermp
Copy link
Contributor Author

cartermp commented Sep 1, 2020

Would appreciate a review at some point @KevinRansom @TIHan @dsyme

@cartermp cartermp force-pushed the better-quickinfo-colors branch 2 times, most recently from 4364551 to 9ad7f48 Compare September 6, 2020 02:25
@cartermp
Copy link
Contributor Author

cartermp commented Sep 6, 2020

I can't update baselines on my machine the "proper" way, since my machine will BSOD if I run tests in release.

pls halp

@dsyme
Copy link
Contributor

dsyme commented Sep 8, 2020

Looking at #9511 and this I thought I'd write up my opinions on the distinctions being made in colorization and classifications. These are "just my opinions" but I'll try to explain them.

This is mostly written with regard to the colorization of code though similar things can apply to tooltips

Regarding methodology - in the absence of user testing (very very hard), one methodology is to consider what distinctions matter in the mind of the F# user and when, and how intrusive the distinction is to the act of coding. Three principles seem to be

  • Distinctions can be prioritised besed on the likelihood that the distinction is highly relevant to the typical (assumed) tasks being performed when writing and understanding F# code.

  • Distinctions should be "appropriately intrusive" for the likely signficance of the distinction being made (e.g. types are all colored greenish, because the difference between them is not that significant, but mutable values get a substantial difference, because the distinction matters)

  • There is a cognitive cost to making distinctions (I spent quite a while wondering about why half my code has different colors)

Now, there are literally zillions of distinctions we could make, e.g. struct v. reference, union v. record, big v. small, delegate v. function, disposable v. non-disposable, static v. non-static, public v. non-public, types  with a Count property and without, etc. etc.

For F#, we shouldn't just suck up C#'s distinctions. However, the F# language design is rooted in the idea that you have values (let) and types (type).  Different types and values have different characteristics, but much is done to emphasise similarity between these, rather than distinction. For example, a union type is different to a record type but both can have members. Iin most code, most the time, the user doesn't care if something is implemented as a record or a union.  Because of this, emphasising the difference in implementation would be wrong.

At the extreme, in F# a signature file can just say "type X" and be essentially non-commital about what "X" is - it doesn't matter if its an interface, class, delegate or whatever (though for technical reasons its structness must be known). Those are implementation details - what matters in functional programming is the set of operations you can do with "X". That said, a type's representation can make some particular operations available (e.g. pattern matching)

Anyway, let's take some examples and I'll give my opinions. These can be read as either arguing for an elimination of a distinction or the reducing of the "size" of the distinction.

  1. Types - "delegate v. enum v. ..."

    Don's opinion: mild preference for no distinction (or at most a subtle distinction on types)

    Explainer: To me this is importing C# OO distinctions into F#. I'm inclined not to do that in the colorizations but I know opinions will differ on this.  Further, we don't do union v. record, for example (and I don't think we should).

  2. Types - value type v. reference type

    Don's opinion: mild preference for no distinction (or at most a subtle distinction on types)

    Explainer: This is a little suspect to me - In F#, putting "Struct" on something often doesn't change the semantics at all, just the operational properties. What the F# programmer might care about is referential transparency (e.g. the thing is a value where deep-cloning it doesn't matter). But that's not something we have knowledge of.

    Caveat: That said I'm likely used to a subtle difference in highlighting of these and of all the type distinctions we could make it's not too bad.

  3. Types - interface v. non-interface

    Don's opinion: mild preference for no distinction (or at most a subtle distinction on types)

    Explainer: Personally, I don't like this distinction. The I prefix distinction in type name is already enough. But opinions vary on this. In F#, it often doesn't matter if something is an interface type (e.g. seq is an interface but who cares?), and the F# programmer rarely implements an interface.

    Caveat: This may matter more when doing outright OO programming in F#.

  4. Types - abbreviation v. non-abbreviation

    Don's opinion: strong preference for no distinction

    Explainer: In F# the programmer thinks of the abbreviation as equivalent to the non-abbrviated type. They shouldn't be shown in different colors

  5. Disposable v. non-DIsposable (types and values)

    Don's opinion: strong preference for no distinction (or at most a very subtle distinction on values)

    Explainer: The distinction is important to some extent, at particular phases, and particularly at the declaration of t disposable value when a "use" might be necessary. But coloring every use of a disposable value looks very wrong. I sent @cartermp some examples

    I definitely don't like coloring disposable types in a different color

  6. Values - Direct mutability v. not

    Don's opinion: strong preference for fairly strong distinction (as we have today)

    Explainer: I think this is a good semantic distinction to highlight, it's been discussed and is effective in helping you get code correct

  7. Values - function/method v. value

    Don's opinion: strong preference for fairly strong distinction (as we have today)

    Explainer: It seems this is based on the value's type (e.g. a parameter of function value is colored). OK, fair enough

  8. Values - top level v. non-top-level (open-scope v. limited scope)

    Don's opinion: I can see the value for a subtle distinction - among other things it likely helps to orient the programmer when they are in a large module v. a large class/function body

    Explainer: Right now there is a fairly subtle distinction between Module/member and class/expression bindings. I don't have strong feelings about this as the distinctions are very subtle. But it's worth noticing that it's not distinguishing static. v. non-static, in the following I'm seeing xxxxxxxxxxxxxxxx and yyyyyyyyyyyyyyyy get different colors.

    module M = 
        let xxxxxxxxxxxxxxxx = 1
    
    type C() =
        static let yyyyyyyyyyyyyyyy = M.xxxxxxxxxxxxxxxx
    
  9. static v. non-static

    This distinction is not currently being coloured.

@cartermp Are there other distinctions planned I could give my 2c worth on?

@cartermp cartermp force-pushed the better-quickinfo-colors branch from c59ad5e to 4ad02be Compare November 12, 2020 19:25
@cartermp
Copy link
Contributor Author

This is ready for review @dsyme @TIHan @KevinRansom @auduchinok

src/fsharp/TypedTreeOps.fs Outdated Show resolved Hide resolved
src/fsharp/TypedTreeOps.fs Outdated Show resolved Hide resolved
src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
src/fsharp/NicePrint.fs Outdated Show resolved Hide resolved
src/fsharp/NicePrint.fs Show resolved Hide resolved
src/fsharp/NicePrint.fs Show resolved Hide resolved
Copy link
Contributor

@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.

Looks great! Looking forward to seeing the consistency.

Only thing is the isFunctionTy should be a different name so we don't get confused by isFunTy.

@cartermp cartermp added this to the 16.9 milestone Nov 17, 2020
@cartermp cartermp merged commit f86a3ae into dotnet:main Nov 17, 2020
@cartermp cartermp deleted the better-quickinfo-colors branch November 17, 2020 23:49
@lfr
Copy link

lfr commented Nov 25, 2020

Is this a known bug?

image

Compiles fine though, so no biggie.

@lfr
Copy link

lfr commented Dec 2, 2020

Please disregard my previous comment, it's fine in 5.0

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants