Suppression Comment Design

[evanrittenhouse]

Background

Several issues (#1054, #3711, #2999) mention the desire for block-scoped noqa comments and general noqa changes. There is also appetite for human-friendly rule names (#1773) which could be incorporated into noqa comments. As such, I believe that there's sufficient motivation to start work on a large-scale suppression comment rework.

I believe that this approach could be executed in two phases, one for reworking suppression scopes and the latter for changing the noqa parser. This document outlines potential solutions for the first phase, which I think would take the most work. The second phase would probably involve changing the noqa parser to allow human-readable names to be picked up as well as a translation layer to go from rule to name and back again. However, it probably falls outside the scope of this document.

Phase 1 Proposal

We can build on the syntax introduced in #2978 to allow code blocks by enabling/disabling noqa selectors.

Note that all approaches can be easily extended to include multiple rules by following the typical convention of <rule1>,<rule2>,....

We can also easily extend both approaches to add enabling on a per-block basis in the future, if we need to. We could build in logic to parse the appropriate suppression comment (either disable or enable) and tailor our logic around that.

Approach 1

z = 1

# ruff: noqa: disable: E741
x = 1
y = 1
# ruff: noqa: enable: E741

...

# ruff: noqa: disable: E741
def somePoorlyDefinedVariable():
	print("this won't throw")
	z = 1
# ruff: noqa: enable: E741

This should yield one AmbiguousVariableName diagnostic on line 1. This is slightly more fragile than Approach 2 since you could delete either a disable or enable without deleting the other, which would prompt a new diagnostic to show up. That could get annoying, though you could make the argument that it also makes the linter more robust. No accidentally deleting a line and not knowing about it :)

Pros/Cons

1. (+) Easily legible and easy to reason about for the reader/user.
2. (-) Clunkier for shorter blocks.
3. (-) More verbose than other options.
4. (-) Doesn't fit the existing ecosystem - more similar to eslint's implementation than Pylint's.

FAQ

1. What happens if E741 isn't enabled for the file being edited and I try to disable it? What if it's enabled and I try to re-enable it?
   1. I think a new RUF rule would be appropriate which alerts the user to an unnecessary noqa block. We could also of course simply ignore it, but I think that goes against Ruff's philosophy - unnecessary suppression comments would clutter up the code with no good reason.
2. Will this ever conflict with # ruff: noqa: E741?
   1. Good question. I think not, since that line is already the trigger for the "disable-everywhere" function. That directive could continue to function as-is, and blocks would be delineated by disable/enable tags. This does lead to the issue of multiple syntaxes, though, which is something I'd like to avoid.
3. What if we disable without ever re-enabling/enable without ever disabling?
   1. As mentioned above, I believe it's more in line with Ruff's philosophy to raise a new diagnostic mentioning that you've opened a suppression block without closing it.

Approach 2

z = 1

# ruff: noqa:< disable:> E741
x = 1
# ruff: noqa:< disable:> E741
y = 1

...

# ruff: noqa:< disable:> E741
def somePoorlyDefinedVariable():
	print("this won't throw")
	z = 1

This should only throw on line 2 - the idea behind this approach is that the disable functions on the AST node directly beneath it.

This is pretty similar to Pylint's current block comment implementation. If we went with this approach, it may be worth considering whether we should simply copy their behavior exactly but with a different directive comment.

I put < disable:> in braces to delineate the fact that we could treat it as optional. I don't think that's a great idea though; if we did, the current # ruff: noqa directive would need to be refactored to act in a per-block fashion and we'd have to figure out how to signify per-file suppressions. Long story short, if we went with this approach I'd strongly recommend having it be # ruff: noqa: disable: E741 and unifying the syntaxes.

Pros/Cons

1. (+) Quicker to write for users.
2. (+) Similarity to Pylint will make it more familiar to users, making it easier to plug and play.
3. (-) Can be a bit unintuitive, especially in the case of lines 2/3 - it's somewhat tough to tell off the bat that we only want to suppress on line 2.
4. (-) More fragile - if you delete one line accidentally, you could disable/enable rules easily.

FAQ

1. What happens if E741 isn't enabled for the file being edited and I try to disable it? What if it's enabled and I try to re-enable it?
   1. Similar to Approach 1, in this case I think that we should throw a new diagnostic.
2. Will this ever conflict with # ruff: noqa: E741?
   1. I think it should. To preserve Python's "one way of doing something" ethos, under this approach we may want to consider changing file-wide suppression so it's only activated by putting # ruff: noqa: disable: E741 at the top of the file (or in the file scope). This is in contrast to the current way of doing this: # ruff: noqa: E741 will disable everywhere, regardless of where the file is. I'd like to avoid the issue of having separate syntaxes for per-file and per-block suppression though, so IMO we should unify the two. See below for an example of how this could work.
3. What if we disable without ever re-enabling/enable without ever disabling?
   1. That wouldn't be possible with this approach. The suppression block would simply go out of scope, then we could continue to read diagnostics as normal.

Approach 2, scoping, and per-file suppression

z = 1

x = 1
# ruff: noqa: enable: E741
y = 1

# ruff: noqa: disable: E741 
def somePoorlyDefinedVariable():
	print("this won't throw")
	z = 1

       # ruff: noqa: enable: E741
      a = 1 # this will throw

In this example, our disable comment applies to the function definition while the enable comment applies ONLY to the expression a = 1. Since we enable E741 in a scope narrower than the one in which we disable it, enabling the rule would take precendence for that line only. The rest of the function would have the rule disabled.

I think for the sake of this issue, it would be best to ignore the enable piece for now. So implementation would involve

1. Finding a suppression comment.
2. Determining the scope that it should apply to. From my initial thinking, probably the scope for the AST node directly beneath it - for example, if the rule was above a class statement, the suppression would apply to the entire class definition. If it's above an expression, the rule would apply only to that expression.
3. Removing diagnostics in that scope from our final diagnostic list.

Using this design would allow us to easily implement per-block rule enabling in the future. We could prioritize directives by narrowness of scope - i.e., a = 1 should throw because the rule is enabled in a scope which is narrower than the scope which disables it. Determining this "scope hierarchy" to enable/disable blocks based on scope narrowness should probably be a separate FR centered on enabling rules for certain blocks though.

Rejected: I (very briefly) considered a pyproject.tomlconfiguration option, but that's pretty infeasible given how often code changes. While it could clean up the code, it'd be more difficult to reason about ("why are these random diagnostics showing up? I haven't disabled them in my code anywhere? Ah they're in pyproject.toml for some reason") and changing them whenever code lines change would be...tedious, to say the least.

---

Notes

#4669 mentions changing the noqa keyword to something else. All designs here can also be considered through the lens of switching noqa for a different keyword.

Verdict

I'm leaning pretty heavily towards Approach 2 for two reasons. First, I remember some issue mentioning that a big component of Ruff's growth was the ease of dropping it into an existing project. Approach 2's cognitive overhead is therefore lower than Approach 1's as it would act similarly to Pylint's block disabling.

Second, and the main reason I advocate for Approach 2, is that it allows for a consistent, sensible syntax that, once understood, is pretty simple to implement and read. It integrates nicely with the existing per-file suppressions, since we could simply place the suppression in a file-scoped line separate from any expressions. That means that users wouldn't need to do all that much to accomadate this breaking change. We would simply need to change # ruff: noqa: <rule> to # ruff: noqa: disable: <rule> and tell users that they'd need to put the comment on a file-scoped line.

Open to any and all feedback! :)

[jfmengels]

I'm not really familiar with how the current suppression comments work in Ruff, so pardon me if I say nonsense.

I put < disable:> in braces to delineate the fact that we could treat it as optional.

This means that you could do # ruff: noqa: XYZ123, right? I'm not sure I got what that would then mean. But the same confusion applies to the first proposal.

As someone not familiar enough with Ruff or other Python linters, I don't know what # ruff: noqa: disable: XYZ123 means (omitting the confusing rule name). Is this for this line? The next line? Until the end of the file. That's quite unclear to me.

What would be clearer to be would more verbose comments noqa-for-file, noqa-until-enable, which speak a lot more as to what they'll do. A few examples of what these could be (wording open for debate)

• # ruff: noqa-line: XYZ123: Disable this rule for this line
• # ruff: noqa-next-line: XYZ123: Disable this rule for the next line (both were stolen from ESLint)
• # ruff: noqa-until-reenable: XYZ123: Disable this rule until there is a # ruff: noqa-reenable: XYZ123 for the same rule. If there is no such comment, Ruff can print an error.
• # ruff: noqa-until-end-of-file: XYZ123: Same as before, but no need for a "enable" comment. The user has explicitly chosen it was until the end of file. If there are new suppression comments, then they could take over for the section.
• # ruff: noqa-for-method: XYZ123: If we want to get fancy, we could have special ones. Not sure I'd recommend this, but it would be possible.

It's a lot more verbose (and autocomplete would help) but it's far easier to understand for a human, which I believe has its merits (I heard something about "explicit over implicit" in Python...).

And as per your question, I think it would make sense to enable rules that have not been enabled for this file, if they've been explicitly been enabled inside of the file. Behind-the-scenes, for rules that collect some information (and not just pattern-match on a single element), I think you would need to run the rule anyway (to collect the information) but suppress the errors, or you could do a quick scan of which rules get enabled.

I think there could be value in having a separation between "enable" comments and "reeanable": If you do want to "enable" rules that were previously not set up for this file, you could have a separate set of comments that follow the ideas from above.

I remember some issue mentioning that a big component of Ruff's growth was the ease of dropping it into an existing project.

Maybe I'm going against decisions that have been taken previously that I'm not aware of. But just like --add-noqa, I'm thinking the tool could support the current noqa syntax, but have its own version (like the one I suggested) and have a command to replace the existing comments — or add to them if you (or the user actually) want to keep both for some reason — by Ruff's style. If needed, this command could be enabled per rule (that you migrate from one tool to Ruff) and/or per file. But maybe this is going too far from the initial discussion.

[evanrittenhouse]

Thanks for the feedback! A couple things:

This means that you could do # ruff: noqa: XYZ123, right? I'm not sure I got what that would then mean.

Currently, # ruff: noqa; XYZ123disables directives on the entire file, which is where I got the idea for that # ruff: noqa: <descriptor>: <rule> syntax. I think that, since it's already implemented, it's important to stay somewhat consistent to that syntax.

What would be clearer to be would more verbose comments noqa-for-file, noqa-until-enable, which speak a lot more as to what they'll do.

I do really like this idea, actually. I think that more verbose comments would be helpful, especially when looking at Approach 2. The main roadblock that I could see happeningt would be how block-level comments would be implemented. I think that we could easily fall down the rabbit hole with stuff like # ruff: noqa-for-method: XYZ123. Maybe we could do something like # ruff: noqa: for-block: XYZ123 to ignore for blocks? Maybe, if we went with Approach 2, it could be something like # ruff: noqa: disable-block: XYZ123, but the same scoping rules would apply?

I think there could be value in having a separation between "enable" comments and "reeanable": If you do want to "enable" rules that were previously not set up for this file, you could have a separate set of comments that follow the ideas from above.

I think this is a good idea as well. If we did end up deciding on # ruff: noqa: <descriptor>: <rule> syntax, we could easily add more descriptors. Especially if we use Approach 1, reenable will be better than enable:

# ruff: noqa: disable: XYZ123
...
# ruff: noqa: reenable: XYZ123

E: formatting

[copdips]

just my 2 cents, why not # noruff: instead of # ruff: noqa:

[evanrittenhouse]

Ruff uses # ruff: noqa: right now, but yeah this would be a good opportunity to change that

[bjtho08]

Just writing to say that I would really love seeing Approach 2 implemented! :)

Is there any active development yet or are you still awaiting feedback?

[evanrittenhouse]

Thanks for the kind words! I haven't heard from the maintainers on the path that they'd prefer, but I think their main priority right now is the formatter.

@MichaReiser would this need further discussion from the team? I'm also in favor of approach 2, FWIW, so could look at starting to implement

[MichaReiser]

Hy @evanrittenhouse I'm afraid. I don't think we're able to commit to a new suppression comment design right now because we want to avoid having to change suppression comments later again once we have a better understanding for the formatter - linter integration and whether we want to adopt human readable rule names.