Conversation
…ns instead of errors
@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.
|
…asking for a suggestion
a07f9f4 to 90313d2 Compare @mhegazy Please re-review |
013840f to 91033ec Compare 91033ec to 8cae633 Compare src/compiler/checker.ts Outdated
| getSuggestionDiagnostics: file => { | ||
| return (suggestionDiagnostics.get(file.fileName) || emptyArray).concat(getUnusedDiagnostics()); | ||
| function getUnusedDiagnostics(): ReadonlyArray<Diagnostic> { | ||
| checkSourceFile(file); |
There was a problem hiding this comment.
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.
src/compiler/types.ts Outdated
| @@ -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?: {}; | |||
There was a problem hiding this comment.
I'm not big on the name here; try reportingUnused, reportsUnused, or describesUnusedSpan
e4f380c to bb4332b Compare bb4332b to a724cd9 Compare @DanielRosenwasser Please re-review |
src/compiler/types.ts Outdated
| @@ -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?: {}; | |||
There was a problem hiding this comment.
nit. why not call both this property and the one on DiagnosticMessage isUnused or isUnusedDeclaration
@amcasey what does Roslyn call this property?
There was a problem hiding this comment.
I think you're asking about WellKnownDiagnosticTags.Unnecessary
There was a problem hiding this comment.
👍 Please check with @amcasey about the name of the property. ideally we would have the same name as Roslyn here.
Some tests are still failing. |
@Andy-MS let's get this change merged. |
@mhegazy In which version of TypeScript will this land? Is there a good way to determine this? |
@StanFisher Should be in 2.9. |
Part of #19392
Fixes #8165 once editors recognize the
unusedattribute 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.