Fix #15200: Query for semantic errors on .js files with '@ts-check' with no config file#15260
Conversation
| return runWithCancellationToken(() => { | ||
| // If skipLibCheck is enabled, skip reporting errors if file is a declaration file. | ||
| // If skipDefaultLibCheck is enabled, skip reporting errors if file contains a | ||
| // '/// <reference no-default-lib="true"/>' directive. |
There was a problem hiding this comment.
This comment isn't making much sense to me as written. If you have no-default-lib set to true, then I assume there is no default lib to check - in which case, what difference does skipDefaultLibCheck make?
There was a problem hiding this comment.
no-default-lib is just a triple-slash directive. it can exist any where.
We have two flags, for historic reasons:
- skipDefaultLibCheck, only skips the checking of files that have
/// <reference no-default-lib="true"/>in them. This flag is now deprecated, but we still honor it. - skipLibCheck, skips checking of all .d.ts files
| const scriptInfo = project.getScriptInfoForNormalizedPath(file); | ||
| return scriptInfo && !scriptInfo.isJavaScript(); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
So this always returns false for configured projects (as the above if is only entered for inferred/external projects)?
There was a problem hiding this comment.
yes. program.getSemanticDiagnostics should do the right thing for configured projects.
There was a problem hiding this comment.
OK. Again, maybe clarify in comments in the function then. It seems this only returns true for a narrow subset of files that may actually have semantic checking skipped eventually.
| // For inferred (e.g. miscellaneous context in VS) and external projects (e.g. VS .csproj project) with only JS files | ||
| // semantic errors in .d.ts files (e.g. ones added by automatic type acquisition) are not interesting. | ||
| // We want to avoid the cost of querying for these errors all together, even if 'skipLibCheck' is not set. | ||
| // We still want to check .js files (e.g. '// @ts-check' or '--checkJs' is set). |
There was a problem hiding this comment.
This code always returns false for JavaScript files (due to && !scriptInfo.isJavaScript()), so the comment about @ts-check / --checkJs is confusing. Where do these come into play then, if not taked into account in this method?
There was a problem hiding this comment.
we do not know here (in the server) if the .js file should be checked or not. we know that later on by inspecting the SourceFile object in the program. so what we are doing here is we are deferring the check to program.getSemanticDiagnostics
There was a problem hiding this comment.
Maybe a comment to that effect in this function then? It's confusing to have a function called shouldSkipSemanticCheck that takes a file, that doesn't actually check if it should skip the semantic check for that file 😕
| // If skipLibCheck is enabled, skip reporting errors if file is a declaration file. | ||
| // If skipDefaultLibCheck is enabled, skip reporting errors if file contains a | ||
| // '/// <reference no-default-lib="true"/>' directive. | ||
| if (options.skipLibCheck && sourceFile.isDeclarationFile || options.skipDefaultLibCheck && sourceFile.hasNoDefaultLib) { |
There was a problem hiding this comment.
Why do we have this check in two places ? I see similar check in checkSourceFileWorker in checker.ts. Do we need to remove that check?
There was a problem hiding this comment.
yes, will get that fixed.
There was a problem hiding this comment.
I remember now why i did not do that, we call TypeChecker.getDiagnostics from the tests directly.
tsserver checks before calling
getSemanticDiagnosticsif this is a JS-only project first to avoid paying the cost of a semantic check on .d.ts files and second to avoid reporting errors that users do not care about (e.f. in .d.ts files auto-injected by ATA).The check does not make sense in the presence of
// @ts-checksince every .js file now is a fair target for calling getSemanitcDiagnostics.This change has two parts:
program.getSemanticDiagnosticsinstead of cutting it too early in the server, and instead make sure the program does not return bind diagnostics for .d.ts files if check diagnostics are not reported (i.e.--skipLibCheckor--skipDefaultLibCheckare set).Fixes #15200
@billti can you please review this change. this touches on a change you have added recently.