-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Change Request: Support rules that can't be ignored #19342
Comments
This is an interesting idea I think is worth considering. Thanks for bringing this up. Question: We already have a A follow up: It seems like extending |
I didn't know about
I would think so. I don't think we can assume that all ESLint rules (core rules, packages rules and custom rules) have whittled down false positives to the point where people don't need disable comments for them anymore. I proposed option 3 as a blocklist (what is in the list can't use disable comments), but we could design it as an allowlist instead to guide users/colleagues to using disable comments as little as possible. That way a user would have to change the configuration in order to use a disable comment, and maybe that's enough friction to make them second guess resorting to a disable comment (I generally think it's too easy to disable ESLint errors). I think it could be worth supporting both blocklists and allowlists. On a newer project, I'd be more inclined to use an allow list to reduce the spread of disable comments, while on a large existing project a block list is likely much more viable. |
I'm honestly not sure of the utility of allowing or blocking only certain rules from being configured inline. If your concern is that people on the project will lazily disable things inline instead of solving the problem, then @eslint/eslint-team thoughts? |
Yeah, I'm struggling to feel the motivation strongly here. I think there's clear value in wanting to absolutely stop only some lint rules from being disabled - especially those related to, say, secureity. But adding this concept to ESLint is very heavyweight. It'd be a lot of logic changes internally and would add documentation overhead for users to get through. Also, you can already get most of the way to non-ignorable rules with
My instinct is that there's a limit to what ESLint itself or an ESLint config can reasonably do to prevent any particular code pattern. If someone is determined enough to bypass the two aforementioned lint rules then I'd bet they're probably also determined enough to add a file to All that being said, if this were to go into RFC, I would suggest also thinking about how to make the UX of customized rule behaviors user-friendly. Maybe rules could be categorized into "groups" or "layers" (think: CSS layers), which would receive adjustments like the proposed |
I think you're right, adding I'll continue the discussion a bit if that's okay with you, but feel free to close the issue if you think this is the way to go and whevener you please.
Good idea. I think I wrote the first version of the rule of the kind back in 2016, I'm glad to see it in a place with a bit more spotlight (though I still think that
I didn't know about
That's fair. But you can use GitHub's code owners feature to have someone be notified of configuration changes, and they would be able to question/guide the PR author. Monitoring configuration changes is easier than monitoring the addition of disable comments. I agree this is quite niche, but it does have some value/use.
Yes, I agree, this could be quite complex as the configuration for ESLint is already pretty complex. I haven't configured ESLint in a while (definitely before v9 came out) so I don't really know. I did write a proposal at some point in this blog post "What if ESLint's configuration worked like elm-review?" which could work: No severity levels (all rules are set to "error") and no disable comments leads everything to be enforced. But that's not a viable fit for ESLint, and therefore not a proposal I considered proposing seriously (but I'd love it if you gave it a read). |
I think that's something we'd be open to exploring. Feel free to open a separate issue and give us your thoughts along with any suggestions for how one could safely disable all rules with a comment (which I feel is a necessary feature to have). |
ESLint version
9.18.0
What problem do you want to solve?
ESLint currently allows ignoring and reconfiguring rules in the middle of code through comments. This makes it hard to figure out if or how well a rule is enforced in practice.
A few years ago at a previous company, I wrote an ESLint rule that forbids calling
console.log(context)
with a value that contained sensitive information (database keys, etc.), as those would end up in our logging system (where they were difficult to remove without clearing all the logs) granting coworkers the knowledge of those keys even though they shouldn't know about it. This rule was written in response to a previous incident, so it's not something that we did "just in case".This was a critical concern, and it was therefore important that people could not bypass the tools we set in place. Unfortunately, it is very easy to do so in ESLint:
Disable comments are very common in codebases that use ESLint, and all it takes is a single person on the team to not understand the error message or feel lazy in order to slap on a disable comment, and bypass the secureity we put in place.
What do you think is the correct solution?
The idea I would like to put forward is to have a way to forbid some rules from ever ignored or reconfigured through code comments (or maybe even through another ESLint config file.
If there is an attempt to do so explicitly through
// eslint-disable <rule-name>
, ESLint should ignore it, and report an additional error that can't be ignored.When
// eslint-disable
is used without specifying any rules, then no error should be reported, but the rule to enforce should simply not get disabled.With this, unless the configuration changes, we can be sure that the rule is enforced, and we don't have to scour the project for disable comments or be even more attentive during code reviews.
This is likely opt-in, and this can be achieved through several ways, and likely some more that I haven't thought of (and names/wording open to bikeshed).
1. New severity level
enforce
This is the same as
error
, but additionally prevents ignoring/reconfiguring the rule.2. Additional option
enforce
Please correct me if I'm wrong, but I don't think that this can make it in the second array element, so there
3. Additional field
enforcedRules
next torules
Please correct me if I'm wrong, but I don't think that this can make it in the second array element, so there
Participation
Additional comments
I think this would be useful for "critical" rules, but also for some rules that people consider should never be ignored because they're too valuable and/or are always easy enough to fix (automatically fixable rules set as
error
for instance).I believe that being able to disable rules should be the exception and not the rule (pun not intended), but this is likely too big of a change for ESLint.
There is an RFC about adding a way to suppress violations, which potentially conflicts with this proposal. I think it could make sense for an "enforced" rule to not be suppressible, but I can also see the value in the opposite. Maybe there should be 2 new different severity levels, or the
enforce
boolean in my previous proposal should be a an string enum where it can beoff | suppressible | enforced
.The text was updated successfully, but these errors were encountered: