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: Add ignoreOnInitialization option to no-shadow rule #14963

Merged
merged 27 commits into from Feb 25, 2022

Conversation

boutahlilsoufiane
Copy link
Contributor

@boutahlilsoufiane boutahlilsoufiane commented Aug 22, 2021

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[X] Changes an existing rule

What changes did you make? (Give an overview)

Fix the issue here: #12687

Adding a new option ignoreOnInitialization to prevent reporting variables on initialization statements.
This is an example:

const person = people.find(person => person.name === "John");

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

No.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Aug 22, 2021
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 24, 2021
Copy link
Member

@nzakas nzakas left a comment

Functionally looks good. I just left some comments on how to clarify what is happening.

CHANGELOG.md Outdated Show resolved Hide resolved
lib/rules/no-shadow.js Outdated Show resolved Hide resolved
lib/rules/no-shadow.js Outdated Show resolved Hide resolved
lib/rules/no-shadow.js Outdated Show resolved Hide resolved
lib/rules/no-shadow.js Outdated Show resolved Hide resolved
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Aug 24, 2021

Hi @nzakas, thank you so much for reviewing the PR.

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Aug 26, 2021

The requested changes are addressed.

@boutahlilsoufiane boutahlilsoufiane requested a review from nzakas Aug 31, 2021
nzakas
nzakas approved these changes Sep 1, 2021
Copy link
Member

@nzakas nzakas left a comment

LGTM. Just want to give others the chance to review too. Thanks!

lib/rules/no-shadow.js Outdated Show resolved Hide resolved
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 5, 2021

CLA Signed

The committers are authorized under a signed CLA.

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Sep 5, 2021

Hi @mdjermanovic, thank you for the explanation. The requested changes are addressed.

@ota-meshi
Copy link
Contributor

ota-meshi commented Sep 14, 2021

I'm not the maintainer but I was interested in this fix so I looked at your code.
Is this fix intended to suppress reporting if it cannot be overridden (If misused, ReferenceError will occur) ?
If so, I think the following source code may need to be reported.
The following code will no longer be reported with this PR fix, but I don't think users will get a ReferenceError if they run it with const a = 42; removed.

const a = fn(()=>{
    class C {
        fn () {
            const a = 42
            return a
        }
    }
    return new C()
})

What do you think?

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Sep 14, 2021

@ota-meshi I think we should report the source code, I will update the pull request to include your example.

@nzakas
Copy link
Member

nzakas commented Sep 15, 2021

@mdjermanovic can you check to see if your comments were addressed?

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 15, 2021

Comments regarding the behavior of this rule with hoist: "all" are addressed - it should report shadowing regardless of the positions of variable declarations.

My other concern is that it isn't clearly defined what we want to fix (only parameters of functions that are arguments of function calls that are variable initializers should be excluded from shadowing that variable?), and why it should be fixed.

The original example from #12687 is:

const person = people.find(person => person.name === "John");

This is shadowing, because at the moment when the inner person is created in an inner scope, the outer person already exists in the outer scope even though it may not be initialized yet. Furthermore, the outer person can be already initialized - we don't know whether the person => person.name === "John" function will be called right away or stored to be used later - so this shadowing doesn't have to be less potentially dangerous than shadowing a variable that is declared in a declaration that appears before this one. It should be only user's preference whether or not this example should be reported.

Problem is that the "hoist" option doesn't unambiguously specify how this example should be interpreted.

The hoist option has three settings:

  • functions (by default) - reports shadowing before the outer functions are defined.
  • all - reports all shadowing before the outer variables/functions are defined.
  • never - never report shadowing before the outer variables/functions are defined.

Is this "shadowing before the outer variable is defined" or not? If we say that variable definition ends where the initializer ends, then it is "before", so hoist: "functions" or hoist: "never" should not report this example, and we have a false positive. On the other hand, it can be argued that the inner person identifier appears after the outer person identifier so it isn't "before", and therefore this rule works as intended (and this change might be undesirable for some users).

My opinion is that isn't clear if #12687 is a bug or intended behavior, and that we should resolve it by adding a new option rather than changing the current behavior.

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Sep 18, 2021

@mdjermanovic , You are right, the rule is working as expected, the inner variable person is shadowing the outer variable person, but developers are confused by this example:

const person = people.find(person => person.name === "John");

I think we should close the issue #12687 and this PR and add the ambiguous example in the documentation.

For the code source mentioned by @ota-meshi the rule doesn't report it, we need to create a new issue and I will fix it.

@boutahlilsoufiane boutahlilsoufiane changed the title Fix: Remove warning in initialized variables (fixes #12687) Docs: Add invalid ambiguous example in doc (fixes #12687) Sep 19, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 20, 2021

I'm not suggesting that we close #12687, but to consider adding an option for that behavior.

For the code source mentioned by @ota-meshi the rule doesn't report it, we need to create a new issue and I will fix it.

It works in the current version of this rule:

/* eslint no-shadow: error */

const a = fn(()=>{
    class C {
        fn () {
            const a = 42 // 'a' is already declared in the upper scope
            return a
        }
    }
    return new C()
})

Online Demo

@mdjermanovic mdjermanovic added the needs bikeshedding Minor details about this change need to be discussed label Sep 24, 2021
@boutahlilsoufiane boutahlilsoufiane changed the title Docs: Add invalid ambiguous example in doc (fixes #12687) Fix: Add invalid ambiguous example in doc (fixes #12687) Sep 26, 2021
@boutahlilsoufiane boutahlilsoufiane changed the title Fix: Add invalid ambiguous example in doc (fixes #12687) Update: Add invalid ambiguous example in doc (fixes #12687) Sep 26, 2021
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Sep 26, 2021

@mdjermanovic The requested changes are addressed.

@boutahlilsoufiane boutahlilsoufiane changed the title Update: Add invalid ambiguous example in doc (fixes #12687) Update: Adding option for variables on intialization (fixes #12687) Sep 26, 2021
@mdjermanovic mdjermanovic changed the title Update: Adding option for variables on intialization (fixes #12687) feat: Adding option for variables on intialization (fixes #12687) Oct 26, 2021
lib/rules/no-shadow.js Outdated Show resolved Hide resolved
@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Nov 7, 2021

Hi @mdjermanovic, thank you so much for your time reviewing the PR. The requested changes are addressed.

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Feb 23, 2022

Hi @mdjermanovic, thanks a lot for your time and your efforts. The comment is addressed.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks! I'll leave this open for a day in case someone else wants to review it before merging.

nzakas
nzakas approved these changes Feb 25, 2022
Copy link
Member

@nzakas nzakas left a comment

Awesome work. Thanks for your patience and responsiveness.

@mdjermanovic mdjermanovic merged commit 6e2c325 into eslint:main Feb 25, 2022
14 checks passed
@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 25, 2022

@boutahlilsoufiane great work, thanks for contributing!

@boutahlilsoufiane
Copy link
Contributor Author

boutahlilsoufiane commented Feb 25, 2022

Hi @nzakas and @mdjermanovic Thanks a lot for approving the changes. Thanks a lot, @mdjermanovic for your big help, this wasn't going to be merged without you.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.9.0` -> `8.10.0`](https://renovatebot.com/diffs/npm/eslint/8.9.0/8.10.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.10.0`](https://github.com/eslint/eslint/releases/v8.10.0)

[Compare Source](eslint/eslint@v8.9.0...v8.10.0)

#### Features

-   [`6e2c325`](eslint/eslint@6e2c325) feat: Add `ignoreOnInitialization` option to no-shadow rule ([#&#8203;14963](eslint/eslint#14963)) (Soufiane Boutahlil)
-   [`115cae5`](eslint/eslint@115cae5) feat: `--debug` prints time it takes to parse a file ([#&#8203;15609](eslint/eslint#15609)) (Bartek Iwańczuk)
-   [`345e70d`](eslint/eslint@345e70d) feat: Add `onlyOneSimpleParam` option to no-confusing-arrow rule ([#&#8203;15566](eslint/eslint#15566)) (Gautam Arora)

#### Bug Fixes

-   [`cdc5802`](eslint/eslint@cdc5802) fix: Avoid `__dirname` for built-in configs ([#&#8203;15616](eslint/eslint#15616)) (DoZerg)
-   [`ee7c5d1`](eslint/eslint@ee7c5d1) fix: false positive in `camelcase` with combined properties ([#&#8203;15581](eslint/eslint#15581)) (Nitin Kumar)

#### Documentation

-   [`1005bd5`](eslint/eslint@1005bd5) docs: update CLA information ([#&#8203;15630](eslint/eslint#15630)) (Nitin Kumar)
-   [`5d65c3b`](eslint/eslint@5d65c3b) docs: Fix typo in `no-irregular-whitespace` ([#&#8203;15634](eslint/eslint#15634)) (Ryota Sekiya)
-   [`b93af98`](eslint/eslint@b93af98) docs: add links between rules about whitespace around block curly braces ([#&#8203;15625](eslint/eslint#15625)) (Milos Djermanovic)
-   [`ebc0460`](eslint/eslint@ebc0460) docs: update babel links ([#&#8203;15624](eslint/eslint#15624)) (Milos Djermanovic)

#### Chores

-   [`7cec74e`](eslint/eslint@7cec74e) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).2.0 ([#&#8203;15648](eslint/eslint#15648)) (Milos Djermanovic)
-   [`11c8580`](eslint/eslint@11c8580) chore: read `ESLINT_MOCHA_TIMEOUT` env var in Makefile.js ([#&#8203;15626](eslint/eslint#15626)) (Piggy)
-   [`bfaa548`](eslint/eslint@bfaa548) test: add integration tests with built-in configs ([#&#8203;15612](eslint/eslint#15612)) (Milos Djermanovic)
-   [`39a2fb3`](eslint/eslint@39a2fb3) perf: fix lazy loading of core rules ([#&#8203;15606](eslint/eslint#15606)) (Milos Djermanovic)
-   [`3fc9196`](eslint/eslint@3fc9196) chore: include `tests/conf` in test runs ([#&#8203;15610](eslint/eslint#15610)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1190
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
srijan-deepsource pushed a commit to deepsourcelabs/eslint that referenced this pull request May 30, 2022
…4963)

* Fix: Remove warning  in initialized variables (fixes eslint#12687)

* Fix: Remove warning  in initialized variables (fixes eslint#12687)

* Fix: Remove warning  in initialized variables (fixes eslint#12687)

* Docs: add invalid ambiguous example in doc (fixes eslint#12687)

* Docs: add invalid ambiguous example in doc (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* Fix: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)

* feat: Adding option for variables on intialization (fixes eslint#12687)
srijan-deepsource added a commit to deepsourcelabs/eslint that referenced this pull request May 30, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 bug ESLint is working incorrectly feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants