microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.93k stars 12.47k forks source link

Provide Grammar Errors for JavaScript Files #45349

Open DanielRosenwasser opened 3 years ago

DanielRosenwasser commented 3 years ago

Recently, we started reporting high-confidence errors for typos as suggestions in the language service. We believe that there is a set of grammar issues that are worth erroring on as well that are not reported today (e.g. duplicate block-scoped variables, extending multiple classes, rest parameters occurring before regular parameters, etc.)

Open questions:

sandersn commented 3 years ago

Here are my rough notes, cleaned up a little. This is only the binder errors.

Runtime Error

These errors are all errors in JS, at least in V8. Mostly they only apply in strict contexts: .mjs files and class bodies. Actually, the errors are good enough that they should use the same rule as for TS: any JS file that uses ES exports counts as a real ES module and is in strict mode. (Even though .js files are usually commonjs at present.)

These errors should always be provided to JS files.

Diagnostics.Cannot_redeclare_block_scoped_variable_0

const x = 1
var x = 1

Diagnostics.A_module_cannot_have_multiple_default_exports

also:

export default 12;
export default 13;

Diagnostics.Identifier_expected_0_is_a_reserved_word_at_the_top_level_of_a_module

export default 12;
const await = 1

Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode_Modules_are_automatically_in_strict_mode

const yield = 2
export default 12;

Diagnostics.Identifier_expected_0_is_a_reserved_word_that_cannot_be_used_here

async function f() {
  const await = 1
}

Diagnostics.Identifier_expected_0_is_a_reserved_word_that_cannot_be_used_here

function* f() {
  const yield = 1
}

Diagnostics.constructor_is_a_reserved_word

class C {
    #constructor = 1
}

Diagnostics.delete_cannot_be_called_on_an_identifier_in_strict_mode

class C {
    m() {
        function container(f) {
            delete f
        }
        var g = 1
        delete g
        delete container
    }
}

Diagnostics.Invalid_use_of_0_Class_definitions_are_automatically_in_strict_mode

class C {
    m() {
        const eval = 1
        const arguments = 2
    }
    g() {
    }
}

Diagnostics.Invalid_use_of_0_Modules_are_automatically_in_strict_mode

export default 13;
const eval = 1
const arguments = 2

Diagnostics.Invalid_use_of_0_in_strict_mode

"use strict" // this doesn't actually turn strict on or off
const eval = 1
const arguments = 2

Diagnostics.Octal_literals_are_not_allowed_in_strict_mode

class C {
    m() {
      return 010
    }
}

Diagnostics.with_statements_are_not_allowed_in_strict_mode

class C {
    m() {
      const n = 010
      with (n) {
        return toFixed()
      }
    }
}

Likely wrong

These errors indicate code that is likely wrong, but won't error.

I'm not sure whether to show these errors in plain JS files.

class C {
  m() {
    return { a: 1, a: 1 }
  }
}

(I'm not sure what 'strict mode' refers to here; the above test works fine in node for .js and .mjs) From a JS perspective, this feels linty, so I don't think I'll include it.

    class C {
        m() {
            label: var x = 1
            break label
        }
    }

There are two reasons I can see for this error:

  1. Intended object literal -- although it's only issued before declarations like var, function, etc, which are not valid property names.
  2. You misunderstand the intent of labels, and try to use the label later.

I still think (1) is possible because there are surely some declarations that start with contextual keywords. And our error for (2) is abysmally misleading, even though when you run such a program, you'll get a clear runtime error. So I'm going to include this error, even though I'm going to skip the "Unused label" that applies to valid label locations.

Harmless/Could be intended

These errors indicate code that could be written by reasonable people trapped by circumstance into being careless.

Probably shouldn't show these errors in JS.

Sir Not-Appearing-in-this-Language

These errors only apply to Typescript constructs or Typescript emit.

sandersn commented 3 years ago

@orta points out that it would be extremely useful to show

Diagnostics.Cannot_assign_to_0_because_it_is_a_constant

in JS when you try to += 'foo' to a const string, for example.

DanielRosenwasser commented 3 years ago

In the same vein, used-before-defined would also be helpful for let``/const variables

sandersn commented 2 years ago

Here are all the relevant errors from the checker that are called from grammarErrorOnNode. This is not all the grammar errors, but it's a convenient way to bucket them. The checker errors seem a lot easier to distinguish between relevant and irrelevant, but maybe that's because I've looked at enough errors in a row now. There are 67 errors on this list, out of 184 grammarErrorOnNode calls.

class C {
    q = #unbound
    m() {
        #p++
        q++
        this.#q
        if (#po in this) {

        }
        const o = { a: 1, a: 2 }
        return o.a
    }
    #m() {
         this.#m = () => {}
    }
}
#unrelated

These 3 errors need improvement, but at least the first two should behave the same in JS as TS

class C {
    #m() {
         this.#m = () => {}
    }
}
var x = 1 || 2 ?? 2
var x = 2 ?? 3 || 4
class C {
    static {
        for await (const x of [1,2,3]) {
            console.log(x)
        }
    }
}
var b = true
switch (b) {
    case false:
        console.log('no')
    default:
        console.log('yes')
    default:
        console.log('wat')
}
label: for (const x in [1,2,3]) {
    label: for (const y in [1,2,3]) {
        break label;
    }
}
try {
    throw 2
}
catch (e) {
    const e = 1
    console.log(e)
}
declare var deco: any
@deco
class C {
    static #p = 1
}

But this doesn't run on any JS runtime today so it probably shouldn't apply

 class C {
    const x = 1
    const y() {
        return 12
    }
}
class C {
    static static x = 1
}
class C {
    static async x (){ }
}
export static var x = 1
function f(static x = 1) { return x }
export export var x = 1
async export function f(x = 1) { return x }
class C {
    export p = 1
    export m() {
    }
}

Note that there's an existing, bogus error in JS which is supposed to be issued on exports from namespaces, which are not in JS of course.

function f(export x = 1) { return x }
default export 13;
async async function f() {
    return 1
}
function f(async x = 1) { return x }
class C {
    static constructor() { }
}
class C {
    async constructor() { }
}
async class C {
    async x = 1
}
async const y = 2
async import './other'
async export { C }
function f (...x, y) {
}
function f (...x = [1,2,3]) {
}
const arr = x
  => x + 1
var a = [1,2]
a?.`length`;
const o = {
    [console.log('oh no'),2]: 'hi'
}
const o = {
    x?: 12
}

But: this already errors with a JS-specific error, so maybe the first should too.

const o = {
    m?() { return 12 }
}
var x = 1
const o = {
    x!
}
const o = {
    m!() { return 12 }
}
const o = {
    x: 1, y: 2
}
let x; let y
;({ ...[] } = o)
const o = { x = 1 }
const o = { #x: 1 }
const o = { export x: 1 }

TODO: Duplicated with An object literal cannot have multiple properties with the same name in strict mode.

const o = { 
    x: 1,
    x() { }
let async
export const l = [1,2,3]
for (async of l) {
    console.log(x)
}
for (const x = 1 in [1,2,3]) {
    console.log(x)
}
for (const x = 1 of [1,2,3]) {
    console.log(x)
}
class C {
    get x();
}
class C {
    get x(n) { return 1 }
}
class C {
    set x() { }
    set y(a, b) { }
}
class C {
    set x(...n) { }
}
function outer() {
    outer: for(;;) {
        function test() {
            break outer
        }
        test()
    }
}
outer()
function outer(x) {
    outer: switch (x) {
        case 1:
            continue outer
    }
}
outer(1)
function outer(x) {
    break outer
}
function outer(x) {
    continue outer
}
outer(1)
function outer(x) {
    break
}
function outer(x) {
    continue
}
const o = { e: 1, m: 1, name: "knee-deep" }
const { ...rest, e: a, m: n } = o
a + n
const o = { e: 1, m: 1, name: "knee-deep" }
const { e: a, m: n, ...est: x } = o
a + n
const { e: a, m: n }
const a
let let = 12
if (true)
  let c3 = 0
if (true)
  const c3 = 0
export let x = import.metal
function foo() { new.targe }
class C {
    "constructor" = 1
}
const x = import()
const y = import('1', '2', '3')
const x = import(...[])