-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271
Conversation
This PR issues "cannot find ${name}, did you mean ${name}" errors for identifiers and propery access expressions in JS files *without* `// @ts-check` and without `// @ts-nocheck`. This brings some benefits of Typescript's binder to all Javascript users, even those who haven't opted into Typescript checking. ```js export var inModule = 1 inmodule.toFixed() // errors on exports function f() { var locals = 2 locale.toFixed() // errors on locals } var object = { spaaace: 3 } object.spaaaace // error on read object.spaace = 2 // error on write object.fresh = 12 // OK, no spelling correction to offer ``` To disable the errors, add `// @ts-nocheck` to the file. To get the normal checkJs experience, add `// @ts-check`. == Why This Works == In a word: precision. This change has low recall — it misses lots of correct errors that would be nice to show — but it has high precision: almost all the errors it shows are correct. And they come with a suggested correction. Here are the ingredients: 1. For unchecked JS files, the compiler suppresses all errors except two did-you-mean name resolution errors. 2. Did-you-mean spelling correction is already tuned for high precision/low recall, and doesn't show many bogus errors even in JS. 3. For identifiers, the error is suppressed for suggestions from global files. These are often DOM feature detection, for example. 4. For property accesses, the error is suppressed for suggestions from other files, for the same reason. 5. For property accesses, the error is suppressed for `this` property accesses because the compiler doesn't understand JS constructor functions well enough. In particular, it doesn't understand any inheritance patterns. == Work Remaining == 1. Code cleanup. 2. Fix a couple of failures in existing tests. 3. Suppress errors on property access suggestions from large objects. 4. Combine (3) and (4) above to suppress errors on suggestions from other, global files. 5. A little more testing on random files to make sure that precision is good there too. 6. Have people from the regular Code editor meeting test the code and suggest ideas.
@typescript-bot pack this |
@@ -693,7 +693,7 @@ namespace ts { | |||
const cachedDiagnostics = state.semanticDiagnosticsPerFile.get(path); | |||
// Report the bind and check diagnostics from the cache if we already have those diagnostics present | |||
if (cachedDiagnostics) { | |||
return filterSemanticDiagnotics(cachedDiagnostics, state.compilerOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driveby typo fix
@@ -2059,10 +2059,13 @@ namespace ts { | |||
let suggestion: Symbol | undefined; | |||
if (suggestedNameNotFoundMessage && suggestionCount < maximumSuggestionCount) { | |||
suggestion = getSuggestedSymbolForNonexistentSymbol(originalLocation, name, meaning); | |||
const isGlobalScopeAugmentationDeclaration = suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration); | |||
const isGlobalScopeAugmentationDeclaration = suggestion?.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driveby upgrade to ?.
src/compiler/program.ts
Outdated
const isCheckJsUndefined = includeUncheckedJS | ||
&& sourceFile.checkJsDirective === undefined && options.checkJs === undefined | ||
&& (sourceFile.scriptKind === ScriptKind.JS || sourceFile.scriptKind === ScriptKind.JSX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github does a poor job diffing in this function. isCheckJsUndefined
is the only real addition here, in includeBindAndCheckDiagnostics
, and the new if (isCheckJsUndefined) ...
I also renamed isCheckJs
-> isCheckJsTrue
, which is what confuses the differ.
src/compiler/program.ts
Outdated
@@ -1832,7 +1833,7 @@ namespace ts { | |||
} | |||
|
|||
function getBindAndCheckDiagnostics(sourceFile: SourceFile, cancellationToken?: CancellationToken): readonly Diagnostic[] { | |||
return getBindAndCheckDiagnosticsForFile(sourceFile, cancellationToken); | |||
return getBindAndCheckDiagnosticsForFile(sourceFile, cancellationToken, /*includeUncheckedJS*/ undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two different behavior between LS and command line could also mean that we cant use tsbuildinfo cached semantic diagnostics if and when we start using tsbuildinfo to report errors on closed files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can think of a couple ways to fix this
- Always return all errors, then have
tsc
have a filter when displaying errors for JS files. This would add errors to all other API consumers, though. - Give the language service a separate way to request unchecked JS errors. The checker will have to store these errors separately and the language service will have to add them late.
- Possibly: Not report these errors in the "errors in closed files" list. But I don't know if that would be what users of that feature expect.
Opinions? Ideas?
I'm pretty sure that this feature is going to stick around and grow, so I'd like for it to be easier to use than to not use. Specifically, it's annoying if the future status quo for the language service is to request diagnostics twice, once for unchecked JS and again for everything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give the language service a separate way to request unchecked JS errors. The checker will have to store these errors separately and the language service will have to add them late.
Isn't that what suggestion diagnostics are suppose to be and seems like a good place to add those errors ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are quite similar, and that would work except that the display should be squiggly underlines, like an error, instead of three grey dots, like a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjbvz Would VS Code be OK to add a separate call to a new function getUncheckedJSDiagnostics
with a matching protocol message uncheckedJSDiagnosticsSync
? It would make it easier to create a setting to disable the errors and for Code to style them differently if desired.
Also @uniqueiniquity @jessetrinity you probably have opinions from the VS side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly can add an additional call on the VS side.
I'm not sure I understand why it's inappropriate for tsc
to report these, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uniqueiniquity is right, why should the behavior be different between LS and tsc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think treading lightly might be the way to go - we've never put errors in JS files unless they're specifically parsing errors.
Providing these as suggestion diagnostics keeps these as "quiet" by default. We can then potentially have nightly/insiders editors promote these to green or red squiggles (similar to how they specially-handle unused variable diagnostics) and see if we get any feedback on it. What do you all think? (CC @mjbvz @RyanCavanaugh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why it's inappropriate for tsc to report these, though?
There are two scenarios here. The first is mixed TS/JS, where people have legacy JS as part of the compilation via allowJs. These people are running tsc
regularly, and I don't want to block their build on a bogus error.
@DanielRosenwasser
The other scenario is pure JS users who never run tsc. I believe it's these people that @DanielRosenwasser is thinking with
I do think treading lightly might be the way to go
I agree. Shipping this as a suggestion at first is a good start. Based on the time I added all implicit any errors as suggestions, I think nobody on VS or Code will notice. Then styling them differently is a question of whether TS provides the errors in a separate function or whether the editor filters the suggestion list. I think either is fine but leaving it to the editors is certainly easier for me. =)
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 1b44d72. You can monitor the build here. |
@DanielRosenwasser you'll have to test this locally; I found that the playground always has (It's a great default since it shows off our JS checking, but makes it impossible to test this PR.) |
Instead of selectively letting errors through.
Hi all, this is ready to review again. It now logs all errors as suggestions in unchecked JS files. The filtering in program.ts is gone entirely since suggestions are handled separately from normal errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm much happier from the consumer side for these to just be suggestion diags.
Straw text for the messages, I just changed the modals to avoid name collisions.
After talking to @DanielRosenwasser, I'm going to merge this as-is, with the error messages as slight variants of the originals. As I add errors to plain JS, better error display will become more important — for this PR, errors are hardly visible with Code's default suggestion UI — so we're going to investigate improved type display next. As part of that, I expect to learn how the error messages should change. |
This PR issues "cannot find ${name}, did you mean ${name}" errors for identifiers and propery access expressions in JS files without
// @ts-check
and without// @ts-nocheck
. This brings some benefits of Typescript's binder to all Javascript users, even those who haven't opted into Typescript checking.In this PR, the errors are provided as suggestions. If the suggestions are useful, future work will change the presentation to be more obvious.
To disable the errors, add
// @ts-nocheck
to the file. To get the normal checkJs experience, add// @ts-check
.Why This Works
In a word: precision. This change has low recall — it misses lots of correct errors that would be nice to show — but it has high
precision: almost all the errors it shows are correct. And they come with a suggested correction.
Here are the ingredients:
this
property accesses for the same reason, and because somethis
property accesses in JS are not on classes.Results
I tested on a random sampling of 1000 single files in a fully installed clone of Definitely Typed. I didn't get false positives from these new errors.
I also tested on all the typescript user tests that are written in TS. There, the results informed the above rules. I got the following results
Good
Bad
require
wrapper.setup
function that copies properties from one object to another.Open Questions
checkJs: false
to a tsconfig that doesn't exist.Future Work
I think there are a lot of other errors that we can issue in JS with enough precision. My next step is to find them and test them.
Fixes #41582