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 · 3 comments
Open
1 task

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

jfmengels opened this issue Jan 14, 2025 · 3 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 security 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
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

2 participants