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 · May be fixed by rsumner868/librealsense2.1#1, rsumner868/librealsense#4, O330oei/node#4 or O330oei/node#11

Comments

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 26, 2019

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

@mdjermanovic mdjermanovic commented Jun 26, 2019

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 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

@mdjermanovic mdjermanovic commented Jun 26, 2019

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 and removed evaluating labels Jun 26, 2019
@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jun 26, 2019

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

@mdjermanovic mdjermanovic commented Jun 26, 2019

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

@mdjermanovic mdjermanovic commented Jun 26, 2019

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

@mdjermanovic mdjermanovic commented Jun 26, 2019

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

@platinumazure platinumazure commented Jun 26, 2019

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

@kaicataldo kaicataldo commented Jun 26, 2019

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 mdjermanovic commented Jun 26, 2019

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

@kaicataldo kaicataldo commented Jun 26, 2019

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

@mdjermanovic
Copy link
Member Author

@mdjermanovic mdjermanovic commented Jun 26, 2019

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
Copy link
Contributor

@eslint eslint bot commented Sep 25, 2019

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 and removed auto closed labels Sep 26, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 26, 2019

Reopening this since we have a PR for it!

@kaicataldo kaicataldo reopened this Sep 26, 2019
@eslint eslint bot closed this Oct 27, 2019
@eslint eslint bot added the auto closed label Oct 27, 2019
@eslint
Copy link
Contributor

@eslint eslint bot commented Oct 27, 2019

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

@kaicataldo kaicataldo commented Oct 29, 2019

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

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

@mdjermanovic mdjermanovic commented Oct 30, 2019

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.

mdjermanovic added a commit that referenced this issue Oct 30, 2019
@eslint eslint bot locked and limited conversation to collaborators May 4, 2020
@eslint eslint bot added the archived due to age label May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.