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

ghost · 2018-03-06T19:40:57Z

Part of #19392
Fixes #8165 once editors recognize the unused attribute on a diagnostic (@amcasey, @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

mhegazy · 2018-03-06T20:22:43Z

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

mhegazy · 2018-03-06T20:25:50Z

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

amcasey · 2018-03-07T18:12:42Z

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

amcasey

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

…asking for a suggestion

ghost · 2018-03-12T20:45:16Z

@mhegazy Please re-review

DanielRosenwasser · 2018-03-20T22:49:14Z

src/compiler/checker.ts

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


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.

DanielRosenwasser · 2018-03-20T22:55:06Z

src/compiler/types.ts

@@ -4043,6 +4044,8 @@ namespace ts {
        length: number | undefined;
        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?: {};


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

ghost · 2018-03-22T23:00:35Z

@DanielRosenwasser Please re-review

mhegazy · 2018-03-28T23:56:29Z

src/compiler/types.ts

@@ -4061,6 +4062,8 @@ namespace ts {
        length: number | undefined;
        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?: {};


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

@amcasey what does Roslyn call this property?

I think you're asking about WellKnownDiagnosticTags.Unnecessary

a bit weird ... but sure.

mhegazy

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

mhegazy · 2018-04-05T16:18:04Z

Some tests are still failing.

mhegazy · 2018-04-05T19:30:00Z

@andy-ms let's get this change merged.

StanFisher · 2018-05-15T17:47:18Z

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

ghost · 2018-05-15T17:54:55Z

@StanFisher Should be in 2.9.

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

372118d

…ns instead of errors

ghost requested review from amcasey and mhegazy March 6, 2018 19:41

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

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

mjbvz mentioned this pull request Mar 23, 2018

Highlight unused symbols microsoft/vscode#15710

Closed

mhegazy reviewed Mar 28, 2018

View reviewed changes

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

mjbvz mentioned this pull request Apr 9, 2018

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

Closed

aboyton mentioned this pull request Apr 15, 2018

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

Closed

mhegazy mentioned this pull request May 8, 2018

Support max-line-length in organize imports #22991

Open

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 locked and limited conversation to collaborators Jul 31, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Mar 6, 2018 •

edited by ghost

Loading

mhegazy commented Mar 6, 2018

mhegazy commented Mar 6, 2018

amcasey commented Mar 7, 2018

amcasey left a comment

ghost commented Mar 12, 2018

DanielRosenwasser Mar 20, 2018

ghost Mar 23, 2018

DanielRosenwasser Mar 20, 2018

ghost commented Mar 22, 2018

mhegazy Mar 28, 2018

amcasey Mar 29, 2018

mhegazy Mar 29, 2018

mhegazy left a comment

mhegazy commented Apr 5, 2018

mhegazy commented Apr 5, 2018

StanFisher commented May 15, 2018

ghost commented May 15, 2018

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

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

Conversation

ghost commented Mar 6, 2018 • edited by ghost Loading

mhegazy commented Mar 6, 2018

mhegazy commented Mar 6, 2018

amcasey commented Mar 7, 2018

amcasey left a comment

Choose a reason for hiding this comment

ghost commented Mar 12, 2018

DanielRosenwasser Mar 20, 2018

Choose a reason for hiding this comment

ghost Mar 23, 2018

Choose a reason for hiding this comment

DanielRosenwasser Mar 20, 2018

Choose a reason for hiding this comment

ghost commented Mar 22, 2018

mhegazy Mar 28, 2018

Choose a reason for hiding this comment

amcasey Mar 29, 2018

Choose a reason for hiding this comment

mhegazy Mar 29, 2018

Choose a reason for hiding this comment

mhegazy left a comment

Choose a reason for hiding this comment

mhegazy commented Apr 5, 2018

mhegazy commented Apr 5, 2018

StanFisher commented May 15, 2018

ghost commented May 15, 2018

ghost commented Mar 6, 2018 •

edited by ghost

Loading