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

no-octal rule on NonOctalDecimalIntegerLiterals #11794

Closed
mdjermanovic opened this issue May 31, 2019 · 5 comments · Fixed by #11805 · May be fixed by Omrisnyk/npm-lockfiles#130
Closed

no-octal rule on NonOctalDecimalIntegerLiterals #11794

mdjermanovic opened this issue May 31, 2019 · 5 comments · Fixed by #11805 · May be fixed by Omrisnyk/npm-lockfiles#130
Assignees
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Tell us about your environment

  • ESLint Version:
    5.16.0
  • Node Version:
    10.16.0
  • npm Version:
    6.9.0

What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:

Configuration
module.exports = {
  parserOptions: {
    ecmaVersion: 2018,
  },
  rules: {
    'no-octal': 'error',
  },
};

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

let x;
x = 018;
x = 081;
x = 018.2;
x = 081.2;
x = 018e2;
x = 081e2;
eslint index.js

What did you expect to happen?
By the specification both 018 and 081 are NonOctalDecimalIntegerLiteral, not LegacyOctalIntegerLiteral.

If the no-octal rule is supposed to catch these literals (which look like octal literals
but are actually evaluated as decimal literals), then it should report all 6 as errors.

Otherwise, if the rule is supposed to catch only octal literals, then it should report zero errors, and perhaps a new rule could be added.

What actually happened? Please include the actual, raw output from ESLint.
Only 018, 018.2 and 018e2 are reported as errors:

2:5 error Octal literals should not be used no-octal
4:5 error Octal literals should not be used no-octal
6:5 error Octal literals should not be used no-octal

✖ 3 problems (3 errors, 0 warnings)

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

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels May 31, 2019
@aladdin-add aladdin-add self-assigned this May 31, 2019
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 31, 2019
@aladdin-add
Copy link
Member

thanks for the issue!
sounds reasonable to me. I'll champion it.

@aladdin-add aladdin-add added rule Relates to ESLint's core rules and removed bug ESLint is working incorrectly labels May 31, 2019
@not-an-aardvark
Copy link
Member

Is NonOctalDecimalIntegerLiteral allowed in strict mode? Given the parsing rules here are mildly complicated for users to understand, it seems like maybe it would be better to just enforce "don't use leading zeros" as a rule of thumb.

By the way, I made a parser that could help with this if we do decide to implement it: https://github.com/not-an-aardvark/js-numeric-literal-parser

@mdjermanovic
Copy link
Member Author

NonOctalDecimalIntegerLiteral is explicitly forbidden in strict mode by the Numeric Literals specification, first section (Syntax), last paragraph:

A conforming implementation, when processing strict mode code, must not extend, as described in B.1.1, the syntax of NumericLiteral to include LegacyOctalIntegerLiteral, nor extend the syntax of DecimalIntegerLiteral to include NonOctalDecimalIntegerLiteral.

In strict mode, ESLint's parser itself correctly reports 'Parsing error: Invalid number' on first occurence of any
LegacyOctalIntegerLiteral or NonOctalDecimalIntegerLiteral.

In non-strict mode, no-octal rule will currently catch:

  • All LegacyOctalIntegerLiteral
  • Only NonOctalDecimalIntegerLiteral that start with 00...07, but not 08 and 09.

Looks like a simple replace of /^0[0-7]/ with /^0[0-9]/ in the rule code would catch all these.

@not-an-aardvark
Copy link
Member

You're right, I think this is just a bug in the rule. It doesn't make sense that 018 would be reported while 081 would not.

@mdjermanovic
Copy link
Member Author

It most likely wasn't a bug when /^0[0-7]/ commit was made, at least by the then actual specifications. There is nothing that could match 018 or 081 in 5.1 spec, its annex or any older specs, so these literals were probably expected to be caught by the parser as invalid.

However, browsers did support them as decimal integers. There were attempts to remove this support, but some important sites/apps were broken, so they are eventually included in ES6 as NonOctalDecimalIntegerLiteral and /^0[0-7]/ became a bug.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Jun 1, 2019
platinumazure pushed a commit that referenced this issue Jun 8, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 6, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 6, 2019
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
3 participants