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

Add official API for fading out unused code #51104

Closed
mjbvz opened this issue Jun 4, 2018 · 25 comments
Closed

Add official API for fading out unused code #51104

mjbvz opened this issue Jun 4, 2018 · 25 comments

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Jun 4, 2018

Add an api that other extensions and languages can use to grey/fade out unused code and variables. JS/TS already implements this using a proposed API

/cc @jrieken @dbaeumer

@mjbvz mjbvz added this to the June 2018 milestone Jun 4, 2018
@mjbvz mjbvz self-assigned this Jun 4, 2018
@mjbvz mjbvz added the api label Jun 4, 2018
@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 4, 2018

Current Proposed Design
Use diagnostics to power this feature:

/**
 * Additional metadata about the type of diagnostic.
 */
export enum DiagnosticTag {
	/**
	 * Unused or unnecessary code.
	 */
	Unnecessary = 1,
}

export interface Diagnostic {
	/**
	 * Additional metadata about the type of the diagnostic.
	 */
	customTags?: DiagnosticTag[];
}

This approach works well because there are often quick fixes associated with unused code.

Alternative Proposal
Design a new API that allows more generic semantic coloring. Perhaps something similar to DiagnosticCollection

enum HighlightingKind {
    Unused,
}

class Highlight {
    kind: HighlightingKind;
    range: Range;
}

class HighlightCollection {
    // like a diagnostic collection of highlights
} 

This approach would require that we also introduce a new "silent" DiagnosticSeverity that does not render anything in the editor.

@mattacosta
Copy link
Contributor

How does this work with existing language servers that already generate warnings for unused variables? Does the region become grayed out, or do squigglies have precedence, or are both rendered? Is the behavior the same if a language server has a setting that allows the severity of warnings to be increased to errors or decreased to some other severity?

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 5, 2018

@mattacosta This will not effect existing behavior; warnings and errors for unused variables will remain the same. The current proposed api is opt-in and just adds supplemental information to a diagnostic which tells vscode to also render it as faded/grey

@jrieken
Copy link
Member

jrieken commented Jun 6, 2018

Another proposal would be a semantic highlights provider (pull instead of push model). So something like this:

enum HighlightingKind {
    Unused,
}

// maybe allow real styles, e.g color, font-style, etc
class Highlight {
    kind: HighlightingKind;
    range: Range;
}

interface HighlightProvider {
  provideHighlights(document, range, token): ProviderResult<Highlights>
}

@KamasamaK
Copy link

I know the title specifically mentions "fading out unused code", but it seems to me that this would be a more general API. Would it be appropriate to also include something like Deprecated as a DiagnosticTag or HighlightingKind which could have its own associated styling?

@jrieken
Copy link
Member

jrieken commented Jun 11, 2018

@DustinCampbell
Copy link
Member

The Visual Studio editor uses a pull model in it's ITagger stuff. The code you referenced is from the Roslyn infrastructure to support the VS editor.

FWIW, we've found the VS editor's pull model to be very advantageous. Computing semantic highlighting can be very expensive. So, it's nice to be able to...

  1. Compute highlights asynchronously and then provide a notification when they are ready.
  2. Only provide highlights for the current view port (along with some lines above and below) rather than the entire view.

@AndrewKvalheim
Copy link

Weighing in on the gray/faded decision—the current behavior of removing syntax highlighting is confusing because the concern of whether code is used is distinct from whether it's syntactically valid. A common process for me is

  1. Write unused code.
  2. Observe that its syntax is valid.
  3. Use the code.

and completely graying out the code blocks at the second step.

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 12, 2018

@DustinCampbell Is the CustomTags value on diagnostics used at all for colorization of unused variables in VS? That's what I based the current proposed vscode API off of but I don't know how VS actually uses customTags

@DustinCampbell
Copy link
Member

The CustomTags property can be used for information that might be interesting to a consumer that's inspecting the diagnostic (for example, a code fix that responds to that diagnostic). In VS, the presentation layer looks for "Unnecessary" among the CustomTags to decide whether the span should be "faded" (not grayed) -- which is orthogonal from colorization and diagnostic severity.

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 12, 2018

Thanks @DustinCampbell. Our current greyin-out serves the same purpose as fading-out in VS. I'm going to switch us over to use fading out too, grey out was just easier to introduce to start

@jrieken Looks like VS is handling unused fade-out separately from general semantic coloring. Any opposition to doing this as well on our side? That way we don't have to get into implementing generalized semantic colorization just yet. Using diagnostics also makes sense from a language implementer perspective, at least for js/ts

@jrieken
Copy link
Member

jrieken commented Jun 13, 2018

Looks like VS is handling unused fade-out separately from general semantic coloring. Any opposition to doing this as well on our side?

Unsure, looks like a one-off API and we shouldn't shy away from doing the real thing (not saying it must be you and me implementing it 🤣)

@dbaeumer
Copy link
Member

I support @jrieken here. Reasons are:

  • this introduces three phases in which color change which will simply be more disturbing for the user (instead of two)
  • since we neither have CustomTags nor semantic coloring we should try to go for the more general approach. I can imagine that VS introduced CustomTags and later on semantic coloring. If semantic coloring would have been first may be VS would have never used CustomTags to influence coloring (I agree it is a guess). So the real question here is if there is something that can be achieved with CustomTags which can't be achieved with semantic coloring.

@mattacosta
Copy link
Contributor

mattacosta commented Jun 13, 2018

Thanks @DustinCampbell. Our current greyin-out serves the same purpose as fading-out in VS. I'm going to switch us over to use fading out too, grey out was just easier to introduce to start

👍

So the real question here is if there is something that can be achieved with CustomTags which can't be achieved with semantic coloring.

Probably anything that requires presentation of a custom UI element. For example, if for some reason you wanted to show the peek widget along with a diagnostic containing a specific tag. Stuff like that can't be done with semantic coloring (although at that point, it could just as well be a totally separate API).

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 13, 2018

I think unused code is a common enough case that it could be handled by a more targeted Api.

If we use a general semantic coloring API for unused variables, TS for example will have to maintain:

  • A set of invisible diagnostics for the unused symbols. These diagnostics would be used for code actions and hovers
  • A set of semantic coloring locations corresponding to each unused symbol

Not terribly difficult to implement but every language that has unused code support will have to re-implenent the same thing. It's also going to be more chatty in terms of extension communication

I understand that a more general API is preferable but still feel that having some semantic colorizations tied to diagnostics makes sense in cases like unused variables or invoking a deprecated api

@jrieken
Copy link
Member

jrieken commented Jun 14, 2018

/cc @rchande

@DustinCampbell
Copy link
Member

since we neither have CustomTags nor semantic coloring we should try to go for the more general approach. I can imagine that VS introduced CustomTags and later on semantic coloring. If semantic coloring would have been first may be VS would have never used CustomTags to influence coloring (I agree it is a guess). So the real question here is if there is something that can be achieved with CustomTags which can't be achieved with semantic coloring.

I think you might be conflating two features here.

  1. Semantic colorization is a feature that has been in VS for C# since VS 2005 and was rewritten as part of Roslyn.
  2. Fading of code is a diagnostics feature. CustomTags appears on Roslyn's Diagnostic as a way to provide a hints to other components. In this case, the Unnecessary tag provides a hint to the diagnotic presenter to "fade" the span.

The two features are really orthogonal, except that they both use the same tagging API in the VS editor to actually perform the highlighting. These taggers provide different IClassificationTags that overlap. However, the IClassificationTag for fading is at a higher priority and just changes the opacity of the span. So, when the VS editor presents the tags, the semantic colorization IClassificationTags are visible beneath the faded IClassificationTag.

So, in VS there is a very general API for classifying spans in the editor. The semantic colorizer and diagnostics with CustomTags that provide hints for fading code are really party of Roslyn.

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 14, 2018

I reviewed some existing vs code language implementation and believe they would be able to easily adopt a CustomTags type property. Most already show unused variables as warnings. For what it's worth, intellij also seems to be using diagnostics for their dead code highlighting.

Other types of tags we could consider adding in the future:

  • Deprecated
  • Experimental / platform specific
  • Redundant (probably the same as unnecessary)
  • Uninitialized

The important part is that these potential tags would provide both a colorization modifier and hover information that tells users exactly why the colorization is occurring and how to fix it.

@mattacosta
Copy link
Contributor

@mjbvz I'm not sure if I entirely follow. As I understand it, your proposal is to modify the definition of an LSP diagnostic in order to reuse the diagnostic provider, but I don't see a reason why a language service, with its own custom definition of a diagnostic, couldn't just send them to its own highlighting provider which would in effect just pass-through the relevant data.

In general though, my primary concern with the diagnostic approach is that it may not work well for certain languages and/or situations. For example, it's entirely reasonable for unused code within a preprocessor directive to not have a diagnostic at all, since there is nothing inherently wrong, or for a diagnostic span to not cover the same area that should be faded.

@mjbvz
Copy link
Contributor Author

mjbvz commented Jun 14, 2018

Yes they could. There are a few problems with this approach however:

  • Language services now have to maintain both diagnostics and highlights. How this is implemented may not be consistent
  • The colorization and diagnostics can get out of sync
  • It's more chatty
  • We don't have a story for generalized semantic coloring yet and solving it is complex

Even in the preprocessor case, I believe we still would want to provide some sort of feedback to users on why a piece of code is faded out (and optionally quick fixes and other tools to fix this). Using diagnostics lets us to that

@DustinCampbell
Copy link
Member

Another concern is that the analysis to determine the symbol kind of identifier (semantic highlighting) and where a piece of code or not is very different and the latter is potential much more expensive.

@mattacosta
Copy link
Contributor

mattacosta commented Jun 15, 2018

Agreed.

I also wanted to add that as more and more tags are added, you are also asking the code that generates the diagnostics to perform more and more potentially complex tasks. All for an editor that may not even support fading (or some other yet to be added feature) in the first place.

@DustinCampbell
Copy link
Member

@mattacosta: To be clear, I was arguing that faded code detection should be part of diagnostic analysis. Semantic highlighting is an orthogonal feature. However, deciding that something should be faded often happens during diagnostic analysis. For example, it would be during diagnostic analysis that an unnecessary cast would be identified. That could be a warning, error, ignored, etc. However, the presentation in the editor should be that it's faded.

@mattacosta
Copy link
Contributor

mattacosta commented Jun 15, 2018

@DustinCampbell The way I see it, semantic highlighting can have many possible triggers. In the case of unused variables or an unnecessary cast, I agree that diagnostics should be one of those triggers, but we should avoid using tags on a bunch of (probably hidden) diagnostics as the sole trigger. For example, someone may want to trigger semantic highlighting (in this case fading) on a symbol that has no diagnostics attached to it. In this case, it wouldn't make sense to add a diagnostic with a tag just so the editor can find it later, because the highlighter could run its own logic to find it.

@DustinCampbell
Copy link
Member

For example, someone may want to trigger semantic highlighting (in this case fading) on a symbol that has no diagnostics attached to it.

Can you think of a concrete user scenario or is that just hypothetical?

FWIW, I think there's a lot of "violent agreement" here. As I mentioned, in Visual Studio, we provide separate classifiers (i.e. "highlighters") to handle different semantic highlighting. This is similar to the "many possible triggers" that you described. 😄

@mjbvz mjbvz closed this as completed in 2e5253d Jun 18, 2018
benjaminjackman added a commit to benjaminjackman/vscode that referenced this issue Jul 4, 2018
This commit allows users to set a color for unused code which had been an
unconfigurable behavior (defaulting to a shade of gray) until microsoft#51104 / microsoft#52218 when
it was replaced with setting that allows for either decreasing the unused
token's opacity while preserving its color and/or setting the color of
a 2px dashed border that "underlines" the unused token.

The previous behavior avoided some contrast issues introduced by microsoft#51104 / microsoft#52218
when reducing the opacity of some syntax colors that were already at
or just over the low-contrast frontier into squintland, the unlegible
country, a place no programmer should be forced gaze.

Conversely, tokens that don't contrast enough with themselves won't scan
well when the programmer is looking for unused tokens.
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants