[ty] Add blanket ignore comment rule

[MichaReiser]

Summary

Adds an opt-in blanket-ignore-comment rule that requires rule codes on ty: ignore comments.

Closes astral-sh/ty#3324.

Test Plan

Testing: Added mdtest coverage and ran the ty semantic test suite, Clippy, and repository hooks.

[astral-sh-bot]

Typing conformance results

No changes detected ✅

Current numbers

The percentage of diagnostics emitted that were expected errors held steady at 94.54%. The percentage of expected errors that received a diagnostic held steady at 90.31%. The number of fully passing files held steady at 96/134.

[astral-sh-bot]

Memory usage report

Memory usage unchanged ✅

[astral-sh-bot]

ecosystem-analyzer results

No diagnostic changes detected ✅

Flaky changes detected. This PR summary excludes flaky changes; see the HTML report for details.

Full report with detailed diff (timing results)

[carljm]

Thanks! I would like to better understand the rationale for some of the design choices here, left comments inline.

I also think the interaction between this rule and --add-ignore should at least be explicitly documented and tested. The current interaction is a bit strange, in that --add-ignore will change:

value = missing  # ty: ignore

into

value = missing  # ty: ignore  # ty: ignore[blanket-ignore-comment]

Which feels a bit strange, but might actually be the most straightforward and intuitive option?

[MichaReiser]

Regarding --add-ignore. We could decide to never add an ignore for blanked-ignore. That's what we do for unused-ignore comment. Although this seems non ideal. The alternative is to fix the ignore comment by adding the specific code. That would certainly be nice but is also more work than I think this rule is currently worth it. So I'd suggest we ship this improvement separately.

[carljm]

Thank you!

[MichaReiser]

Having thought about this again, I actually think what I had before was better. Maybe not in this every exact form but raising a blanket-ignore-comment warning AND unused-ignore-comment just feels unnecessarily noisy. The right fix is always to remove the unused ignore comment. The blanked ignore comment warning is just noise and not helpful for users.

However, we should report any blanked ignore comment if unused-ignore-comment is disabled.

The current design is also unfavorable to --fix and --add-ignore. Running --add-ignore on:

value = 1  # ty: ignore

First produces

value = 1  # ty: ignore  # ty:ignore[blanket-ignore-comment]

If you now run --fix, ty has to iterate twice:

1. To remove the ty: ignore, which is unused
2. To remove the now unused # ty:ignore[blanket-ignore-comment] comment

[carljm]

Sorry, I still strongly disagree here. I think it is confusing for different rules to interact in this way and silence each other. Each rule should have simple documented rules for what causes it to fire, and follow those rules, independent of other rules, even if that results in two diagnostics on one line.

It is not necessarily the case that "The right fix is always to remove the unused ignore comment." -- that's an unwarranted assumption that is trying too hard to read the user's mind. This is up to the user to decide. They've asked us to error on blanket ignores, and we should do that.

Consider the case where the configuration specifies blanket-ignore-comment = "error" and unused-ignore-comment = "warn". The user has requested that every blanket ignore comment should be a diagnostic of "error" severity. It is a bug if they have a blanket ignore comment, and we only issue a diagnostic of "warn" severity on that line.

[MichaReiser]

It is not necessarily the case that "The right fix is always to remove the unused ignore comment." -- that's an unwarranted assumption that is trying too hard to read the user's mind. This is up to the user to decide. They've asked us to error on blanket ignores, and we should do that.

That would imply that --fix doesn't do what the user intended or that it's not a safe fix. Both outcomes don't feel desirable to me.

Consider the case where the configuration specifies blanket-ignore-comment = "error" and unused-ignore-comment = "warn"

Fix also doesn't make this distinction.

[carljm]

I think it is reasonable for --fix to assume that the right fix for an unused ignore comment is to remove it, and that still doesn't imply that we should silence other diagnostics that the user has asked for, related to that ignore comment.

I don't think that whether --fix distinguishes between warnings and errors is relevant to the case I mentioned. --fix isn't the only use case for diagnostics. If the user has asked for an error diagnostic for a certain condition, then we should emit an error diagnostic when that condition applies.

[MichaReiser]

The error/warning case is interesting. But I still think raising unused-ignore-comment is the more accurate diagnostic than raising both. We also apply similar heuristics in ty. E.g. we prefer duplicate-base over raising inconsistent-mro, because it's the more accurate diagnostic

[carljm]

IMO those cases are not parallel. duplicate-base is effectively a more specific variant of inconsistent-mro. unused-ignore-comment and blanket-ignore-comment are totally orthogonal conditions: neither one is a more specific variant of the other. An ignore comment can be both unused and blanket, or it can be one or the other, or neither.

[MichaReiser]

I don't fully agree, but it seems you feel more strongly about this than I. So I'll back out of my last change for now, so that this gets merged (and I can put it behind me :))

[AlexWaygood]

It would be great to add this rule to https://docs.astral.sh/ty/coming-from-mypy-or-pyright/#recommended-configuration and https://docs.astral.sh/ty/coming-from-mypy-or-pyright/#mapping-pyrightmypy-rules-to-tyruff-rules — mypy enables an equivalent rule in its strict mode