Plain JS binder errors #46816

sandersn · 2021-11-15T20:09:35Z

Issue some errors from the binder in JS files that do not have checkJS explicitly turned on or off. These errors mirror runtime checks done by Javascript. The checker has many more errors that also cover runtime checks. I'll start on those next.

There are a few errors in the binder that don't have matching runtime errors: they nearly always indicate incorrect code, but not always.

A label is not allowed here

I decided to include "A label is not allowed here" because, even though it's not a runtime error on its own, if you use the label it will not be defined, which is a runtime error, and because the compiler error for the undefined label is so bad.

An object literal cannot have multiple properties with the same name in strict mode

I decided not to include this error, since it's not a runtime error in ES2015 and above according to an SO answer I found. It's a useful error for TS, though; probably the error message should be changed to read "An object literal should not have multiple properties with the same name.", or else it should be restricted to ES5-only. I filed #46815 to track that.

Addresses but does not fully fix #45349.

Issue select errors from the binder in JS files that do not have checkJS explicitly turned on or off. These errors mirror runtime checks done by Javascript.

sandersn · 2021-11-15T20:10:35Z

@mjbvz You might also have opinions about the appropriateness of these errors.

src/compiler/program.ts

DanielRosenwasser · 2021-11-15T21:19:55Z

I decided to include "A label is not allowed here" because, even though it's not a runtime error on its own, if you use the label it will not be defined, which is a runtime error, and because the compiler error for the undefined label is so bad.

#30408

I decided not to include this error, since it's not a runtime error in ES2015 and above according to an SO answer I found. It's a useful error for TS, though; probably the error message should be changed to read "An object literal should not have multiple properties with the same name.", or else it should be restricted to ES5-only. I filed #46815 to track that.

My 2 cents is to just make it a suggestion diagnostic - don't go too crazy with when and how to report this in JS, or modifying how it surfaces in TS.

DanielRosenwasser · 2021-11-15T21:21:19Z

@typescript-bot test this

typescript-bot · 2021-11-15T21:21:23Z

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 9c85b4b. You can monitor the build here.

amcasey

Seems sensible.

src/compiler/program.ts

DanielRosenwasser · 2021-11-16T01:40:56Z

@typescript-bot pack this

typescript-bot · 2021-11-16T01:40:59Z

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

orta · 2021-11-16T15:58:05Z

@typescript-bot pack this

typescript-bot · 2021-11-16T15:58:28Z

Heya @orta, I've started to run the tarball bundle task on this PR at 9c85b4b. You can monitor the build here.

sandersn · 2021-11-16T16:14:24Z

I'm going to hold this until we have a chance to discuss this in the editor meeting tomorrow.

andrewbranch · 2021-11-16T17:52:45Z

src/compiler/program.ts

+                const isCheckJs = isJs && isCheckJsEnabledForFile(sourceFile, options);
+                const isPlainJs = isJs && !sourceFile.checkJsDirective && options.checkJs === undefined;


What’s the difference between isCheckJsEnabledForFile the property checks you’re doing for isPlainJs?

I almost inlined that function to make this easier to read, except that it's used everywhere. isCheckJsEnabledForFile is true whenever checkJs is explicitly true, whether locally or globally. To put it in the most parallel form:

sourceFile.checkJsDirective?.enabled || options.checkJs === true vs
sourceFile.checkJsDirective === undefined && options.checkJs === undefined

Ah, so the third kind of JS is JS where checkJs is explicitly disabled?

That's right. // @ts-nocheck is the local form of that.

andrewbranch · 2021-11-16T17:55:39Z

src/compiler/program.ts

+                // - check JS: .js files with either // ts-check or checkJs: true
+                // - external: files that are added by plugins
+                const includeBindAndCheckDiagnostics = !isTsNoCheck && (sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX
+                        || sourceFile.scriptKind === ScriptKind.External || isPlainJs || isCheckJs || sourceFile.scriptKind === ScriptKind.Deferred);


isPlainJs || isCheckJs || ...

Related to my other question, but what other kind of JS is there?

It took me a second to realize, it’s easier to think about this condition in terms of what it excludes, although it may or may not be easier to write it that way:

// @ts-nocheck files

JSON files

???

plain JS (no check options anywhere)

checked JS (check turned on locally or globally)

no-checked JS (check turned off locally)

TS

no-checked TS (check turned off locally)

external (from plugins?)

deferred (??)

???

I have no idea what's in ???, that's the problem with inverting the predicate.

src/compiler/program.ts

sandersn · 2021-11-17T23:11:42Z

src/compiler/program.ts

+                const includeBindAndCheckDiagnostics = !isTsNoCheck && (sourceFile.scriptKind === ScriptKind.TS || sourceFile.scriptKind === ScriptKind.TSX
+                        || sourceFile.scriptKind === ScriptKind.External || isPlainJs || isCheckJs || sourceFile.scriptKind === ScriptKind.Deferred);
+                let bindDiagnostics: readonly Diagnostic[] = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray;
+                let checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : emptyArray;


try out some minified or otherwise compiled JS files to make sure perf doesn't tank or crash.
(Code uses line length to disable certain features)

Checker errors require requesting diagnostics, which stll needs to be peformance tested. This commit removes one cross-file duplicate declaration error in the tests.

sandersn · 2021-11-19T17:19:15Z

On a desktop with tsserver logging enabled [1], I created a simple project, then opened a new Typescript file and pasted in some code [2].
I opened ajv.min.js, a single-line 121k file, and switched back to my test file. Goto-def was responsive after a second or so.
Then I opened Typescript's own run.js that we use to run tests, a 14 MB file, and switched back to my test file. Goto-def was responsive after 2-3 seconds.

In both cases, as soon goto-def was responsive, the rest of the language service was responsive.
Code notified me that it was not tokenising ajv.min.js, and there was no syntax highlighting.
It took considerably longer to get quick info from run.js itself when I switched back to it. Probably another 10 seconds?

Based on this:

I think it's fine to proceed with the simple approach of requesting semantic diagnostics for plain JS files. If a particularly slow use case turns up in the beta period, we'll have a couple of ways to fix it.
I'll try to find out why grammar errors are in the checker and see how hard it would be to move them earlier. They make up about 20% of the errors in the checker, and most of the value for plain JS is probably in the grammar errors.

[1] So, faster because it's a desktop; slower because logging is on.
[2] Just some random code I worked on recently.

export class C {
    set x(...n) { }
}
declare var x: Promise<number>
await x
const y = await x

class Test {
    foo() {

    }

    bar() {
        this.blargh(this, "completely wrong") // Can't autocomplete second parameter

        const t = new Test()
        this.blargh(t, "bar"); // Second parameter autocompletes to "foo", "blargh", "bar"
    }

    blargh<T>(a: T, k: keyof T) {
        // some implementation
    }
}

const t = new Test()
t.blargh(t, "bar")
t.blargh(t, "hi")

amcasey · 2021-11-19T17:30:34Z

@sandersn How long did it take for GTD to become usable without the diagnostics? (Sorry if I just missed that in your write-up.)

If we think we might have to roll this back (contextually?), would it be useful to have a configuration option so that editors can add an escape hatch?

sandersn · 2021-11-19T18:03:20Z

Good question. After opening ajv.min.js, a lot less than a second. After opening run.js, around 2 seconds -- there probably was a difference from the semantics-requesting version, but not one that I could measure without a stopwatch.

DanielRosenwasser · 2021-11-19T18:48:22Z

If a particularly slow use case turns up in the beta period, we'll have a couple of ways to fix it.

How do you expect most JavaScript users, who don't opt into nightly/beta versions of TypeScript in their editor, to provide feedback during the beta period?

amcasey · 2021-11-19T20:40:47Z

I think @andrewbranch looked into ways to monitor completion performance via telemetry when he was working on auto-imports. Maybe that would help?

sandersn · 2021-12-06T18:36:51Z

Now that I think about it, getSuggestionDiagnostics is called on plain JS files, and it checks the entire file as well. Calling getDiagnostics shouldn't check the file twice, so calling it shouldn't add much time at all.

@mjbvz Can you confirm how much getSuggestionDiagnostics is called on plain JS files compared to getDiagnostics on TS files? I poked around vscode's typescript code but couldn't figure it out. From usage, it seems like suggestions show up on plain JS files at the same rate that errors do on TS files.

mjbvz · 2021-12-07T00:00:38Z

@sandersn VS Code uses the geterr TS Server command to get all the errors for a file instead of requesting each type of diagnostic individually. Is that what you were asking about?

sandersn · 2021-12-07T00:02:16Z

Pretty much--I want to know whether it's calling geterr on JS files as often as it is on TS files. I suspect the answer is yes, in order for it to be able to show suggestions in JS files.

mjbvz · 2021-12-07T00:06:36Z

Yes we should have the same code path for both JS and TS on the VS Code side

* Plain JS binder errors Issue select errors from the binder in JS files that do not have checkJS explicitly turned on or off. These errors mirror runtime checks done by Javascript. * Rest of plain JS binder errors * address PR comments * Only issue binder errors in plain JS. Checker errors require requesting diagnostics, which stll needs to be peformance tested. This commit removes one cross-file duplicate declaration error in the tests. * fix const lint

sandersn added 2 commits November 12, 2021 13:27

Plain JS binder errors

0f619db

Issue select errors from the binder in JS files that do not have checkJS explicitly turned on or off. These errors mirror runtime checks done by Javascript.

Rest of plain JS binder errors

9c85b4b

typescript-bot assigned sandersn Nov 15, 2021

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 15, 2021

sandersn requested review from andrewbranch, amcasey and DanielRosenwasser November 15, 2021 20:10

typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 15, 2021

sandersn commented Nov 15, 2021

View reviewed changes

src/compiler/program.ts Show resolved Hide resolved

amcasey approved these changes Nov 16, 2021

View reviewed changes

src/compiler/program.ts Show resolved Hide resolved

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

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

src/compiler/program.ts Show resolved Hide resolved

DanielRosenwasser reviewed Nov 16, 2021

View reviewed changes

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

address PR comments

6a0fe21

Merge branch 'main' into plain-js-binder-errors

c6e9f3d

andrewbranch reviewed Nov 16, 2021

View reviewed changes

sandersn commented Nov 17, 2021

View reviewed changes

src/compiler/program.ts Show resolved Hide resolved

sandersn commented Nov 17, 2021

View reviewed changes

sandersn added 2 commits November 18, 2021 13:56

Only issue binder errors in plain JS.

2f9f77e

Checker errors require requesting diagnostics, which stll needs to be peformance tested. This commit removes one cross-file duplicate declaration error in the tests.

fix const lint

4824d64

sandersn merged commit 868c275 into main Nov 19, 2021

sandersn deleted the plain-js-binder-errors branch November 19, 2021 01:13

sandersn mentioned this pull request Dec 8, 2021

Plain JS grammar errors #47067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plain JS binder errors #46816

Plain JS binder errors #46816

sandersn commented Nov 15, 2021 •

edited

sandersn commented Nov 15, 2021

DanielRosenwasser commented Nov 15, 2021 •

edited

DanielRosenwasser commented Nov 15, 2021

typescript-bot commented Nov 15, 2021 •

edited

amcasey left a comment

DanielRosenwasser commented Nov 16, 2021

typescript-bot commented Nov 16, 2021 •

edited

orta commented Nov 16, 2021

typescript-bot commented Nov 16, 2021 •

edited

sandersn commented Nov 16, 2021

andrewbranch Nov 16, 2021

sandersn Nov 16, 2021

andrewbranch Nov 16, 2021

sandersn Nov 18, 2021

andrewbranch Nov 16, 2021

sandersn Nov 16, 2021

sandersn Nov 17, 2021

sandersn commented Nov 19, 2021

amcasey commented Nov 19, 2021

sandersn commented Nov 19, 2021

DanielRosenwasser commented Nov 19, 2021

amcasey commented Nov 19, 2021

sandersn commented Dec 6, 2021

mjbvz commented Dec 7, 2021

sandersn commented Dec 7, 2021

mjbvz commented Dec 7, 2021

		const isCheckJs = isJs && isCheckJsEnabledForFile(sourceFile, options);
		const isPlainJs = isJs && !sourceFile.checkJsDirective && options.checkJs === undefined;

Plain JS binder errors #46816

Plain JS binder errors #46816

Conversation

sandersn commented Nov 15, 2021 • edited

A label is not allowed here

An object literal cannot have multiple properties with the same name in strict mode

sandersn commented Nov 15, 2021

DanielRosenwasser commented Nov 15, 2021 • edited

DanielRosenwasser commented Nov 15, 2021

typescript-bot commented Nov 15, 2021 • edited

amcasey left a comment

Choose a reason for hiding this comment

DanielRosenwasser commented Nov 16, 2021

typescript-bot commented Nov 16, 2021 • edited

orta commented Nov 16, 2021

typescript-bot commented Nov 16, 2021 • edited

sandersn commented Nov 16, 2021

andrewbranch Nov 16, 2021

Choose a reason for hiding this comment

sandersn Nov 16, 2021

Choose a reason for hiding this comment

andrewbranch Nov 16, 2021

Choose a reason for hiding this comment

sandersn Nov 18, 2021

Choose a reason for hiding this comment

andrewbranch Nov 16, 2021

Choose a reason for hiding this comment

sandersn Nov 16, 2021

Choose a reason for hiding this comment

sandersn Nov 17, 2021

Choose a reason for hiding this comment

sandersn commented Nov 19, 2021

amcasey commented Nov 19, 2021

sandersn commented Nov 19, 2021

DanielRosenwasser commented Nov 19, 2021

amcasey commented Nov 19, 2021

sandersn commented Dec 6, 2021

mjbvz commented Dec 7, 2021

sandersn commented Dec 7, 2021

mjbvz commented Dec 7, 2021

sandersn commented Nov 15, 2021 •

edited

DanielRosenwasser commented Nov 15, 2021 •

edited

typescript-bot commented Nov 15, 2021 •

edited

typescript-bot commented Nov 16, 2021 •

edited

typescript-bot commented Nov 16, 2021 •

edited