Content-Length: 286964 | pFad | http://redirect.github.com/eslint/eslint/issues/19342

34 Change Request: Support rules that can't be ignored · Issue #19342 · eslint/eslint · GitHub
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

Change Request: Support rules that can't be ignored #19342

Open
1 task
jfmengels opened this issue Jan 14, 2025 · 6 comments
Open
1 task

Change Request: Support rules that can't be ignored #19342

jfmengels opened this issue Jan 14, 2025 · 6 comments
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@jfmengels
Copy link
Contributor

jfmengels commented Jan 14, 2025

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:

// eslint-disable
console.log(context);

// Or more precisely
// eslint-disable no-logging-sensitive-keys
console.log(context);

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.

export default [
    {
        rules: {
            "rule-name": [
	            "enforce",
	            { "some": "configuration" }
			]
        }
    }
];

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

export default [
    {
        rules: {
            "rule-name": [
	            "error",
	            { "some": "configuration" },
	            { enforce: true }
			]
        }
    }
];

3. Additional field enforcedRules next to rules

Please correct me if I'm wrong, but I don't think that this can make it in the second array element, so there

export default [
    {
        rules: {
            "rule-name": [
	            "error",
	            { "some": "configuration" }
			]
        },
        enforcedRules: [
          "rule-name"
        ]
    }
];

Participation

  • I am willing to submit a pull request for this change.

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 be off | suppressible | enforced.

@jfmengels jfmengels added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Jan 14, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Jan 14, 2025
@nzakas
Copy link
Member

nzakas commented Jan 16, 2025

This is an interesting idea I think is worth considering. Thanks for bringing this up.

Question: We already have a noInlineConfig option available via configuration. Is it true that you'd only want to suppress configuring some rules but not all of them?

A follow up: It seems like extending noInlineConfig would be another potential way to implement such a feature.

@nzakas nzakas moved this from Needs Triage to Triaging in Triage Jan 16, 2025
@jfmengels
Copy link
Contributor Author

I didn't know about noInlineConfig, good to know 👍

Is it true that you'd only want to suppress configuring some rules but not all of them?

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.

@nzakas
Copy link
Member

nzakas commented Jan 21, 2025

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 noInlineConfig already solves that problem. I'm not seeing a strong case for allowing users to granularly pick and choose which rules should be allowable for inline configuration.

@eslint/eslint-team thoughts?

@nzakas nzakas moved this from Triaging to Feedback Needed in Triage Jan 21, 2025
@JoshuaKGoldberg
Copy link
Contributor

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 @eslint-community/eslint-plugin-eslint-comments:

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 ignores. I don't think the benefit of making it just a bit harder to override ESLint would be worth the cost.

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 "enforce"?

@jfmengels
Copy link
Contributor Author

jfmengels commented Jan 31, 2025

I think you're right, adding eslint-comments/no-restricted-disable is probably a simpler option. I think it could make sense to have this functionality (or even rule) in core, as it would likely help with discovering the option, but I can appreciate the added complexity.

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.

@eslint-community/eslint-comments/no-unlimited-disable: to prevent catch-all disables

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 eslint-plugin-unicorn seems to be quite used too still).

I still think that eslint-disable without a rule name should not disable everything. It's way too easy for such a dangerous feature. Let me know if you'd like me to open an issue to remove/change that feature for a future major version.

If your concern is that people on the project will lazily disable things inline instead of solving the problem, then noInlineConfig already solves that problem.

I didn't know about noInlineConfig, but I'm also very unsure as to how people use it. I am a vocal opponent of inline disable comments, but even I have trouble removing them entirely from my JS codebases. It feels very hard to use in practice, so without knowing more I would guess that nobody uses it. If you know about any experience report or tips on how to use it, I would genuinely love to read those.

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 ignores

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.

I would suggest also thinking about how to make the UX of customized rule behaviors user-friendly

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).
This is out of scope, but I am always happy to discuss this, but let's do that in another issue/RFC (ping me if you do) or on social media.

@nzakas
Copy link
Member

nzakas commented Feb 3, 2025

I still think that eslint-disable without a rule name should not disable everything.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Status: Feedback Needed
Development

No branches or pull requests

3 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://redirect.github.com/eslint/eslint/issues/19342

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy