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

Rule Proposal/Extension: Blacklist of bad identifier names #3358

Closed
keithamus opened this Issue Aug 11, 2015 · 20 comments

Comments

Projects
None yet
5 participants
@keithamus
Copy link
Contributor

commented Aug 11, 2015

The new id-length and id-match rules are fantastic for enforcing good quality identifier names, but they have room for improvement - one obvious win would be to introduce an id-blacklist rule, or extend id-match to include a blacklist option - for badly formed identifier names which still manage to slip by id-match and id-length.

Example:

id-blacklist allows you to blacklist certain identifier names, meaning that they cannot be used without a warning:

"id-blacklist": [
    2,
    "err",
    "data",
    "num",
    "temp",
],

with the above config, the following will be warnings:

function handleFileRead(err, fileContents) {
  //                    ^ err is blacklisted
}
function processFileContents(data) {
   //                        ^ data is blacklisted
}
var entityList = {
  num: 3, // < num is blacklisted
};
var temp = 1; // < temp is blacklisted
@eslintbot

This comment has been minimized.

Copy link

commented Aug 11, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@BYK

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

This is a good idea. I feel like we should merge all these three rules actually since id-match theoretically covers id-length and this one can be baked in too.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2015

I was actually working on a plugin to do this, before the rule actually made it into eslint core. I found that just using the regexp was sufficient, and id-length wasn't needed - just simply make the regexp like ^[a-z]{3,20}$. Having said that I think I prefer how explicit id-length is, not to mention it gives better error messages than id-match does.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2015

Just for posterity, here was the syntax style for my plugin:

"improper-variables": [
    1,
    "^[a-zA-Z]{3,20}$",
    {
        blacklist: [
            "err",
            "data",
            "count",
            "num",
            "string",
            "number",
            "temp",
            "val"
        ],
        whitelist: ["i", "x", "y"]
    }
]

@gyandeeps gyandeeps added the triage label Aug 11, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

I think it's a good idea, question is, where would these IDs be restricted? All variables in all scores? What about properties? Arguments?

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2015

Everywhere where id-match and id-length currently restrict identifiers; so yes - variables, properties, function arguments, function names.

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

Sounds good, do you want to work on it?

@nzakas nzakas added enhancement rule accepted and removed triage labels Aug 12, 2015

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2015

@nzakas What's the plan? To create a new rule called id-blacklist or to merge the three rules into one?

@nzakas

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

Create a new rule.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

@nzakas I realise this has been sitting here for a while, I've been a little busy but I've just had a chance to revisit this...

However, given the new comments in #3488 - which talks about a whitelist of acceptable identifier names, the conclusion was slightly different (an option rather than a new rule).

I'd suggest that both be options to id-match - as the options allow and deny. If that's acceptable I'll formulate a PR for both this issue and #3488 shortly.

I also think we automatically whitelist all of the identifiers in ecmascript, such as Array, Object, JSON, etc - and maybe some like XMLHttpRequest. Thoughts on this?

@nzakas

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

What if the rule had both allow and deny?

I don't believe we can automatically whitelist anything, as people may not want to allow built-ins as well.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

Are you asking what should be done if allow and deny contained the same identifier? I'd say we just pick a precedent ant document it - e.g. anything in allow is explicitly allowed, even if is in deny.

My suggestion to automatically whitelist was due to the fact that there are many inconsistencies between ecmascript and the DOM, for example JSON is a ES global which is an acronym and so all uppercased, however XMLHttpRequest does not capitalise the Http part, but does capitalise the XML part. These are potential breakages for developers who may have strict enough id-match regexps to fail on these - but they have no choice about the names of these, and so it is just an increased burden for them to have to add them as-and-when to the whitelist.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

Having said all of that, I'm keen to get this PRd ASAP, so for now am I good to go ahead adding allow and deny options to id-match?

@nzakas

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

No. I'm asking what happens when both allow and deny are present? You can't actually apply both a whitelist and a blacklist at the same time, so I'm afraid we would get a lot of bug reports.

I'm not convinced we should have these two options in the same rule, I'm still thinking a separate blacklist rule makes more sense.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

That is the same question that I assumed, if you have a rule like:

"id-match": [2, { "allow": ["foo"], "deny": ["foo"] }]

I'd say we just pick a precedent ant document it - e.g. anything in allow is explicitly allowed, even if is in deny.

Alternatively we could just throw an error early (Error: Duplicate identifier 'foo' exists in both id-match allow and id-match deny). If someone is putting the same identifier in both allow and deny, they're already doing something pretty wrong and should probably be notified of it. I don't see how having it as a separate rule is going to make it any easier:

"id-match": [2, { "allow": ["foo"] }],
"id-blacklist": [2, "foo"]

or

"id-match": [2],
"id-whitelist": [2, "foo"]
"id-blacklist": [2, "foo"]

are going to produce the same kind of errors. If anything these will be harder to enforce/track-down as they are separate rules and don't share context.

@nzakas

This comment has been minimized.

Copy link
Member

commented Sep 29, 2015

We can't do cross-rule configuration validation.

Having a separate rule means you are being explicit. id-match isn't really for blocking specific identifiers, it's for defining a set of allowable identifiers. An id-blacklist for would then work to further limit that set.

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2015

Okay, makes sense, I see your point. I'll get to work on this.

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Dec 28, 2015

@keithamus Are you still interested in this rule and/or working on this rule?

@keithamus

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2015

Hey @gyandeeps yeah, I'll make a PR soon

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

Sounds good. Thanks.

keithamus added a commit to keithamus/eslint that referenced this issue Jan 13, 2016

keithamus added a commit to keithamus/eslint that referenced this issue Jan 13, 2016

keithamus added a commit to keithamus/eslint that referenced this issue Jan 13, 2016

keithamus added a commit to keithamus/eslint that referenced this issue Jan 14, 2016

keithamus added a commit to keithamus/eslint that referenced this issue Jan 14, 2016

keithamus added a commit to keithamus/eslint that referenced this issue Jan 14, 2016

ilyavolodin added a commit that referenced this issue Jan 14, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.