realm / SwiftLint

A tool to enforce Swift style and conventions.
https://realm.github.io/SwiftLint
MIT License
18.58k stars 2.22k forks source link

The superfluous_disable_command rule should report disabled rules too #5715

Open revolter opened 1 month ago

revolter commented 1 month ago

New Issue Checklist

Describe the bug

If you disable a <rule> using the disabled_rules configuration key, but still have // swiftlint:disable <rule> in the code, the superfluous_disable_command rule should still report a warning for them.

I didn't complete the rest of the template, because I think it's more of a logic issue than a rule issue, and it's pretty self explanatory, but let me know if you want me to do that.

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint

Environment

# insert yaml contents here
// This triggers a violation:
let foo = try! bar()
SimplyDanny commented 1 month ago

Seems like this has never been intended with this rule. However, this sounds like a reasonable enhancement of the current logic. A dedicated rule for disable/enable commands without a matching active rule would also be an option. Perhaps this is even better to really match all command types (not only commands to disable the rule).

mildm8nnered commented 1 month ago

So I think the existing behaviour is kind of intentional, and has some advantages.

For example, consider analyzer rules - we do want to be able to suppress false alarms (or stuff we can't be bothered to fix), but if we got a superfluous_disable_command warning when running linting, that would not be helpful.

Of course that's just a happy accident of the current behaviour, and we could always special case analyzer rules, but in the general case, if I turn some rules off, for whatever reason, it's not helpful to then get a bunch of superfluous_disable_command warnings

SimplyDanny commented 1 month ago

These are good arguments for a dedicated rule.

mildm8nnered commented 1 month ago

These are good arguments for a dedicated rule.

I think my natural inclination is to say a configuration option feels better somehow. Especially in this case - superfluous_disable_command is a bit of a special case - it's kind of a "meta" rule that gets implemented in the Linter.

I'm really struggling with that right now in #5670

The non-discoverability of options versus rules make making it a rule quite appealing, but I think what we need to do there is improve the discoverability and documentation of options.

SimplyDanny commented 1 month ago

An option is fine for me as well.