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

feat!: no-inner-declaration new default behaviour and option #17885

Merged
merged 18 commits into from Jan 9, 2024

Conversation

Tanujkanti4441
Copy link
Contributor

@Tanujkanti4441 Tanujkanti4441 commented Dec 20, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?
no-inner-declarations

What change do you want to make (place an "X" next to just one item)?

[ ] Generate more warnings
[x] Generate fewer warnings
[ ] Implement autofix
[ ] Implement suggestions

How will the change be implemented (place an "X" next to just one item)?

[x] A new option
[x] A new default behavior
[ ] Other

Please provide some example code that this change will affect:

/*eslint no-inner-declarations: ["error", { blockScopeFunctions: "allow" }]*/
/*eslint-env es6*/

"use strict"
if (test) {
    function doSomething() { } // no error
}
/*eslint no-inner-declarations: ["error", { blockScopeFunctions: "disallow" }]*/

"use strict"
if (test) {
    function doSomething() { } // error
}

What does the rule currently do for this code?
By default it reports function declaration in all conditions.

What will the rule do after it's changed?
New option called blockScopeFunctions that has two string options allow (default) and disallow will allow the function declarations in strict mode when ecmaVersion is >=2015.

Is there anything you'd like reviewers to focus on?

Fixes: #15576

@Tanujkanti4441 Tanujkanti4441 requested a review from a team as a code owner December 20, 2023 11:30
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 20, 2023
Copy link

netlify bot commented Dec 20, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 4f00873
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/659bde5f346365000869a279
😎 Deploy Preview https://deploy-preview-17885--docs-eslint.netlify.app/rules/no-inner-declarations
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Tanujkanti4441 Tanujkanti4441 added the rule Relates to ESLint's core rules label Dec 20, 2023
@mdjermanovic mdjermanovic changed the title feat: no-inner-declaration new default behaviour and option feat!: no-inner-declaration new default behaviour and option Dec 20, 2023
@eslint-github-bot eslint-github-bot bot added the breaking This change is backwards-incompatible label Dec 20, 2023
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Dec 20, 2023
@mdjermanovic
Copy link
Member

I marked this as a breaking change because of the new default behavior.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. Overall looks good, just a bit of cleanup.

Note that modules are always in strict mode, so we should add tests for that, too.

And we also need to update eslint:recommended to remove this.

@@ -66,6 +76,22 @@ module.exports = {

create(context) {

const sourceCode = context.sourceCode;
const programAst = sourceCode.ast;
const ecmaVersion = context.languageOptions.ecmaVersion || 6;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are really checking for undefined here?

Suggested change
const ecmaVersion = context.languageOptions.ecmaVersion || 6;
const ecmaVersion = context.languageOptions.ecmaVersion ?? 6;

Comment on lines 82 to 83
const options = context.options[1];
const legacy = options && options.legacy || false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make this a bit simpler.

Suggested change
const options = context.options[1];
const legacy = options && options.legacy || false;
const legacy = context.options[1]?.legacy ?? false;

* Check whether the linted file is in the strict mode or not.
* @returns {boolean} `true` if code is in strict mode.
*/
function isStrictCode() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't cover all of ways to determine if code is in strict mode. It's better to check the global scope, like this:

const isStrict = sourceCode.getScope(sourceCode.ast).isStrict;

@Tanujkanti4441
Copy link
Contributor Author

Tanujkanti4441 commented Dec 21, 2023

Note that modules are always in strict mode, so we should add tests for that, too.

@nzakas following test is failing in valid, isn't it the right way?

{
  code: "if (foo) var fn = function(){} ",
  options: ["both"],
  languageOptions: { ecmaVersion: 2022, sourceType: "module" }
}

@nzakas
Copy link
Member

nzakas commented Dec 26, 2023

@Tanujkanti4441 I think that should be in the invalid section?

Also, please be sure to rebase. There have been a lot of changes since this PR was started.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Just a bit of cleanup and I think we are done.

@@ -61,14 +61,15 @@ function doSomething() {

## Rule Details

This rule requires that function declarations and, optionally, variable declarations be in the root of a program, or in the root of the body of a function, or in the root of the body of a class static block.
This rule requires that function declarations and, optionally, variable declarations be in the root of a program, or in the root of the body of a function, or in the root of the body of a class static block in the non-strict code. This rule doesn't report function declarations and variable declarations if code is in strict mode (code with `"use strict"` tag).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity

Suggested change
This rule requires that function declarations and, optionally, variable declarations be in the root of a program, or in the root of the body of a function, or in the root of the body of a class static block in the non-strict code. This rule doesn't report function declarations and variable declarations if code is in strict mode (code with `"use strict"` tag).
This rule requires that function declarations and, optionally, variable declarations be in the root of a program, or in the root of the body of a function, or in the root of a class static block in non-strict code. This rule doesn't report function declarations and variable declarations if code is in strict mode (code with `"use strict"` tag or ESM modules).


* `"functions"` (default) disallows `function` declarations in nested blocks
* `"both"` disallows `function` and `var` declarations in nested blocks
* `{ legacy: false }` (default) if this option set to `true` then the rule will report only function declarations even in the strict mode but only if the eslint is configured to `ecmaVersion: 5`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is clearer?

Suggested change
* `{ legacy: false }` (default) if this option set to `true` then the rule will report only function declarations even in the strict mode but only if the eslint is configured to `ecmaVersion: 5`.
* `{ legacy: false }` (default) if this option set to `true` then the rule will report only function declarations when `ecmaVersion` is set to `5`.

VariableDeclaration(node) {
if (isStrict) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check strict mode here again, because a variable can be inside of a function that is in strict mode even if the entire script is not.

Suggested change
if (isStrict) {
if (sourceCode.getScope(node).isStrict) {

@mdjermanovic
Copy link
Member

Here's a short overview of the problem (more details in #15576 (comment)):

  • Inner var declarations behave the same in all contexts. Whether or not they should be allowed in a codebase is a stylistic choice.
  • Inner function declarations have different behavior in different contexts.
    • ES6+ environment AND strict code: well-defined, consistent behavior. Whether or not they should be allowed in a codebase is a stylistic choice.
    • < ES6 environment OR non-strict code: problematic legacy behavior, should be avoided.

The rule currently disallows all inner function declarations, regardless of the context. The idea is to provide an option to allow inner function declarations in strict code if languageOptions.ecmaVersion >= 2015, while all other function declarations would still be disallowed. When the option is not enabled, the rule should retain the current behavior.

// languageOptions.ecmaVersion < 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: false }] */

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion < 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: true }] */

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion < 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: false }] */

"use strict";

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion < 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: true }] */

"use strict";

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion >= 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: false }] */

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion >= 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: true }] */

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion >= 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: false }] */

"use strict";

if (test) {
    function doSomething() {} // error
}
// languageOptions.ecmaVersion >= 2015

/* eslint no-inner-declarations: ["error", "functions", { legacy: true }] */

"use strict";

if (test) {
    function doSomething() {} // ok
}

I think the option should be enabled by default so that the rule by default disallows only legacy behavior, and then the user can opt-in to stylistic preferences by configuring { legacy: false } to disallow all inner function declarations (current default behavior) and/or by configuring "both" to also disallow inner var declarations.`

@mdjermanovic
Copy link
Member

We didn't precisely define what the option name means, and my understanding was that "legacy" refers to legacy JS semantics of inner function declarations rather than the legacy behavior of this rule.

Either way, we could define it now? It might be better to name the option in a way that describes what it includes/excludes than to refer to a previous behavior of this rule.

What do you think about allowBlockScopedFunctions (default true)?

@nzakas
Copy link
Member

nzakas commented Jan 3, 2024

What do you think about allowBlockScopedFunctions (default true)?

I think the term "block-scoped function" may be confusing in this context. The rule exists because ES < 6 did not have block scoped functions and therefore putting a function inside a block was dangerous. So maybe something like forbidFunctionsInBlocks with a default of false, or we could do functionsInBlocks with a default of "allow" and optional "disallow".

Regardless, whenever we have a boolean option, I think the default should be false.

@mdjermanovic
Copy link
Member

What do you think about allowBlockScopedFunctions (default true)?

I think the term "block-scoped function" may be confusing in this context. The rule exists because ES < 6 did not have block scoped functions and therefore putting a function inside a block was dangerous. So maybe something like forbidFunctionsInBlocks with a default of false, or we could do functionsInBlocks with a default of "allow" and optional "disallow".

Regardless, whenever we have a boolean option, I think the default should be false.

That's the point of the allowBlockScopedFunctions option name - it allows only block-scoped inner functions. i.e. inner functions in ES6+ strict code.

functions in blocks = inner functions

Maybe blockScopedFunctions (default "allow", optional "disallow")?

@nzakas
Copy link
Member

nzakas commented Jan 4, 2024

That's the point of the allowBlockScopedFunctions option name - it allows only block-scoped inner functions. i.e. inner functions in ES6+ strict code.

functions in blocks = inner functions

Ah okay, I see what you mean. I think the implication that "block scoped" refers to ES6+ code only is a little bit hidden in the name.

Maybe blockScopedFunctions (default "allow", optional "disallow")?

That sounds good to me, but I think we need to make it explicit in the documentation that this option allows functions in blocks only in ES6+ contexts and in others it does not.

@Tanujkanti4441
Copy link
Contributor Author

Tanujkanti4441 commented Jan 4, 2024

So can we go with the option name blockScopedFunctions that will have two string options allow and disallow where allow will let the use of inner-function (not variable) in ES6 + strict mode? And default will be allow

@mdjermanovic
Copy link
Member

So can we go with the option name blockScopedFunctions that will have two string options allow and disallow where allow will let the use of inner-function (not variable) in ES6 + strict mode? And default will be allow

Yes, exactly that.

@Tanujkanti4441
Copy link
Contributor Author

Should we still consider this change as a breaking change as it just adding a new option and default behavior is same as current?

lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Jan 5, 2024

This is still a change to the default behavior, isn't it? Doesn't the current default always flag functions inside of blocks, and the new default would be to only flag functions inside of blocks in ES <6?

(Note: This probably doesn't matter because we'll release this as part of v9 regardless.)

docs/src/_data/rules.json Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
tests/lib/rules/no-inner-declarations.js Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
docs/src/rules/no-inner-declarations.md Outdated Show resolved Hide resolved
Comment on lines 219 to 223
::: incorrect { "sourceType": "script" }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint-env es6*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
::: incorrect { "sourceType": "script" }
```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint-env es6*/
::: incorrect { "sourceType": "script", "ecmaVersion": 2015 }
```js
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "disallow" }]*/

Configuration comment was missing the string option (["error", { blockScopedFunctions: "disallow" }] would be an invalid configuration for this rule).

Also, /* eslint-env */ comments do not apply anymore, but we can pass "ecmaVersion": 2015 to the Playground.


:::

Example of **correct** code for this rule with `{ blockScopedFunctions: "disallow" }` option with `ecmaVersion: 6`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Example of **correct** code for this rule with `{ blockScopedFunctions: "disallow" }` option with `ecmaVersion: 6`:
Example of **correct** code for this rule with `{ blockScopedFunctions: "disallow" }` option with `ecmaVersion: 2015`:

Comment on lines 252 to 256
::: correct { "sourceType": "script" }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint-env es6*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
::: correct { "sourceType": "script" }
```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint-env es6*/
::: correct { "sourceType": "script", "ecmaVersion": 2015 }
```js
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "disallow" }]*/

/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint-env es6*/

"use strict";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"use strict" doesn't seem necessary in this example?


:::

Example of **correct** code for this rule with `{ blockScopedFunctions: "allow" }` option with `ecmaVersion: 6`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Example of **correct** code for this rule with `{ blockScopedFunctions: "allow" }` option with `ecmaVersion: 6`:
Example of **correct** code for this rule with `{ blockScopedFunctions: "allow" }` option with `ecmaVersion: 2015`:

Comment on lines 271 to 275
::: correct { "sourceType": "script" }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint-env es6*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
::: correct { "sourceType": "script" }
```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint-env es6*/
::: correct { "sourceType": "script", "ecmaVersion": 2015 }
```js
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "allow" }]*/

Comment on lines 307 to 308
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint-env es6*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint-env es6*/
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "allow" }]*/

```

:::

## When Not To Use It

The function declaration portion rule will be rendered obsolete when [block-scoped functions](https://bugzilla.mozilla.org/show_bug.cgi?id=585536) land in ES6, but until then, it should be left on to enforce valid constructions. Disable checking variable declarations when using [block-scoped-var](block-scoped-var) or if declaring variables in nested blocks is acceptable despite hoisting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The function declaration portion rule will be rendered obsolete when [block-scoped functions](https://bugzilla.mozilla.org/show_bug.cgi?id=585536) land in ES6, but until then, it should be left on to enforce valid constructions. Disable checking variable declarations when using [block-scoped-var](block-scoped-var) or if declaring variables in nested blocks is acceptable despite hoisting.
By default, this rule disallows inner function declarations only in contexts where their behavior is unspecified and thus inconsistent (pre-ES6 environments) or legacy semantics apply (non-strict mode code). If your code targets pre-ES6 environments or is not in strict mode, you should enable this rule to prevent unexpected behavior.
In ES6+ environments, in strict mode code, the behavior of inner function declarations is well-defined and consistent - they are always block-scoped. If your code targets only ES6+ environments and is in strict mode (ES modules, or code with `"use strict"` directives) then there is no need to enable this rule unless you want to disallow inner functions as a stylistic choice, in which case you should enable this rule with the option `blockScopedFunctions: "disallow"`.
Disable checking variable declarations when using [block-scoped-var](block-scoped-var) or if declaring variables in nested blocks is acceptable despite hoisting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@mdjermanovic
Copy link
Member

Can you also add an entry for this change in the v9 migration guide?

@Tanujkanti4441
Copy link
Contributor Author

Can you also add an entry for this change in the v9 migration guide?

If no problems can i add it in a follow up PR? just having some issues in rebasing

::: incorrect { "sourceType": "script", "ecmaVersion": 2015 }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "disallow" }]*/

Without the string option, this would be an invalid configuration for this rule.

::: correct { "sourceType": "script", "ecmaVersion": 2015 }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "disallow" }]*/
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "disallow" }]*/

::: correct { "sourceType": "script", "ecmaVersion": 2015 }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "allow" }]*/

::: correct { "sourceType": "module" }

```js
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*eslint no-inner-declarations: ["error", { blockScopedFunctions: "allow" }]*/
/*eslint no-inner-declarations: ["error", "functions", { blockScopedFunctions: "allow" }]*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@mdjermanovic
Copy link
Member

Can you also add an entry for this change in the v9 migration guide?

If no problems can i add it in a follow up PR? just having some issues in rebasing

Fine with me 👍

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Leaving open for @nzakas to verify.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Completely ok with updating the migration guide in a separate PR. Thanks for all of your work on this.

@nzakas nzakas merged commit 701f1af into eslint:main Jan 9, 2024
17 checks passed
@Tanujkanti4441 Tanujkanti4441 deleted the new-default branch January 9, 2024 23:55
bmish added a commit to bmish/eslint that referenced this pull request Jan 16, 2024
* main:
  chore: Split Docs CI from core CI (eslint#17897)
  fix: Ensure config keys are printed for config errors (eslint#17980)
  chore: delete relative-module-resolver.js (eslint#17981)
  docs: migration guide entry for `no-inner-declarations` (eslint#17977)
  docs: Update README
  feat: maintain latest ecma version in ESLint (eslint#17958)
  feat!: no-inner-declaration new default behaviour and option (eslint#17885)
  feat: add `no-useless-assignment` rule (eslint#17625)
  fix: `no-misleading-character-class` edge cases with granular errors (eslint#17970)
  feat: `no-misleading-character-class` granular errors (eslint#17515)
  docs: fix number of code-path events on custom rules page (eslint#17969)
  docs: reorder entries in v9 migration guide (eslint#17967)
  fix!: handle `--output-file` for empty output when saving to disk (eslint#17957)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Rule Change: Update no-inner-declaration and remove from eslint:recommended
3 participants