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

Improve no-func-assign docs and add tests #13705

Closed
bendtherules opened this issue Sep 20, 2020 · 12 comments
Closed

Improve no-func-assign docs and add tests #13705

bendtherules opened this issue Sep 20, 2020 · 12 comments

Comments

@bendtherules
Copy link

@bendtherules bendtherules commented Sep 20, 2020

Tell us about your environment

Eslint demo page with Eslint v7.9.0 - Demo URL

  • ESLint Version: v7.9.0
  • Node Version: Doesn't matter (web hosted)
  • npm Version: Doesn't matter (web hosted)

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using? - default

Please show your full configuration:

Configuration
{
    "parserOptions": {
        "ecmaVersion": 12,
        "sourceType": "script",
        "ecmaFeatures": {}
    },
    "rules": {
        "constructor-super": 2,
        "for-direction": 2,
        "getter-return": 2,
        "no-async-promise-executor": 2,
        "no-case-declarations": 2,
        "no-class-assign": 2,
        "no-compare-neg-zero": 2,
        "no-cond-assign": 2,
        "no-const-assign": 2,
        "no-constant-condition": 2,
        "no-control-regex": 2,
        "no-debugger": 2,
        "no-delete-var": 2,
        "no-dupe-args": 2,
        "no-dupe-class-members": 2,
        "no-dupe-else-if": 2,
        "no-dupe-keys": 2,
        "no-duplicate-case": 2,
        "no-empty": 2,
        "no-empty-character-class": 2,
        "no-empty-pattern": 2,
        "no-ex-assign": 2,
        "no-extra-boolean-cast": 2,
        "no-extra-semi": 2,
        "no-fallthrough": 2,
        "no-global-assign": 2,
        "no-import-assign": 2,
        "no-inner-declarations": 2,
        "no-invalid-regexp": 2,
        "no-irregular-whitespace": 2,
        "no-misleading-character-class": 2,
        "no-mixed-spaces-and-tabs": 2,
        "no-new-symbol": 2,
        "no-obj-calls": 2,
        "no-octal": 2,
        "no-prototype-builtins": 2,
        "no-redeclare": 2,
        "no-regex-spaces": 2,
        "no-self-assign": 2,
        "no-setter-return": 2,
        "no-shadow-restricted-names": 2,
        "no-sparse-arrays": 2,
        "no-this-before-super": 2,
        "no-undef": 2,
        "no-unexpected-multiline": 2,
        "no-unreachable": 2,
        "no-unsafe-finally": 2,
        "no-unsafe-negation": 2,
        "no-unused-labels": 2,
        "no-unused-vars": 2,
        "no-useless-catch": 2,
        "no-useless-escape": 2,
        "no-with": 2,
        "require-yield": 2,
        "use-isnan": 2,
        "valid-typeof": 2
    },
    "env": {}
}

Only actual change from default config is - disabled "no-func-assign"

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

var a = function hello() {
  'use strict';
  hello = 123; // should throw error here
};
a()

Ran demo web version - Demo URL

What did you expect to happen?

I have disabled "no-func-assign" rule.
On the line "hello = 123" within named function expression, this will always throw a runtime JS error saying TypeError: Assignment to constant variable. That means, this is a assignment to a (implicitly created) immutable binding for the function name "hello". "no-const-assign" rule should also catch this error, irrespective of "no-func-assign" - because this is a const reassignment.

The spec says that for named function expressions, there is a implicitly created extra scope above the function local scope which has immutable binding for funcName = funcObject. So, this means -

A. This code should not be allowed

var a = function hello() {
  'use strict';
  hello = 123; // reassign const
};

B. but this should be allowed -

var a = function hello() {
  'use strict';
  const hello = 123; // creates new const
};

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

No lint error or warning (lint-free)

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

Yes

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Sep 20, 2020

Thanks for the issue.

IMO, this is out of the scope of this rule. This rule checks for the const variables alone.
According to the rule's docs

Disallow modifying variables that are declared using const (no-const-assign)

no-func-assign does catches the same.

Let me know if I am missing something.

@bendtherules
Copy link
Author

@bendtherules bendtherules commented Sep 20, 2020

I understand your point about the scope - that it is only supposed to catch const s declared explicitly by the user.

But what about the ones implicitly created by the language? This is one such example. Is there any other rule for that?

Also no-func-assign might be moved to warning in some configs, because it alone does not cause a error.
On the other hand, this example always throws a error.

This throws the exact same error that assigning to a const throws. User would expect no-const-assign to protect against all such errors.

@bendtherules
Copy link
Author

@bendtherules bendtherules commented Sep 20, 2020

Ok, i take back my words.
Seems like in all other implicitly immutable cases (class name, imported var) - it shows a specific error instead of no-const-assign.

Only suggestion in that case is to add more strict words to no-func-assign doc saying that it can throw error also, not just a clean code issue

Does that make sense?

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Sep 20, 2020

Yeah, I think there can be some more effective docs as an example similar to the one you shared.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 20, 2020

Only suggestion in that case is to add more strict words to no-func-assign doc saying that it can throw error also, not just a clean code issue

Sounds good to me!

Marked as an accepted issue to improve documentation for no-func-assign. In particular, to add named function expressions.

The actual document doesn't even mention named function expressions and doesn't have any examples with them. Also, there are no test cases, so it's possible that the original intention was to check only function declarations:

This rule disallows reassigning function declarations.

Either way, this rule checks function expressions from the start (v0.0.9), this behavior does make sense, so we should update the docs to reflect the actual behavior.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 2, 2020

@bendtherules would you like to submit a pull request to improve documentation for no-func-assign?

@bendtherules
Copy link
Author

@bendtherules bendtherules commented Oct 2, 2020

Yes, i would like to add the pr. But it will probably take some time.
If someone would like to pick it up quickly, i am good with that 👍

@bendtherules bendtherules changed the title no-const-assign doesn't warn when you reassign name binding within func expression Improve no-func-assign docs and add tests Oct 6, 2020
@snitin315
Copy link
Contributor

@snitin315 snitin315 commented Oct 15, 2020

@beaugunderson @bendtherules can I send a PR if you haven't already started working on this one?

@bendtherules
Copy link
Author

@bendtherules bendtherules commented Oct 15, 2020

Feel free to pick it up. I have not done any work on this. (in case you meant to ask me)

snitin315 added a commit to snitin315/eslint that referenced this issue Oct 21, 2020
snitin315 added a commit to snitin315/eslint that referenced this issue Oct 22, 2020
snitin315 added a commit to snitin315/eslint that referenced this issue Oct 22, 2020
mdjermanovic pushed a commit that referenced this issue Oct 22, 2020
* Chore: add test case for no-func-assign (refs #13705)

* Chore: Update tests/lib/rules/no-func-assign.js
mdjermanovic pushed a commit that referenced this issue Oct 22, 2020
@bendtherules
Copy link
Author

@bendtherules bendtherules commented Oct 23, 2020

While the PR adds a example of incorrect code, i think the whole scope is not holistically covered.

  1. The description text should say that in some case, this will actually cause a error and is not just a styling thing.
  2. Add tests

Can we reopen this? (can't see button)

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 23, 2020

We added an example in #13777, and a test in #13783.

@bendtherules I agree that the docs and the tests can be further improved, feel free to submit a PR!

@bendtherules
Copy link
Author

@bendtherules bendtherules commented Oct 24, 2020

Ahh, i missed the tests pr. Then it looks better, maybe some text can be changed in the description is all. I'll try to raise a pr for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.