Plain JS binder errors

[sandersn]

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.

[sandersn]

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

[DanielRosenwasser]

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]

@typescript-bot test this

[typescript-bot]

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.

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

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

[orta]

@typescript-bot pack this

[typescript-bot]

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

[sandersn]

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

[andrewbranch]

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

[sandersn]

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

[andrewbranch]

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

[sandersn]

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

[andrewbranch]

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
• ???

[sandersn]

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

[sandersn]

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)

[sandersn]

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:

1. 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.
2. 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]

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

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]

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]

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

[sandersn]

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]

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

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]

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