When --noUnusedLocals/--noUnusedParameters is disabled, add suggestions instead of errors · Pull Request #22361 · microsoft/TypeScript · GitHub

Part of #19392
Fixes #8165 once editors recognize the unused attribute on a diagnostic (Andrew Casey (@amcasey), Matt Bierner (@mjbvz))

We will now always create these diagnostics, but the list we add them to varies based on compiler options.
Also had to change some codeFix tests to use all of their variables to avoid getting extra fixes.

…ns instead of errors

Andy (Andrewkraft) (@Andy-MS) can you run the perf tests as well.. i am concerned about the time and memory we spend building these error messages. one option is to have an additional flag to the checker that the LS sets, but not the command-line compiler, specially that we only need it for one file (open) and not the rest of the program that we will ignore the results of anyways.

Also we need to mark the unused name diagnostic with a new flag to enable the VS tagging behavior.

I think that, to create a good user experience in the editor, we'll want to do more than just report the same diagnostics with a lower severity.

As Mohamed Hegazy (@mhegazy) suggested, we'll want to have a flag on the diagnostics so that the editor can gray out, rather than squiggle the range.
If all symbols declared by a given import are unused, we'll want to combine the diagnostics into a single diagnostic spanning the entire import.
We'll generally want to report larger spans (than just the declared identifier) to make the grayed out regions more visible (e.g. "class C", rather than just "class").

I don't object to checking this in as-is, but I do think there's more work to do.

…asking for a suggestion

Mohamed Hegazy (@mhegazy) Please re-review

So is the idea that getting suggestion diagnostics always triggers type-checking the file? You could make this slightly smarter by only checking the source file if the file itself is a declaration file.

Done

I'm not big on the name here; try reportingUnused, reportsUnused, or describesUnusedSpan

Daniel Rosenwasser (@DanielRosenwasser) Please re-review

nit. why not call both this property and the one on DiagnosticMessage isUnused or isUnusedDeclaration

Andrew Casey (@amcasey) what does Roslyn call this property?

I think you're asking about WellKnownDiagnosticTags.Unnecessary

a bit weird ... but sure.

👍 Please check with Andrew Casey (@amcasey) about the name of the property. ideally we would have the same name as Roslyn here.

Some tests are still failing.

Andy (Andrewkraft) (@Andy-MS) let's get this change merged.

Mohamed Hegazy (@mhegazy) In which version of TypeScript will this land? Is there a good way to determine this?

Stan Fisher (@StanFisher) Should be in 2.9.

When --noUnusedLocals/--noUnusedParameters is disabled, add suggestio…

372118d

…ns instead of errors

ghost requested review from Andrew Casey (amcasey) and Mohamed Hegazy (mhegazy) March 6, 2018 19:41

Andrew Casey (amcasey) approved these changes Mar 7, 2018

View reviewed changes

This was referenced Mar 7, 2018

When every import is unused, error on the entire import declaration #22386

Merged

Increase size of span for unused declaration #22388

Merged

Merge branch 'master' into unused_suggestion

3a04ac0

This was referenced Mar 12, 2018

Block statement in arrow function is checked twice #22491

Closed

Error replacing statement that shares line with another statement #22492

Closed

Improve performance: do not add unused suggestion diagnostics unless …

90313d2

…asking for a suggestion

ghost force-pushed the unused_suggestion branch from a07f9f4 to 90313d2 Compare March 12, 2018 19:28

ghost force-pushed the unused_suggestion branch from 013840f to 91033ec Compare March 12, 2018 21:13

Add "unused" flag to diagnostics

8cae633

ghost force-pushed the unused_suggestion branch from 91033ec to 8cae633 Compare March 12, 2018 21:25

ghost mentioned this pull request Mar 13, 2018

convertToEs6Module: Avoid replacing entire function #22507

Merged

Merge branch 'master' into unused_suggestion

ba9cc5b

Daniel Rosenwasser (DanielRosenwasser) requested changes Mar 20, 2018

View reviewed changes

Merge branch 'master' into unused_suggestion

4dc9423

ghost force-pushed the unused_suggestion branch from e4f380c to bb4332b Compare March 22, 2018 21:53

Code review

a724cd9

ghost force-pushed the unused_suggestion branch from bb4332b to a724cd9 Compare March 22, 2018 22:17

Merge branch 'master' into unused_suggestion

ede51a1

Matt Bierner (mjbvz) mentioned this pull request Mar 23, 2018

Highlight unused symbols microsoft/vscode#15710

Closed

Mohamed Hegazy (mhegazy) reviewed Mar 28, 2018

View reviewed changes

Mohamed Hegazy (mhegazy) approved these changes Mar 29, 2018

View reviewed changes

Andy Hanson added 3 commits March 29, 2018 12:38

reportsUnused -> reportsUnnecessary

1fde07a

Merge branch 'master' into unused_suggestion

d2956ef

Merge branch 'master' into unused_suggestion

3b8a87b

Fix test

0af1025

ghost merged commit 24842b4 into master Apr 5, 2018

ghost deleted the unused_suggestion branch April 5, 2018 19:33

Matt Bierner (mjbvz) mentioned this pull request Apr 9, 2018

Add flag to mark TS Server unused variable suggestion diagnostics #23288

Closed

Andrew Boyton (aboyton) mentioned this pull request Apr 15, 2018

Confused by no-undef errors on type declarations bradzacher/eslint-plugin-typescript#110

Closed

Mohamed Hegazy (mhegazy) mentioned this pull request May 8, 2018

Support max-line-length in organize imports #22991

Open

Nikolay Yakimov (lierdakil) mentioned this pull request Jun 19, 2018

noUnusedParameters always enabled by default when using typescript from node_modules TypeStrong/atom-typescript#1433

Closed

Microsoft (microsoft) locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

+                          getSuggestionDiagnostics: file => {
+                              return (suggestionDiagnostics.get(file.fileName) || emptyArray).concat(getUnusedDiagnostics());
+                              function getUnusedDiagnostics(): ReadonlyArray<Diagnostic> {
+                                  checkSourceFile(file);

                       messageText: string | DiagnosticMessageChain;
                       category: DiagnosticCategory;
+                      /** May store more in future. For now, this will simply be `true` to indicate when a diagnostic is an unused-identifier diagnostic. */
+                      unused?: {};

                       messageText: string | DiagnosticMessageChain;
                       category: DiagnosticCategory;
+                      /** May store more in future. For now, this will simply be `true` to indicate when a diagnostic is an unused-identifier diagnostic. */
+                      reportsUnused?: {};

When --noUnusedLocals/--noUnusedParameters is disabled, add suggestions instead of errors · Pull Request #22361 · microsoft/TypeScript · GitHub

Uh oh!

When --noUnusedLocals/--noUnusedParameters is disabled, add suggestions instead of errors - #22361

When --noUnusedLocals/--noUnusedParameters is disabled, add suggestions instead of errors#2236112 commits merged intomastermicrosoft/TypeScript:masterfrom unused_suggestionmicrosoft/TypeScript:unused_suggestionCopy head branch name to clipboard

Conversation

Deleted user (ghost) commented Mar 6, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mohamed Hegazy (mhegazy) commented Mar 6, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented Mar 6, 2018

Uh oh!

Andrew Casey (amcasey) commented Mar 7, 2018

Uh oh!

Andrew Casey (amcasey) left a comment

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Mar 12, 2018

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Mar 22, 2018

Uh oh!

Mohamed Hegazy (mhegazy) Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) Mar 29, 2018

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) Mar 29, 2018

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) left a comment

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Apr 5, 2018

Uh oh!

Mohamed Hegazy (mhegazy) commented Apr 5, 2018

Uh oh!

Stan Fisher (StanFisher) commented May 15, 2018

Uh oh!

Deleted user (ghost) commented May 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

When --noUnusedLocals/--noUnusedParameters is disabled, add suggestions instead of errors#22361
12 commits merged into
masterfrom
unused_suggestion

Deleted user (ghost) commented Mar 6, 2018 •
edited by ghost
Loading