Add official API for fading out unused code

[mjbvz]

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]

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]

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]

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

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]

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]

@mjbvz Roslyn sees to use a pull-model (http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Classification/SemanticClassificationUtilities.cs,2ef4a2c8d3e1fe6f,references) but @DustinCampbell might know more about it

[DustinCampbell]

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]

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]

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

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]

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]

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]

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]

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]

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]

/cc @rchande

[DustinCampbell]

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]

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]

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

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]

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]

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]

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

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

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