id-blacklist catches calls to Number #12567
Comments
hnipps
added
bug
|
Hi @hnipps, thanks for the issue. This is in your configuration: Based on this configuration, I think it's correct that the |
|
@platinumazure the documentation seems to say that the rule will only flag instances where the blacklisted id is assigned a value and will not flag instances when existing functions are called or property names are referenced. In my case I'm not assigning anything to Did I misunderstand the docs? |
|
@hnipps Thanks for clarifying and for drawing my attention to the documentation. I can see how this is confusing. There's a comment in the documentation after a few examples that says: "all function calls are ignored". However, in those examples, the identifier is either a standalone identifier, or the property of a member expression. For whatever reason, identifiers that are objects of the member expression are always reported. I'm not sure what the correct behavior should be here. 😄 I am hoping another team member can provide historical context. |
|
Thanks @platinumazure . Some more context: I came across this issue while migrating from TSLint to ESLint using We were using the TSLint "variable-name" rule with the "ban-keywords" option and I suppose "id-blacklist" is the ESLint equivalent of that rule. |
|
I'm also missing the historical context here, but I agree that this seems like a bug. |
kaicataldo
added
evaluating
|
It seems that each of the 'naming' rules has its own logic on which Rules: Would it make sense to have a consistent logic, e.g., to check only places where variables/properties are created in all 5 rules? Or maybe to make one |
|
It seems the "ban-keywords" option in the TSLint "variable-name" rule should actually be translated to a combination of ESLint rules: With these results enabled, it gives me the expected result and works equivalent to the TSLint rule. module.exports = {
"env": {
"es6": true,
"node": true
},
"parser": "@typescript-eslint/parser",
"parserOptions": {
"project": "tsconfig.json",
"sourceType": "module"
},
"plugins": [
"@typescript-eslint"
],
"rules": {
"no-redeclare": [
"error",
],
"no-shadow": [
"error",
{
"builtinGlobals": true
}
],
}
};
const Object = 0; // 'Object' is already declared in the upper scope
class Number {} // 'Number' is already declared in the upper scope
function foo(undefined: number) {} // 'undefined' is already declared in the upper scopeThis is probably an issue with the tslint-to-eslint-config scripts. But "id-blacklist" is giving me strange behaviour as well, if I change the eslint rules to have the "id-blacklist" rule, then suddenly perfectly valid code is flagged as an error.
...
"rules": {
"id-blacklist": [
"error",
"undefined",
],
}
...
let foo = undefined; // Identifier 'undefined' is blacklistedSuddenly, using It seems that "id-blacklist" is too aggressive in its flagging, and not a correct translation of the "ban-keywords" option in the TSLint "variable-name" rule. |
|
This also looks like a bug as the rule effectively restricts importing /*eslint id-blacklist: ["error", "foo"]*/
import { foo as bar } from "mod"; // error: Identifier 'foo' is blacklisted.Since the rule is in the |
|
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
|
Reopening since this is likely a bug we should fix. |
|
Thanks @kaicataldo 👍 I also think this is a valid bug. I am hitting the issue with valid uses of |
|
Unfortunately, it looks like there wasn't enough interest from the team Thanks for contributing to ESLint and we appreciate your understanding. |
|
Just facing this issue and thought the rule was focused on naming variables instead of value identifiers as stated by author. |
|
Since it isn't clear whether this is a bug or not, maybe we could add an option to ignore all references to global variables (except for those declared in the same file). |
|
I still think this is a bug. As stated above, the documentation seems to pretty clearly indicate that this shouldn't warn. I would argue for this being a patch fix, but would love to hear from some other team members as well. |
|
The idea with an option was to have both behaviors in the case that someone prefers the actual one, but I also think this is a bug and would agree to just change the default behavior. This rule already allows all
In my opinion, the intention was to allow the use of built-in/environment/third party globals (which are all names "you do not have control over"), but the assumption was they're all functions. |
kaicataldo
removed
the
evaluating
kaicataldo
added
the
accepted
|
I'm going to mark this as accepted, since multiple team members agree that this is a bug. |
|
@kaicataldo thoughts about rewriting this rule to check and report only places where variables are created (variable declarations, parameters, imports...)? /* eslint id-blacklist: ["error", "foo"] */
var foo; // report
foo = 1; // don't reportThat would fix this issue by itself. Properties would have a separate logic. A disadvantage of this approach might be that it would stop reporting identifiers in experimental syntax: /* eslint id-blacklist: ["error", "foo"] */
class A {
#foo; // this wouldn't be reported anymore
}though, that could be a good thing because there are some false positives with /* eslint id-blacklist: ["error", "foo"] */
a = b?.foo; // this is an error in the actual version, but it shouldn't beIt would also stop reporting identifiers in typescript specific syntax, but typescript users have an alternative in @typescript-eslint/naming-convention. |
|
@mdjermanovic in |
In that case, this is also a false positive in the actual version. |
|
I think it makes sense to only warn on cases where the author has complete control over naming. It would be great to have My vote would be to treat this issue as a bug fix and then to create a new issue/RFC where we can discuss the direction we'd like to go with rules that enforce |
I think we should combine these 3 +
Bug fix to just ignore identifiers that represent references to global variables, for the start? |
|
I used the latest eslint package with version 7.0.0, but it seems we still have the problem that the rule reports error on the following case: |
This was supposed to be fixed, and I can't reproduce it in our online demo v7.0.0 now: /*eslint id-blacklist: ["error", "undefined"] */
let foo = undefined; // no error
function bar() {
let foo = undefined; // no error
}
function baz() {
let undefined; // error
}@kuyezhiying can you please open a new bug report issue (with all details from the template) so we could check if we have missed something? |
|
@mdjermanovic Thanks for taking the try. Sorry it was my bad, it's indeed fixed in v7.0.0, it works when I use eslint directly. However, I met the problem because I am using gulp-eslint to setup the workflow, I just found that gulp-eslint has the dependency on eslint v6.0.0 and higher versions, but when I install the latest version of gulp-eslint (v6.0.0), the dependent eslint was v6.8.0 which may not containing the fix. |
eslint-deprecated
bot
added
the
archived due to age
Tell us about your environment
What parser (default, Babel-ESLint, etc.) are you using?
@typescript-eslint/parser
Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.
Run via VSCode ESLint plugin, should be something like:
VSCode ESLint settings:
{ "eslint.lintTask.options": "./src/**/*.ts", "eslint.lintTask.enable": true, "eslint.options": { "configFile": ".eslintrc.js" }, "eslint.validate": [ "javascript", "typescript" ], "eslint.enable": true }What did you expect to happen?
The call to
Number.parseInt()would not be caught by the ESLintid-blacklistrule.What actually happened? Please include the actual, raw output from ESLint.
The call to
Number.parseInt()is caught by eslint -Identifier 'Number' is blacklisted.Are you willing to submit a pull request to fix this bug?
No
The text was updated successfully, but these errors were encountered: