Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271

sandersn · 2021-05-26T17:40:44Z

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.

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:

For unchecked JS files, the compiler suppresses all errors except two did-you-mean name resolution errors.
Did-you-mean spelling correction is already tuned for high precision/low recall, and doesn't show many bogus errors even in JS.
The error is suppressed for suggestions from global files that are not the current file. These are often DOM feature detection, for example.
The error is suppressed for property accesses on a class because the compiler doesn't understand JS constructor functions well enough. In particular, it doesn't understand any inheritance patterns.
The error is suppressed for this property accesses for the same reason, and because some this 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

minimatch: Found a 9-year-old bug in untested code that has never worked.

Bad

grunt: Didn't understand grunt's convenience require wrapper.
debug: Didn't produce an index signature for a loop in a setup function that copies properties from one object to another.
Uglify-JS: Didn't understand post-initialisation addition of properties to a very large object — because the object was large, the compiler suggested a similar name already in it.
chrome-devtools-frontend: Didn't understand lazy initialization of properties in several classes. The access was '_property', the suggestion was 'property'.

Open Questions

I'm not sure of the best way to indicate that getSemanticDiagnostics should return errors for unchecked JS files. I threaded an optional boolean parameter through the interface for now.
The wording for the error might need to be customised to point new users to the codefix.
VS, for example, doesn't use tsconfig as much and may need a user preference to disable these errors, since it won't be easy to add 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

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.

sandersn · 2021-05-26T17:41:04Z

@typescript-bot pack this

typescript-bot · 2021-05-26T17:41:07Z

Heya @sandersn, I've started to run the tarball bundle task on this PR at 141de3f. You can monitor the build here.

sandersn · 2021-06-02T18:25:43Z

src/compiler/builder.ts

@@ -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);


driveby typo fix

sandersn · 2021-06-02T18:25:54Z

src/compiler/checker.ts

@@ -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);


driveby upgrade to ?.

sandersn · 2021-06-02T18:30:33Z

src/compiler/program.ts

+                const isCheckJsUndefined = includeUncheckedJS
+                    && sourceFile.checkJsDirective === undefined && options.checkJs === undefined
+                    && (sourceFile.scriptKind === ScriptKind.JS || sourceFile.scriptKind === ScriptKind.JSX);


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.

sheetalkamat · 2021-06-02T19:00:55Z

src/compiler/program.ts

@@ -1832,7 +1833,7 @@ namespace ts {
        }

        function getBindAndCheckDiagnostics(sourceFile: SourceFile, cancellationToken?: CancellationToken): readonly Diagnostic[] {
-            return getBindAndCheckDiagnosticsForFile(sourceFile, cancellationToken);
+            return getBindAndCheckDiagnosticsForFile(sourceFile, cancellationToken, /*includeUncheckedJS*/ undefined);


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.

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.

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 ?

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.

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

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?

@uniqueiniquity is right, why should the behavior be different between LS and tsc ?

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)

@uniqueiniquity @sheetalkamat

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. =)

DanielRosenwasser · 2021-06-02T22:38:17Z

@typescript-bot pack this

typescript-bot · 2021-06-02T22:38:21Z

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 1b44d72. You can monitor the build here.

src/compiler/program.ts

sandersn · 2021-06-03T16:24:35Z

@DanielRosenwasser you'll have to test this locally; I found that the playground always has checkJs: true for JS files,

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

sandersn · 2021-06-08T20:43:09Z

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.

uniqueiniquity

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.

sandersn · 2021-06-15T15:30:57Z

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.

typescript-bot assigned sandersn May 26, 2021

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 26, 2021

sandersn added 10 commits May 28, 2021 12:01

all (most?) tests pass

ac5834a

NOW they all pass

0384117

add tonnes of semi-colons

2f5ba2b

restore this.x check+add a test case

bb589a4

make ts-ignore/no-check codefix work in unchecked js

37ec450

Issues errors only in the language service

60fc7bb

Merge branch 'main' into always-issue-cannot-find-name-dym-error

85e4b07

add a few more tests

70ce293

fix incorrect parentheses

77667b7

More cleanup in program.ts

38d806d

sandersn requested review from DanielRosenwasser, jessetrinity and uniqueiniquity June 2, 2021 18:23

sandersn commented Jun 2, 2021

View reviewed changes

sheetalkamat reviewed Jun 2, 2021

View reviewed changes

Improve readability of isExcludedJSError

1b44d72

uniqueiniquity reviewed Jun 2, 2021

View reviewed changes

src/compiler/program.ts Outdated Show resolved Hide resolved

sandersn added 4 commits June 3, 2021 09:40

make diff in program.ts smaller via closure

73451cd

Merge branch 'main' into always-issue-cannot-find-name-dym-error

657d872

Switch unchecked JS did-you-mean to suggestion

3ceffd1

Instead of selectively letting errors through.

undo more missed changes

1163b78

disallow ignoring suggestions

99ab915

sandersn changed the title ~~Always issue cannot find name did-you-mean error~~ Issue "Cannot find name did-you-mean" errors as suggestions in unchecked JS Jun 8, 2021

uniqueiniquity approved these changes Jun 8, 2021

View reviewed changes

Issue different messages for plain JS than others

c463a00

Straw text for the messages, I just changed the modals to avoid name collisions.

sandersn changed the title ~~Issue "Cannot find name did-you-mean" errors as suggestions in unchecked JS~~ Issue "Cannot find name did-you-mean" errors as suggestions in plain JS Jun 15, 2021

sandersn merged commit e53f19f into main Jun 15, 2021

sandersn deleted the always-issue-cannot-find-name-dym-error branch June 15, 2021 16:00

DanielRosenwasser mentioned this pull request Aug 16, 2021

No suggestion diagnostic for unchecked JS files in method from class #45479

Closed

sandersn mentioned this pull request Sep 30, 2021

TS2339 refactoring warnings (three gray dots) appearing under javascript properties #46103

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271

Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271

sandersn commented May 26, 2021 •

edited

Loading

sandersn commented May 26, 2021

typescript-bot commented May 26, 2021 •

edited

Loading

sandersn Jun 2, 2021

sandersn Jun 2, 2021

sandersn Jun 2, 2021

sheetalkamat Jun 2, 2021

sandersn Jun 2, 2021

sheetalkamat Jun 2, 2021

sandersn Jun 2, 2021

sandersn Jun 3, 2021

uniqueiniquity Jun 3, 2021

sheetalkamat Jun 3, 2021 •

edited

Loading

DanielRosenwasser Jun 3, 2021

sandersn Jun 3, 2021

DanielRosenwasser commented Jun 2, 2021

typescript-bot commented Jun 2, 2021 •

edited

Loading

sandersn commented Jun 3, 2021

sandersn commented Jun 8, 2021 •

edited

Loading

uniqueiniquity left a comment

sandersn commented Jun 15, 2021

Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271

Issue "Cannot find name did-you-mean" errors as suggestions in plain JS #44271

Conversation

sandersn commented May 26, 2021 • edited Loading

Why This Works

Results

Good

Bad

Open Questions

Future Work

sandersn commented May 26, 2021

typescript-bot commented May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheetalkamat Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Jun 2, 2021

typescript-bot commented Jun 2, 2021 • edited Loading

sandersn commented Jun 3, 2021

sandersn commented Jun 8, 2021 • edited Loading

uniqueiniquity left a comment

Choose a reason for hiding this comment

sandersn commented Jun 15, 2021

sandersn commented May 26, 2021 •

edited

Loading

typescript-bot commented May 26, 2021 •

edited

Loading

sheetalkamat Jun 3, 2021 •

edited

Loading

typescript-bot commented Jun 2, 2021 •

edited

Loading

sandersn commented Jun 8, 2021 •

edited

Loading