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

ES3 reserved words #15017

Closed
zloirock opened this issue Sep 1, 2021 · 22 comments · Fixed by #15046
Closed

ES3 reserved words #15017

zloirock opened this issue Sep 1, 2021 · 22 comments · Fixed by #15046
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible bug ESLint is working incorrectly repro:yes

Comments

@zloirock
Copy link

zloirock commented Sep 1, 2021

The issue: zloirock/core-js#980

In the code used char as a variable name. The ESLint config contains ecmaVersion: 3, till this week here was used the default ESLint parser. char is a ES3 reserved word:

Снимок экрана 2021-09-01 в 20 46 37

ESLint parsed it without any errors or warnings. However, this code might not work in some ES3 engines.

@zloirock zloirock added bug ESLint is working incorrectly repro:needed labels Sep 1, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 1, 2021
@nzakas
Copy link
Member

nzakas commented Sep 2, 2021

Please update your issue to use our bug report template and we will take a look. Thanks.

@zloirock
Copy link
Author

zloirock commented Sep 2, 2021

@nzakas all this info you were able to find in core-js repo by the link above.

ESLint Version: 7.32.0 https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/package.json#L37
Node Version: 16 https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/.github/workflows/lint.yml#L12
npm Version: 7.21.0 https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/package.json#L55
Operating System: ubuntu-latest https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/.github/workflows/lint.yml#L11

Default ESLint parser with ecmaVersion: 3 option https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/.eslintrc.js#L989-L991

Full configuration https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/.eslintrc.js

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

https://github.com/zloirock/core-js/tree/4a56ece64adc9653a4c8a648955a5a95aba22fd8/packages/core-js

npm i && npm run lint

What did you expect to happen?

I expect a parsing error on
https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/packages/core-js/modules/web.url.js#L121
https://github.com/zloirock/core-js/blob/4a56ece64adc9653a4c8a648955a5a95aba22fd8/packages/core-js/modules/es.symbol.description.js#L34
since it should be a syntax error in ES3
Снимок экрана 2021-09-01 в 20 46 37

What actually happened? Please copy-paste the actual, raw output from ESLint.

eslint --ext .js,.mjs,.json ./

Steps to reproduce this issue:

  1. Copy https://github.com/zloirock/core-js/tree/4a56ece64adc9653a4c8a648955a5a95aba22fd8
  2. Run npm i && npm run lint

Are you willing to submit a pull request to fix this bug? No.


I don't think that it made understanding the issue simpler, I just spent time filling out a useless bureaucratic form.

@aladdin-add
Copy link
Member

aladdin-add commented Sep 2, 2021

I was able to repro, and it seems an issue in acorn.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2021

Acorn has an option to modify this behavior:

https://github.com/acornjs/acorn/tree/master/acorn#interface

  • allowReserved: If false, using a reserved word will generate
    an error. Defaults to true for ecmaVersion 3, false for higher
    versions. When given the value "never", reserved words and
    keywords can also not be used as property names (as in Internet
    Explorer's old parser).

Consequently, espree (ESLint's default parser, which uses acorn) allows reserved words when ecmaVersion: 3 is specified, since we are not setting this option so the defaults apply.

I'm not sure what was the reasoning for these defaults in acorn. With allowReserved: false, it does throw a parsing error (astexplorer.net snippet).

We could consider setting allowReserved: false here, but that feels like a breaking change. Thoughts?

@mdjermanovic mdjermanovic moved this from Needs Triage to Feedback Needed in Triage Sep 2, 2021
@zloirock
Copy link
Author

zloirock commented Sep 2, 2021

As an option, instead of parsing error, it can be a new rule that will forbid the usage of ES3 reserved words as variables names.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2021

As an option, instead of parsing error, it can be a new rule that will forbid the usage of ES3 reserved words as variables names.

This could be also achieved with the existing no-restricted-syntax rule:

/* eslint no-restricted-syntax: [
    "error",
    {
        selector: "Identifier[name=/^(?:abstract|boolean|byte|char)$/]",
        message: "ES3 reserved word"
    }
]*/

var char; // error

Online Demo

This configuration disallows the list of reserved words - I added only the first 4 for the example - everywhere where estree spec uses Identifier nodes (variables, labels, property names in object literals/member access, etc.).

Would this work for you?

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2021

id-denylist also provides similar functionality, though with some exceptions (e.g., property names are allowed).

@zloirock
Copy link
Author

zloirock commented Sep 2, 2021

@mdjermanovic I know, but I don't think that it's fine to recommend adding the ES3 reserved words list to those rules for all who need ES3 support.

@aladdin-add
Copy link
Member

aladdin-add commented Sep 2, 2021

the easiest way is to use the allowReserved option:

"parser": "espree",
"parserOptions": { "ecmaVersion": 3, "allowReserved": false}

I also agreed this can be the default.


oops, sorry it would not be working, as espree do not pass unknown options to acorn: https://github.com/eslint/espree/blob/63bd0bc46adfbcd3a71d4cc222aa3923b76ebcf2/lib/espree.js#L69

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2021

As an option, instead of parsing error, it can be a new rule that will forbid the usage of ES3 reserved words as variables names.

@mdjermanovic I know, but I don't think that it's fine to recommend adding the ES3 reserved words list to those rules for all who need ES3 support.

I agree this isn't an ideal solution, but it's a currently available workaround. We don't have core rules to check syntax, that rule seems more like a candidate for eslint-plugin-es.

"parser": "espree",
"parserOptions": { "ecmaVersion": 3, "allowReserved": false}

This would solve the problem in a non-breaking way, but we aim to avoid exposing Acorn options for various reasons.

I think that espree should just always pass "allowReserved": false to acorn, but that would have to wait for espree/eslint v9.

@zloirock
Copy link
Author

zloirock commented Sep 2, 2021

eslint-plugin-es

This plugin seems dead. Anyway, this issue looks like an ESLint core problem.

that would have to wait for espree/eslint v9

Why not add it to v8 while it's still in beta?

@aladdin-add
Copy link
Member

aladdin-add commented Sep 2, 2021

filed the issue in acorn: acornjs/acorn#1062 to see if they want to change the default.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2021

Why not add it to v8 while it's still in beta?

espree v8.0.0 has been already released, and the changes in eslint v8.0.0 are already announced.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible labels Sep 3, 2021
@nzakas
Copy link
Member

nzakas commented Sep 3, 2021

Because there’s agreement that this is a bug, I’m marking as accepted. We just need to determine how to proceed.

I’m guessing we don’t have a lot of ES3 usage right now, and ES3 code using reserved words will throw a runtime error, so even though this is a breaking change, I think the impact to end users would be minimal. So, we may want to evaluate this a bit differently than other breaking changes.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 4, 2021

We could fix this in Espree as a breaking change, release Espree v9.0.0, and then use Espree 9 in ESLint v8.0.0.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 4, 2021

All of my packages are authored in ES3, and none of them use char as a variable name, so I'd love to see this fix come in, even as a patch.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 4, 2021

Should we actually set allowReserved: "never" for ES3, to also disallow obj.char and var obj = { char: 1 }?

By https://www-archive.mozilla.org/js/language/E262-3.pdf (if that's the right document?):

Identifier ::
     IdentifierName but not ReservedWord

MemberExpression :
     ...
     MemberExpression . Identifier

ObjectLiteral :
     { }
     { PropertyNameAndValueList }
PropertyNameAndValueList :
     PropertyName : AssignmentExpression
     PropertyNameAndValueList , PropertyName : AssignmentExpression
PropertyName :
     Identifier
     StringLiteral
     NumericLiteral

@nzakas
Copy link
Member

nzakas commented Sep 7, 2021

We could fix this in Espree as a breaking change, release Espree v9.0.0, and then use Espree 9 in ESLint v8.0.0.

That’s what I was thinking, too.

Should we actually set allowReserved: "never" for ES3, to also disallow obj.char and var obj = { char: 1 }?

Yes. Reserved words were completely illegal in ES3.

@nzakas nzakas added this to Planned in Public Roadmap via automation Sep 7, 2021
@nzakas nzakas added this to Need Discussion in v8.0.0 Sep 7, 2021
@nzakas
Copy link
Member

nzakas commented Sep 7, 2021

Note from marijn about Acorn's default:

Most browsers in the ES3 era allowed reserved words to be used in scripts, hence this default. It is not going to change for backwards-compatibility reasons.

@nzakas
Copy link
Member

nzakas commented Sep 7, 2021

Opened a PR: eslint/espree#513

@nzakas nzakas moved this from Need Discussion to Planned in v8.0.0 Sep 9, 2021
@mdjermanovic mdjermanovic moved this from Planned to Pull Request Opened in v8.0.0 Sep 10, 2021
@mdjermanovic mdjermanovic moved this from Feedback Needed to Pull Request Opened in Triage Sep 10, 2021
@mdjermanovic mdjermanovic moved this from Planned to In Progress in Public Roadmap Sep 10, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 10, 2021

In yesterday's TSC meeting, we decided to accept this change for ESLint v8.0.0.

The change will be made directly in Espree. We'll release Espree v9.0.0 and use Espree v9 in ESLint v8.0.0.

Reserved words will also be disallowed in property names, such as obj.char or var obj = { char: 1 }. The new behavior is spec-compliant, and it isn't configurable.

This change only affects ESLint users with the default parser and "parserOptions": { "ecmaVersion": 3 }.

Public Roadmap automation moved this from In Progress to Complete Sep 10, 2021
Triage automation moved this from Pull Request Opened to Complete Sep 10, 2021
v8.0.0 automation moved this from Pull Request Opened to Done Sep 10, 2021
mdjermanovic added a commit that referenced this issue Sep 10, 2021
* Breaking: Disallow reserved words in ES3 (fixes #15017)

* use real version
@ljharb
Copy link
Sponsor Contributor

ljharb commented Nov 17, 2021

Turns out there's ES3 browsers where these reserved words work fine - native and int, in particular, work fine in IE 6, for example.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 10, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible bug ESLint is working incorrectly repro:yes
Projects
Public Roadmap
  
Complete
Triage
Complete
v8.0.0
  
Done
Development

Successfully merging a pull request may close this issue.

5 participants