Skip to content

[ty] Prefer innermost inline suppressions - #26940

Merged
charliermarsh merged 3 commits into
mainfrom
charlie/prefer-innermost-suppressions
Jul 20, 2026
Merged

charliermarsh merged 3 commits into
mainfrom
charlie/prefer-innermost-suppressions

Conversation

@charliermarsh

@charliermarsh charliermarsh commented Jul 17, 2026 •

Copy link
Copy Markdown
Member

Summary

When multiple same-line or own-line suppressions apply to the same diagnostic, prefer the innermost suppression when their ranges are nested. This keeps the closest directive marked as used and lets an outer directive continue to suppress diagnostics elsewhere in the logical statement.

For a diagnostic spanning multiple lines, a suppression on its opening line takes precedence over a separate suppression on its closing line. This is not an innermost/outermost case because neither suppression contains the other. Keeping opening-line precedence also prevents a previously used opening-line suppression from becoming unused.

The lookup is indexed by suppression target so innermost precedence does not require scanning unrelated nested suppressions for every diagnostic. The same precedence is used when checking unused-ignore-comment and blanket-ignore-comment.

Rules

Nested suppression ranges

On main, applicable suppressions are checked in source order. The outer suppression appears first, so it suppresses both diagnostics and the inner suppression is reported as unused:

# ty: ignore[unresolved-reference]  # Used for `missing` and `absent`.
values = [
    # ty: ignore[unresolved-reference]  # Unused.
    missing,
    absent,
]

With this PR, the innermost suppression wins for missing. The outer suppression remains responsible for absent, so both suppressions are used:

# ty: ignore[unresolved-reference]  # Used for `absent`.
values = [
    # ty: ignore[unresolved-reference]  # Used for `missing`.
    missing,
    absent,
]

Separate opening-line and closing-line suppressions

For a diagnostic that spans multiple lines, suppressions on its opening and closing lines can both apply even though neither suppression contains the other:

f(  # ty: ignore[missing-argument]
    g(missing))  # ty: ignore[unresolved-reference, missing-argument]

On main, the opening-line suppression wins because it appears first. This PR deliberately preserves that behavior: innermost precedence applies only when the suppression ranges are nested. The closing-line suppression still handles unresolved-reference, but its missing-argument code is unused.

@astral-sh-bot astral-sh-bot Bot added the ty The ty type checker label Jul 17, 2026
@astral-sh-bot

astral-sh-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

Typing conformance results

No changes detected ✅

Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 96.83%. The percentage of expected errors that received a diagnostic held steady at 91.74%. The number of fully passing files held steady at 99/133.

@astral-sh-bot

astral-sh-bot Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot

astral-sh-bot Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

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)

@charliermarsh
charliermarsh force-pushed the charlie/fix-add-ignore-trailing-reasons branch from b1afe5b to 9b56eb2 Compare July 17, 2026 17:41
@charliermarsh
charliermarsh force-pushed the charlie/prefer-innermost-suppressions branch from cc68889 to 50892ef Compare July 17, 2026 17:41
@charliermarsh
charliermarsh marked this pull request as ready for review July 17, 2026 18:00
@charliermarsh
charliermarsh requested review from a team as code owners July 17, 2026 18:00
@charliermarsh
charliermarsh force-pushed the charlie/prefer-innermost-suppressions branch from 50892ef to 7577dbd Compare July 17, 2026 18:09
@charliermarsh
charliermarsh force-pushed the charlie/fix-add-ignore-trailing-reasons branch from 9b56eb2 to 6012ceb Compare July 17, 2026 18:09
@MichaReiser

Copy link
Copy Markdown
Member

Would you mind updating your summary with today's behavior and what behavior this PR is exactly changing? Or is all of this behavior new?

@charliermarsh

Copy link
Copy Markdown
Member Author

But, it has that already!

Screenshot 2026-07-18 at 7 46 56 AM

@charliermarsh
charliermarsh force-pushed the charlie/fix-add-ignore-trailing-reasons branch from 6012ceb to 34b87c8 Compare July 18, 2026 11:58
@charliermarsh
charliermarsh requested a review from a team as a code owner July 18, 2026 11:58
@charliermarsh
charliermarsh force-pushed the charlie/prefer-innermost-suppressions branch from 7577dbd to e6efa92 Compare July 18, 2026 11:58
Base automatically changed from charlie/fix-add-ignore-trailing-reasons to main July 18, 2026 12:02
@MichaReiser

Copy link
Copy Markdown
Member

Lol, I only read the first two paragraphs and was like, uff, I don't know how this differs from main. I'm sorry

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new datastructure feels somewhat over-engineered to me. I'm to hear your thinking of why you chose it.

Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
Comment thread crates/ty_python_semantic/src/suppression.rs Outdated
@charliermarsh
charliermarsh marked this pull request as draft July 19, 2026 19:31
@charliermarsh
charliermarsh marked this pull request as ready for review July 19, 2026 19:42
@astral-sh-bot
astral-sh-bot Bot requested a review from dcreager July 19, 2026 19:42
@charliermarsh
charliermarsh requested review from MichaReiser and removed request for dcreager July 19, 2026 20:10

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect that there's so much room for making our suppression checking faster. Nice!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is from your other PR, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry, it just needed to be rebased.

/// closing line while a separate earlier candidate covers its opening line. The closing-line
/// candidate wins only when its suppression range is nested within the opening-line candidate's
/// range; otherwise the opening-line candidate retains precedence.
fn select_preferred_suppression<'a>(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like this API

SuppressionTarget::Lint(id) => {
let mut hasher = FxHasher::default();
id.hash(&mut hasher);
1 << (1 + hasher.finish() % 63)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever. I was a bit concerned after making the hashing suggestion that this would actually lead to a pretty bad filter, because & ing hashes are likely to have some bit in common. But the bucketing solves this. I suggest adding a comment somewhere (if there's not already one that I missed) saying that we bucket rules into 64 buckets and only traverse into sub-trees that overlap.

@charliermarsh
charliermarsh force-pushed the charlie/prefer-innermost-suppressions branch from cb4e4f9 to da3cc62 Compare July 20, 2026 13:55
@charliermarsh
charliermarsh enabled auto-merge (squash) July 20, 2026 13:58
@charliermarsh
charliermarsh merged commit 502937d into main Jul 20, 2026
62 checks passed
@charliermarsh
charliermarsh deleted the charlie/prefer-innermost-suppressions branch July 20, 2026 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty The ty type checker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants