no-inner-declarations inconsistency

[mdjermanovic]

Tell us about your environment

• ESLint Version: 6.3.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: 2015,
  }
};

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

Demo link

/*eslint no-inner-declarations: ["error", "both"]*/

if (foo)
  var a;

if (foo)
  function f(){}

function bar() {
  if (foo)
    var a;

  if (foo)
    function f(){}
}

eslint index.js

What did you expect to happen?

To be consistent, either 4 errors or no errors?

This rule requires that function declarations and, optionally, variable declarations be in the root of a program or the body of a function.

Are these declarations in the root/body? Also, the documentation is later more specific:

"functions" (default) disallows function declarations in nested blocks

"both" disallows function and var declarations in nested blocks

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

2 errors:

11:5  error  Move variable declaration to function body root  no-inner-declarations
14:5  error  Move function declaration to function body root  no-inner-declarations

/*eslint no-inner-declarations: ["error", "both"]*/

if (foo)
  var a; // no error

if (foo)
  function f(){} // no error

function bar() {
  if (foo)
    var a; // error

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

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

Yes, if this is confirmed to be a bug.

Perhaps the rule should allow only these places for function and var declaration nodes:

• Program > node
• ExportNamedDeclaration > node
• ExportDefaultDeclaration > node (function only)
• Function > BlockStatement > node

meaning that there should be 4 errors in the above example, but I'm not sure is this the intention of this rule.

[platinumazure]

I would be 👍 for having 4 errors in your example.

[eslint-deprecated]

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 issues failing to reach accepted status after 21 days tend to
never be accepted, 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]

This looks like a bug to me - reopening so we can evaluate.

[eslint-deprecated]

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.

[anikethsaha]

@mdjermanovic when wrapping the if statement blocks with parens { } , all are showing errors

/*eslint no-inner-declarations: ["error", "both"]*/

if (foo){
  var a; // error
}

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

function bar() {
  if (foo)
    var a;  // error

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

demo

[anikethsaha]

BlockStatement > VariableDeclaration should be check as well

[anikethsaha]

@mdjermanovic I have a fix working for this! mind if I submit the change?

here is the fix anikethsaha@83f83c5

[mdjermanovic]

@anikethsaha Sure, thanks!

It isn't entirely clear what should be done, but it looks like a good idea to continue the discussion on a PR.

[kaicataldo]

@mdjermanovic @anikethsaha Could one of you sum up what needs to be discussed here so that the rest of the team can easily provide input?

[mdjermanovic]

The question is should we change the logic to report all declarations except those that have either:

• Program parent
• ExportNamedDeclaration parent
• ExportDefaultDeclaration parent
• BlockStatement parent and FunctionDeclaration grandparent
• BlockStatement parent and FunctionExpression grandparent
• BlockStatement parent and ArrowFunctionExpression grandparent

In particular, cases like these would be new warnings:

if (foo)
  var a; 

if (foo)
  function f(){}

while (foo)
  var a; 

[kaicataldo]

Understood. I think there should be no errors in the original example since the documentation specifically refers to blocks. This feels especially true with lexically scoped variable declarations, since the actual scope would change.

[mdjermanovic]

I believe there are no lexically scoped variable declarations in those cases.

if (foo) function f(){} is allowed only by Annex B (B.3.4)

In strict mode, it's a syntax error.

In non-strict mode, it's treated as if it was if (foo) { function f(){} }, which by B.3.3 isn't lexically scoped in non-strict mode.

/* not strict */

f; // doesn't throw ReferenceError

console.log(typeof f); // undefined, isn't intialized yet

if (true)
    function f() { return 33; }

console.log(f()); // 33


g; // doesn't throw ReferenceError

console.log(typeof g); // undefined, isn't intialized yet

if (true) {
    function g() { return 55; }
}

console.log(g()); // 55

If this rule shouldn't report if (foo) function f(){} by default, then it would be useful to have an option or another rule for that, since it's a legacy Annex B feature with a confusing behavior.

[kaicataldo]

I believe there are no lexically scoped variable declarations in those cases.

Ah, you're right. Thanks for explaining this. I agree with you that this should have 4 errors. 👍

[kaicataldo]

Given three team members (@platinumazure, @mdjermanovic, and myself) believe this should have 4 errors, I think we can confidently move forward here. Removing the "needs bikeshedding" label.