-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
curly 'multi' autofix produces syntax errors with lexical declarations #11908
Comments
This was recently fixed for I guess the solution should be the same - don't report. |
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? |
Yes, and So: First three are syntax errors.
|
I'm not sure how the spec treats these two cases:
In Chrome it seems that the names 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() {}' |
When I try the same examples without braces after 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 |
This might be also a bug: with the default if (foo) function bar() {} |
Sorry for the long thread. To summarize, these are the only blocking questions related directly to this issue and the
if (foo) {
function f() {}
} into this: if (foo)
function f() {}
if (foo)
function f() {} into this: if (foo) {
function f() {}
} |
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. |
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. |
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 |
Agreed - better to play it safe than potentially do something unsafe. |
Ok, I'm working on this. It might take some time, all options have to be checked and the |
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
Reopening this since we have a PR for it! |
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
@mdjermanovic would you like to assign this to yourself so it doesn't get autoclosed? |
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 From the specification B.3.4 FunctionDeclarations in IfStatement Statement Clauses it seems that a function as a single-statement in
So I plan to prepare another PR instead of #11912, which would only prevent removing braces around |
Tell us about your environment
What parser (default, Babel-ESLint, etc.) are you using?
default
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.
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.
Are you willing to submit a pull request to fix this bug?
Yes.
The text was updated successfully, but these errors were encountered: