Skip to content
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

Provide Grammar Errors for JavaScript Files #45349

Open
DanielRosenwasser opened this issue Aug 6, 2021 · 4 comments · Fixed by #46816
Open

Provide Grammar Errors for JavaScript Files #45349

DanielRosenwasser opened this issue Aug 6, 2021 · 4 comments · Fixed by #46816
Assignees
Labels
Domain: JavaScript The issue relates to JavaScript specifically Experimentation Needed Someone needs to try this out to see what happens Fix Available A PR has been opened for this issue Meta-Issue An issue about the team, or the direction of TypeScript Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

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:

  • How do we implement this as part of our API?
  • What sorts of errors are acceptable here?
  • Would we be giving errors that run against the expectations of any popular runtimes, build tools, or syntax extensions?
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript Experimentation Needed Someone needs to try this out to see what happens labels Aug 6, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.5.0 milestone Aug 6, 2021
@DanielRosenwasser DanielRosenwasser added the Domain: JavaScript The issue relates to JavaScript specifically label Aug 6, 2021
@sandersn
Copy link
Member

sandersn commented Aug 9, 2021

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:

  • Diagnostics.A_module_cannot_have_multiple_default_exports
  • Diagnostics.Another_export_default_is_here
  • Diagnostics.and_here <-- (!) this might be used elsewhere
  • Diagnostics.The_first_export_default_is_here
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.

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

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

  • Diagnostics.Duplicate_identifier_0
  • Diagnostics.Unused_label
  • Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode_Class_definitions_are_automatically_in_strict_mode
  • Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode_Modules_are_automatically_in_strict_mode
  • Diagnostics.Identifier_expected_0_is_a_reserved_word_in_strict_mode <-- (probably)
  • Diagnostics.Unreachable_code_detected

Sir Not-Appearing-in-this-Language

These errors only apply to Typescript constructs or Typescript emit.

  • Diagnostics.Enum_declarations_can_only_merge_with_namespace_or_other_enum_declarations
  • Diagnostics.Did_you_mean_0
  • Diagnostics.export_modifier_cannot_be_applied_to_ambient_modules_and_module_augmentations_since_they_are_always_visible
  • Diagnostics.Pattern_0_can_have_at_most_one_Asterisk_character
  • Diagnostics.Function_declarations_are_not_allowed_inside_blocks_in_strict_mode_when_targeting_ES3_or_ES5_Class_definitions_are_automatically_in_strict_mode
  • Diagnostics.Function_declarations_are_not_allowed_inside_blocks_in_strict_mode_when_targeting_ES3_or_ES5_Modules_are_automatically_in_strict_mode
  • Diagnostics.Function_declarations_are_not_allowed_inside_blocks_in_strict_mode_when_targeting_ES3_or_ES5
  • Diagnostics.Modifiers_cannot_appear_here
  • Diagnostics.Global_module_exports_may_only_appear_at_top_level
  • Diagnostics.Global_module_exports_may_only_appear_in_module_files
  • Diagnostics.Global_module_exports_may_only_appear_in_declaration_files
  • Diagnostics.Duplicate_identifier_0

@sandersn sandersn added the Meta-Issue An issue about the team, or the direction of TypeScript label Sep 21, 2021
@sandersn
Copy link
Member

@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
Copy link
Member Author

DanielRosenwasser commented Oct 14, 2021

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

@sandersn
Copy link
Member

sandersn commented Nov 20, 2021

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.

  • Diagnostics.Private_identifiers_are_not_allowed_outside_class_bodies
  • Diagnostics.Private_identifiers_are_only_allowed_in_class_bodies_and_may_only_be_used_as_part_of_a_class_member_declaration_property_access_or_on_the_left_hand_side_of_an_in_expression
  • Diagnostics.Cannot_find_name_0
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

  • Diagnostics.Cannot_assign_to_private_method_0_Private_methods_are_not_writable
class C {
    #m() {
         this.#m = () => {}
    }
}
  • Diagnostics.Private_identifiers_are_not_allowed_outside_class_bodies
    junk.#m (you get a better error if C is defined and even better if C.#m is defined)

  • Diagnostics._0_and_1_operations_cannot_be_mixed_without_parentheses

var x = 1 || 2 ?? 2
var x = 2 ?? 3 || 4
  • Diagnostics.For_await_loops_cannot_be_used_inside_a_class_static_block
class C {
    static {
        for await (const x of [1,2,3]) {
            console.log(x)
        }
    }
}
  • Diagnostics.A_default_clause_cannot_appear_more_than_once_in_a_switch_statement
var b = true
switch (b) {
    case false:
        console.log('no')
    default:
        console.log('yes')
    default:
        console.log('wat')
}
  • Diagnostics.Duplicate_label_0
label: for (const x in [1,2,3]) {
    label: for (const y in [1,2,3]) {
        break label;
    }
}
  • Diagnostics.Cannot_redeclare_identifier_0_in_catch_clause
try {
    throw 2
}
catch (e) {
    const e = 1
    console.log(e)
}
  • Diagnostics.Class_decorators_can_t_be_used_with_static_private_identifier_Consider_removing_the_experimental_decorator
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

  • Diagnostics.A_class_member_cannot_have_the_0_keyword
 class C {
    const x = 1
    const y() {
        return 12
    }
}
  • Diagnostics._0_modifier_already_seen
class C {
    static static x = 1
}
  • Diagnostics._0_modifier_must_precede_1_modifier
class C {
    static async x (){ }
}
  • Diagnostics._0_modifier_cannot_appear_on_a_module_or_namespace_element
export static var x = 1
  • Diagnostics._0_modifier_cannot_appear_on_a_parameter
function f(static x = 1) { return x }
  • Diagnostics._0_modifier_already_seen
export export var x = 1
  • Diagnostics._0_modifier_must_precede_1_modifier
async export function f(x = 1) { return x }
  • Diagnostics._0_modifier_cannot_appear_on_class_elements_of_this_kind
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.

  • Diagnostics._0_modifier_cannot_appear_on_a_parameter
function f(export x = 1) { return x }
  • Diagnostics._0_modifier_must_precede_1_modifier
default export 13;
  • Diagnostics._0_modifier_already_seen
async async function f() {
    return 1
}
  • Diagnostics._0_modifier_cannot_appear_on_a_parameter
function f(async x = 1) { return x }
  • Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration
class C {
    static constructor() { }
}
  • Diagnostics._0_modifier_cannot_appear_on_a_constructor_declaration
class C {
    async constructor() { }
}
  • Diagnostics._0_modifier_cannot_be_used_here
async class C {
    async x = 1
}
async const y = 2
async import './other'
async export { C }
  • Diagnostics.A_rest_parameter_must_be_last_in_a_parameter_list
function f (...x, y) {
}
  • Diagnostics.A_rest_parameter_cannot_have_an_initializer
function f (...x = [1,2,3]) {
}
  • Diagnostics.Line_terminator_not_permitted_before_arrow
const arr = x
  => x + 1
  • Diagnostics.Tagged_template_expressions_are_not_permitted_in_an_optional_chain
var a = [1,2]
a?.`length`;
  • Diagnostics.A_comma_expression_is_not_allowed_in_a_computed_property_name
const o = {
    [console.log('oh no'),2]: 'hi'
}
  • InvalidQuestionMark
const o = {
    x?: 12
}

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

const o = {
    m?() { return 12 }
}
  • InvalidExclamationToken
var x = 1
const o = {
    x!
}
const o = {
    m!() { return 12 }
}
  • Diagnostics.A_rest_element_cannot_contain_a_binding_pattern
const o = {
    x: 1, y: 2
}
let x; let y
;({ ...[] } = o)
  • Diagnostics.Did_you_mean_to_use_a_Colon_An_can_only_follow_a_property_name_when_the_containing_object_literal_is_part_of_a_destructuring_pattern
const o = { x = 1 }
  • Diagnostics.Private_identifiers_are_not_allowed_outside_class_bodies
const o = { #x: 1 }
  • Diagnostics._0_modifier_cannot_be_used_here
const o = { export x: 1 }
  • Diagnostics.Duplicate_identifier_0

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

const o = { 
    x: 1,
    x() { }
  • Diagnostics.JSX_elements_cannot_have_multiple_attributes_with_the_same_name

  • Diagnostics.JSX_attributes_must_only_be_assigned_a_non_empty_expression

  • Diagnostics.JSX_property_access_expressions_cannot_include_JSX_namespace_names

  • Diagnostics.JSX_expressions_may_not_use_the_comma_operator_Did_you_mean_to_write_an_array
    -- TODO Come back here and figure out what JSX errors are good --

  • Diagnostics.The_left_hand_side_of_a_for_of_statement_may_not_be_async

let async
export const l = [1,2,3]
for (async of l) {
    console.log(x)
}
  • Diagnostics.The_variable_declaration_of_a_for_in_statement_cannot_have_an_initializer
for (const x = 1 in [1,2,3]) {
    console.log(x)
}
  • Diagnostics.The_variable_declaration_of_a_for_of_statement_cannot_have_an_initializer;
for (const x = 1 of [1,2,3]) {
    console.log(x)
}
  • Diagnostics._0_Expected
class C {
    get x();
}
  • Diagnostics.A_get_accessor_cannot_have_parameters
class C {
    get x(n) { return 1 }
}
  • Diagnostics.A_set_accessor_must_have_exactly_one_parameter
class C {
    set x() { }
    set y(a, b) { }
}
  • Diagnostics.A_set_accessor_cannot_have_rest_parameter
class C {
    set x(...n) { }
}
  • Diagnostics.Jump_target_cannot_cross_function_boundary
function outer() {
    outer: for(;;) {
        function test() {
            break outer
        }
        test()
    }
}
outer()
  • Diagnostics.A_continue_statement_can_only_jump_to_a_label_of_an_enclosing_iteration_statement
function outer(x) {
    outer: switch (x) {
        case 1:
            continue outer
    }
}
outer(1)
  • Diagnostics.A_break_statement_can_only_jump_to_a_label_of_an_enclosing_statement
function outer(x) {
    break outer
}
  • Diagnostics.A_continue_statement_can_only_jump_to_a_label_of_an_enclosing_iteration_statement;
function outer(x) {
    continue outer
}
outer(1)
  • Diagnostics.A_break_statement_can_only_be_used_within_an_enclosing_iteration_or_switch_statement
function outer(x) {
    break
}
  • Diagnostics.A_continue_statement_can_only_be_used_within_an_enclosing_iteration_statement;
function outer(x) {
    continue
}
  • Diagnostics.A_rest_element_must_be_last_in_a_destructuring_pattern
const o = { e: 1, m: 1, name: "knee-deep" }
const { ...rest, e: a, m: n } = o
a + n
  • Diagnostics.A_rest_element_cannot_have_a_property_name
const o = { e: 1, m: 1, name: "knee-deep" }
const { e: a, m: n, ...est: x } = o
a + n
  • Diagnostics.A_destructuring_declaration_must_have_an_initializer
const { e: a, m: n }
  • Diagnostics.const_declarations_must_be_initialized
const a
  • Diagnostics.let_is_not_allowed_to_be_used_as_a_name_in_let_or_const_declarations
let let = 12
  • Diagnostics.let_declarations_can_only_be_declared_inside_a_block
if (true)
  let c3 = 0
  • Diagnostics.const_declarations_can_only_be_declared_inside_a_block
if (true)
  const c3 = 0
  • Diagnostics._0_is_not_a_valid_meta_property_for_keyword_1_Did_you_mean_2
export let x = import.metal
  • Diagnostics._0_is_not_a_valid_meta_property_for_keyword_1_Did_you_mean_2
function foo() { new.targe }
  • Diagnostics.Classes_may_not_have_a_field_named_constructor
class C {
    "constructor" = 1
}
  • Diagnostics.Dynamic_imports_can_only_accept_a_module_specifier_and_an_optional_assertion_as_arguments
const x = import()
const y = import('1', '2', '3')
  • Diagnostics.Argument_of_dynamic_import_cannot_be_spread_element
const x = import(...[])

sandersn added a commit that referenced this issue Dec 8, 2021
From the list in
#45349 (comment),
excluding the JSX errors, decorator errors and unresolved #private
error. The latter re-uses "Cannot resolve name X" which naturally shows
up a *lot* in plain JS.
sandersn added a commit that referenced this issue Dec 8, 2021
* Plain JS grammar errors

From the list in
#45349 (comment),
excluding the JSX errors, decorator errors and unresolved #private
error. The latter re-uses "Cannot resolve name X" which naturally shows
up a *lot* in plain JS.

* Add grammarErrorAtPos errors

Also remove checkGrammarArguments; it's blocked entirely by the same
parser error.
sandersn added a commit that referenced this issue Dec 8, 2021
These are the last ones that I know of. They come from calls to
`grammarErrorOnFirstToken`.

Fixes part of #45349
Follow-up to #47067
sandersn added a commit that referenced this issue Dec 9, 2021
These are the last ones that I know of. They come from calls to
`grammarErrorOnFirstToken`.

Fixes part of #45349
Follow-up to #47067
mprobst pushed a commit to mprobst/TypeScript that referenced this issue Jan 10, 2022
* Plain JS grammar errors

From the list in
microsoft#45349 (comment),
excluding the JSX errors, decorator errors and unresolved #private
error. The latter re-uses "Cannot resolve name X" which naturally shows
up a *lot* in plain JS.

* Add grammarErrorAtPos errors

Also remove checkGrammarArguments; it's blocked entirely by the same
parser error.
mprobst pushed a commit to mprobst/TypeScript that referenced this issue Jan 10, 2022
These are the last ones that I know of. They come from calls to
`grammarErrorOnFirstToken`.

Fixes part of microsoft#45349
Follow-up to microsoft#47067
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 4.6.0 milestone Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: JavaScript The issue relates to JavaScript specifically Experimentation Needed Someone needs to try this out to see what happens Fix Available A PR has been opened for this issue Meta-Issue An issue about the team, or the direction of TypeScript Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants