Add type for diagnostics where location is defined #23686
Conversation
ghost
force-pushed
the
diagnosticWithLocation
branch
from
17ac500 to
0301fed
Compare
src/compiler/program.ts
Outdated
| @@ -1300,14 +1300,15 @@ namespace ts { | |||
| const includeBindAndCheckDiagnostics = sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX || | |||
| sourceFile.scriptKind === ScriptKind.External || isCheckJs; | |||
| const bindDiagnostics = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray; | |||
| const checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : emptyArray; | |||
| // TODO: GH#18217 | |||
| const checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) as ReadonlyArray<DiagnosticWithLocation> : emptyArray; | |||
There was a problem hiding this comment.
Here we are asking for diagnostics for a particular file. But getDiagnostics in checker.ts seems to add global diagnostics in some cases. @rbuckton Do you know how this works?
There was a problem hiding this comment.
if you are missing some of the built in types like Array for instance, that is a global diagnostic.
There was a problem hiding this comment.
As @mhegazy said. We add those only when they are first requested.
| if (file && isSourceFileJavaScript(file)) { | ||
| return []; // No declaration diagnostics for js for now | ||
| } | ||
| const compilerOptions = host.getCompilerOptions(); | ||
| const result = transformNodes(resolver, host, compilerOptions, file ? [file] : filter(host.getSourceFiles(), isSourceFileNotJavaScript), [transformDeclarations], /*allowDtsFiles*/ false); | ||
| return result.diagnostics; | ||
| return result.diagnostics as DiagnosticWithLocation[]; // TODO: GH#18217 |
There was a problem hiding this comment.
I think these all should have a location.. can we push that into TransformationResult
There was a problem hiding this comment.
@weswigham can you take a look as well.
There was a problem hiding this comment.
They should all have a location.
There was a problem hiding this comment.
Yeah, every addDiagnostic call uses createDiagnsoticForNode, which will always have a location - the type of addDiagnsotic on TransformationContext and diagnostics on TransformationResult can just be changed.
src/services/codeFixProvider.ts
Outdated
| for (const diag of program.getSemanticDiagnostics(sourceFile).concat(computeSuggestionDiagnostics(sourceFile, program))) { | ||
| if (contains(errorCodes, diag.code)) { | ||
| cb(diag); | ||
| cb(diag as Diagnostic & { file: SourceFile }); |
There was a problem hiding this comment.
Should this cast be DiagnosticWithLocation?
ghost
force-pushed
the
diagnosticWithLocation
branch
from
6d23224 to
7ade816
Compare
| @@ -1403,15 +1403,15 @@ namespace ts { | |||
| // By default, only type-check .ts, .tsx, 'Deferred' and 'External' files (external files are added by plugins) | |||
| const includeBindAndCheckDiagnostics = sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX || | |||
| sourceFile.scriptKind === ScriptKind.External || isCheckJs || sourceFile.scriptKind === ScriptKind.Deferred; | |||
| const bindDiagnostics = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray; | |||
| const bindDiagnostics: ReadonlyArray<Diagnostic> = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray; | |||
There was a problem hiding this comment.
Can you instead use concatenate on the diagnostics instead of using empty array assignment to sub arrays to be concatenated, that way multiple emptyArray concatenate wont create new array with length 0 but use empty array in the end?
There was a problem hiding this comment.
emptyArray is a const and not a function call -- it doesn't create a new empty array, it just refers to a global empty array. So shouldn't be any issue of too many allocations.
There was a problem hiding this comment.
ghost
force-pushed
the
diagnosticWithLocation
branch
from
4f09020 to
919cfc0
Compare
|
@sheetalkamat Good to go? |
Related to #22088 -- this would allow us to access
diag.filewithout anundefinedcheck if it came from the diagnostics of a source file instead of global diagnostics.