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

curly 'multi' autofix produces syntax errors with lexical declarations #11908

Closed
mdjermanovic opened this issue Jun 26, 2019 · 18 comments · Fixed by #12513, basics/vector#111, basics/vector#113, thinkwee/thinkwee.github.io#39 or alxtford/numconv#46
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 autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Tell us about your environment

  • ESLint Version: 6.0.1 (same with 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: 6,
  },
  rules: {
    "curly": ["error", "multi"]
  }
};

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

if (foo) {
  let bar;
}
eslint index.js --fix

What did you expect to happen?

No errors, similar to how multi-or-nest works.

What actually happened? Please include the actual, raw output from ESLint.

if (foo) 
  let bar; // SyntaxError: Lexical declaration cannot appear in a single-statement context

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 Jun 26, 2019
@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jun 26, 2019
@mdjermanovic
Copy link
Member Author

This was recently fixed for multi-or-nest (#11663) but not for multi, which is another option that can remove braces.

I guess the solution should be the same - don't report.

@kaicataldo
Copy link
Member

kaicataldo commented Jun 26, 2019

That makes sense to me. I was able to verify this in our demo. Just for clarity, this would also apply to

if (foo) {
  const bar = true;
}

correct?

@mdjermanovic
Copy link
Member Author

Yes, and class and function shouldn't be fixed as well.

So: const, let, class and function

First three are syntax errors.

function might depend on the parser, I'll check it more, but it certainly isn't a good idea to move it up from braces.

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 26, 2019
@mdjermanovic
Copy link
Member Author

I'm not sure how the spec treats these two cases:

  • function foo () { if (bar) function a() {} }
  • function foo () { if (bar) { function a() {} } }

In Chrome it seems that the names a are hoisted, but values not, Value is undefined until the execution reaches the declaration.

const a = 1;
function f() { return a; if (false) { function a() {}  } }
f(); // undefined, 'a' is shadowed
const a = 1;
function f() { if (true) { function a() {}  }; return a;  }
f(); // 'ƒ a() {}'

@mdjermanovic
Copy link
Member Author

When I try the same examples without braces after if, the results are the same.

But perhaps it wouldn't be like that in other engines, and maybe it still isn't a good idea to remove or put braces around function declarations in the fix.

By the way, ESLint's scope manager treats nested function declarations as block scoped like let, which doesn't match this behavior in Chrome.

@mdjermanovic
Copy link
Member Author

This might be also a bug: with the default all option, curly would put braces around the declaration:

if (foo) function bar() {}

@mdjermanovic
Copy link
Member Author

Sorry for the long thread.

To summarize, these are the only blocking questions related directly to this issue and the curly rule:

  1. Should the rule ever transform this:
if (foo) {
    function f() {}
}

into this:

if (foo) 
    function f() {}
  1. Should the rule ever transform this:
if (foo) 
    function f() {}

into this:

if (foo) {
    function f() {}
}

@platinumazure
Copy link
Member

Function scopes aren't lexical, right? They're hoisted to the top of the function scope, right? And having a function declaration as part of an if statement's blockless consequent is not a syntax error?

If I'm understanding that correctly, I would say the rule could safely autofix if there are functions inside the if statement.

@kaicataldo
Copy link
Member

To add another wrinkle to this, it looks like strict mode can affect whether this is a syntax error or not:

This yields a syntax error:

'use strict'

if (foo) 
    function f() {}

This does not:

if (foo) 
    function f() {}

I'm trying to find where this is defined in the spec without much luck (hopefully someone else can find it!), but some relevant resources I found:

The above two articles make it seem like different browser engines might treat them differently, as well.

@mdjermanovic
Copy link
Member Author

@mdjermanovic
Copy link
Member Author

It's most likely a bad practice to have a function declaration anywhere other than directly in the body.

Perhaps ESLint should never modify such code, i.e. never move function declarations to {} or out from its existing {}, because it can produce some unpredictable differences even if it isn't a syntax error.

@kaicataldo
Copy link
Member

Agreed - better to play it safe than potentially do something unsafe.

@mdjermanovic
Copy link
Member Author

Ok, I'm working on this.

It might take some time, all options have to be checked and the consistent modifier adds some complexity.

@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@kaicataldo kaicataldo added autofix This change is related to ESLint's autofixing capabilities and removed auto closed The bot closed this issue labels Sep 26, 2019
@kaicataldo
Copy link
Member

Reopening this since we have a PR for it!

@kaicataldo kaicataldo reopened this Sep 26, 2019
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Oct 27, 2019
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@kaicataldo kaicataldo reopened this Oct 29, 2019
@kaicataldo
Copy link
Member

@mdjermanovic would you like to assign this to yourself so it doesn't get autoclosed?

@kaicataldo kaicataldo removed the auto closed The bot closed this issue label Oct 29, 2019
@mdjermanovic mdjermanovic self-assigned this Oct 30, 2019
@mdjermanovic
Copy link
Member Author

Yes, and I think it might be better to just fix the error from the original post at the moment.

The assumption that it isn't safe to convert if (foo) function f(){} to if (foo) { function f(){} } (which makes problems in PR #11912) might be wrong.

From the specification B.3.4 FunctionDeclarations in IfStatement Statement Clauses it seems that a function as a single-statement in if is treated as if it was in a block statement:

Code matching this production is processed as if each matching occurrence of FunctionDeclaration was the sole StatementListItem of a BlockStatement occupying that position in the source code.

So I plan to prepare another PR instead of #11912, which would only prevent removing braces around function, class, let and const, with a min. diff.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 4, 2020
@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 May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.