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 unused diagnostic subtype #49646

Merged
merged 2 commits into from
May 17, 2018
Merged

Add unused diagnostic subtype #49646

merged 2 commits into from
May 17, 2018

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented May 10, 2018

Fixes #15710

Adds a new DiagnosticTag class that provide additional information about a diagnostic. Introduce DiagnosticTag.Unnecessary to mark when a diagnostic is for unused / unnecessary code

The design comes from Rosyln's diagnostic object and allows us to modify how a diagnostic is rendered without changing its serverity.

Hooks up JS and TS to use this new tag. This is controlled by the javascript.showUnused.enabled setting which is enabled by default

Alternate design considered

  • Introduce a new diagnostic severity for unused.

    However, using this approach, if a user sets noUnusedLocals in their tsconfig.json, the resulting diagnostic could only show the squiggly OR be grayed out. Using customTags allows us to support both graying out and showing the squiggly

  • Custom JS/TS implementation using decorators

    Not themable. We want a standard experience across languages.

@mjbvz mjbvz added this to the May 2018 milestone May 10, 2018
@mjbvz mjbvz self-assigned this May 10, 2018
@mjbvz mjbvz requested a review from jrieken May 10, 2018 20:36
@mjbvz
Copy link
Collaborator Author

mjbvz commented May 10, 2018

/**
* Unused or unnecessary code.
*/
Unnecessary = 'unnecessary'
Copy link
Member

Choose a reason for hiding this comment

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

@jrieken
Copy link
Member

jrieken commented May 17, 2018

What other tags do we envision? Any of those used by Roslyn seem very specific: http://source.roslyn.io/#Microsoft.CodeAnalysis/Diagnostic/WellKnownDiagnosticTags.cs,03ef56ba68ffca36,references, in fact I only understand half of them... Is this maybe a dupe of #24682 just with some predefined tags/defaults that we know how to render? Will there be a future with custom rendering of tags?

@jrieken
Copy link
Member

jrieken commented May 17, 2018

Further questions to seed some disucssion

  • Why do we like TSLint using green squiggles for unused things but don't like that for TypeScript?
  • Do we want DiagnosticSeverity.Hidden to help code actions but do the rendering via some (tbd) semantic highlighting API?

mjbvz added 2 commits May 17, 2018 13:03
Fixes #15710

Adds a new `DiagnosticTag` class that provide additional information about a diagnostic. Introduce `DiagnosticTag.Unnecessary` to mark when a diagnostic is for unused / unnecessary code

The design comes from Rosyln's diagnostic object and allows us to modify how a diagnostic is rendered without changing its serverity.

Hooks up JS and TS to use this new tag. This is controlled by the `javascript.showUnused.enabled` setting which is enabled by default

- Introduce a new diagnostic severity for unused.

    However, using this approach, if a user sets `noUnusedLocals` in their `tsconfig.json`, the resulting diagnostic could only show the squiggly OR be grayed out. Using `customTags` allows us to support both graying out and showing the squiggly

- Custom JS/TS implementation using decorators

    Not themable. We want a standard experience across languages.
- Use numeric enum
@mjbvz mjbvz merged commit 8bb27cd into master May 17, 2018
@mjbvz
Copy link
Collaborator Author

mjbvz commented May 17, 2018

Merging with proposed api using diagnostic tags. We can revisit this to see if we want try implementing this with a semantic highlighting api next iteration

@mjbvz mjbvz deleted the unused-grayout branch July 24, 2018 01:11
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight unused symbols
2 participants