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: fix logic for top-level this in no-invalid-this and no-eval #15712

Merged
merged 3 commits into from Mar 25, 2022

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Mar 21, 2022

Prerequisites checklist

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

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

This bug fix produces more warnings from the no-eval rule, and thus this change is marked as feat.

Tell us about your environment:

  • ESLint Version: v8.11.0
  • Node Version: 16.14.0
  • npm Version: 8.3.1

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, 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.

Online Demo

/* eslint no-invalid-this: "error", no-eval: "error" */

"use strict";

this.eval("alert('foo')");

What did you expect to happen?

No no-invalid-this errors, one no-eval error.

At the top level of scripts, this always refers to the global object, regardless of "use strict".

In browsers, the above code alerts foo, so this is valid and this.eval() is an eval call.

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

One no-invalid-this error, no no-eval errors.

  5:1  error  Unexpected 'this'  no-invalid-this

✖ 1 problem (1 error, 0 warnings)

What changes did you make? (Give an overview)

Fixed the no-invalid-this to treat this at the top level of scripts as valid, regardless of strict mode.

Fixed the no-eval rule to treat this.eval at the top level of scripts as access to the global eval, regardless of strict mode.

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

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Mar 21, 2022
@aladdin-add
Copy link
Member

aladdin-add commented Mar 23, 2022

"use strict";

this;

I just had thought this should be undef. lol.

Copy link
Member

@aladdin-add aladdin-add left a comment

💯

Copy link
Member

@btmills btmills left a comment

The change to no-eval LGTM, but I believe the existing behavior of no-invalid-this was already correct. In your demo link, I would expect errors from both rules. no-invalid-this "Disallows this keywords outside of classes or class-like objects." At the top level of a script, from the perspective of no-invalid-this, it's still outside of a class or class-like object, so this is still invalid even though it does have a value. The name of the rule is somewhat poor here, but the docs do a better job of conveying its purpose, which is consistent with the existing behavior.

@mdjermanovic
Copy link
Member Author

mdjermanovic commented Mar 24, 2022

I believe the no-invalid-this rule only intends to report this that is undefined and thus "invalid". Otherwise, it wouldn't check whether or not functions are in strict mode.

For example:

// not strict

function foo() {
    this.alert('foo');
}

If the intent was to "disallow this keywords outside of classes or class-like objects.", then the above this would be reported regardless of "use strict", because it is considered to be outside of classes or class-like objects. But it isn't reported, because in this case this is the global object. If we add "use strict", that this becomes undefined and it is reported by this rule.

The same applies to top-level this. The following code is not reported by no-invalid-this:

// not strict

this.alert('foo');

But the following is:

"use strict";

this.alert('foo');

That is the bug, as there's no difference between these two cases, both this refer to the global object.

@mdjermanovic
Copy link
Member Author

mdjermanovic commented Mar 24, 2022

In my opinion, we should change "Disallows this keywords outside of classes or class-like objects." to something like "Disallows use of this where it is undefined.". By the current behavior and implementation details, that was the intent for this rule. Otherwise, it wouldn't allow the following code:

/* eslint no-invalid-this: "error" */

this.alert('foo');

() => {
    this.alert('foo');
}

function foo() {
    this.alert('foo');
}

@mdjermanovic mdjermanovic marked this pull request as draft Mar 24, 2022
Copy link
Member

@btmills btmills left a comment

Based on what you've shared, the current behavior of no-invalid-this does appear to be more like "where this is not undefined" than "inside classes and class-like objects", and this change as written is consistent with the current behavior, so LGTM. Your suggested wording change to the stated purpose in the docs also LGTM. Since the behavior already seems to have drifted from the docs somewhat, I'm fine updating the docs as part of this bug fix or leaving it for a follow-up PR.

@mdjermanovic mdjermanovic marked this pull request as ready for review Mar 25, 2022
@mdjermanovic
Copy link
Member Author

mdjermanovic commented Mar 25, 2022

I'll close-reopen to try to run checks.

@mdjermanovic
Copy link
Member Author

mdjermanovic commented Mar 25, 2022

I updated the docs for no-invalid-this to clarify this rule.

I also noticed that no-eval doesn't work well with this in arrow functions, and wanted to check if that's due to changes from this PR, but it isn't. I'll fix that in another PR.

@btmills btmills merged commit 685a67a into main Mar 25, 2022
14 checks passed
@btmills btmills deleted the top-level-this branch Mar 25, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 27, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.11.0...v8.12.0)

#### Features

-   [`685a67a`](eslint/eslint@685a67a) feat: fix logic for top-level `this` in no-invalid-this and no-eval ([#&#8203;15712](eslint/eslint#15712)) (Milos Djermanovic)

#### Chores

-   [`18f5e05`](eslint/eslint@18f5e05) chore: padding-line-between-statements remove useless `additionalItems` ([#&#8203;15706](eslint/eslint#15706)) (Martin Sadovy)

</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/1256
Reviewed-by: Epsilon_02 <epsilon_02@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
…slint#15712)

* feat: fix logic for top-level `this` in no-invalid-this and no-eval

* add test with `this.eval` to no-invalid-this

* update docs to clarify `no-invalid-this`
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 Sep 22, 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 Sep 22, 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

3 participants