Skip to content

[ty] Fix precedence of all selector in TOML configurations#23723

Merged
charliermarsh merged 3 commits intomainfrom
charlie/toml-order
Mar 5, 2026
Merged

[ty] Fix precedence of all selector in TOML configurations#23723
charliermarsh merged 3 commits intomainfrom
charlie/toml-order

Conversation

@charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 5, 2026 •
edited
Loading

Summary

Closes astral-sh/ty#2952.

TOML tables are unordered — the spec says key/value pairs within tables are not guaranteed to be in any specific order. However, the toml crate happens to sort keys lexicographically when deserializing. This means that rules like abstract-method-in-final-class sort before all, so the all selector silently overrides the more-specific rule when processed sequentially.

This PR fixes the issue by sorting selectors from the same configuration file so that all is always applied first, then specific per-rule selectors are applied afterwards. This ensures that specific rules always take precedence over all within a given config file, regardless of lexicographic ordering.

The fix is scoped to file-based configuration only — CLI argument order is still preserved as-is, since users have explicit control over ordering there.

Test plan

Added a test (configuration_all_rules_with_rule_sorting_before_all) that uses abstract-method-in-final-class (which sorts before all) to verify the specific rule takes precedence.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 5, 2026 •
edited
Loading

Typing conformance results

No changes detected ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 5, 2026 •
edited
Loading

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 5, 2026 •
edited
Loading

mypy_primer results

Changes were detected when running on open source projects 
scikit-build-core (https://github.com/scikit-build/scikit-build-core)
+ src/scikit_build_core/build/wheel.py:99:20: error[no-matching-overload] No overload of bound method `__init__` matches arguments
- Found 57 diagnostics
+ Found 58 diagnostics

@charliermarsh charliermarsh marked this pull request as ready for review March 5, 2026 00:47
@carljm carljm assigned MichaReiser and unassigned carljm Mar 5, 2026
@carljm carljm removed their request for review March 5, 2026 01:02
@AlexWaygood AlexWaygood added configuration Related to settings and configuration ty Multi-file analysis & type inference labels Mar 5, 2026
MichaReiser
MichaReiser requested changes Mar 5, 2026
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I don't think this fix is correct. We do want to preserve the relative order between different configuration files (or main configuration and overrides). The same problem doesn't exist for CLI arguments either.

There's an open question about whether all should always take precedence and disregard order on the CLI and in an editor configuration, even though these sources have perfectly defined ordering. Or if we should take a different approach here altogether. I don't know what the right answer to this. But ordering the rules here is incorrect as it prevents a configuration from using all to disable any rules enabled in a parent configuration.

@charliermarsh
Copy link
Member Author

charliermarsh commented Mar 5, 2026

Isn’t this just sorting all in front of other selectors, within a single file’s selectors? Why would this disregard relative ordering across files, as implemented? (I’m not sure that sorting in this way is stale / safe but that seems different than the concern you’re getting at.)

@MichaReiser
Copy link
Member

MichaReiser commented Mar 5, 2026 •
edited
Loading

Rules::to_rule_selection is called from Options::to_rule_selection. Options::to_rule_selection is only called on the merged Options from different sources. If you want to sort the rules from a single file, you have to merge them within Options before a potential .combine call

@charliermarsh charliermarsh changed the title [ty] Apply file-level all selector before other rules [ty] Fix precedence of all selector in TOML configurations Mar 5, 2026
@charliermarsh
Copy link
Member Author

charliermarsh commented Mar 5, 2026

That makes sense, thank you.

MichaReiser
MichaReiser approved these changes Mar 5, 2026
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thank you.

I did some research on why cargo decided to go for a priority field. It turns out, that cargo supports intersecting categories (CC: @ntBre this might be interesting for you) and, therefore, requires an explicit tie break from the user (source)

MichaReiser
MichaReiser reviewed Mar 5, 2026
View reviewed changes
@charliermarsh charliermarsh enabled auto-merge (squash) March 5, 2026 15:24
@charliermarsh charliermarsh merged commit a6a5e8d into main Mar 5, 2026
50 checks passed
@charliermarsh charliermarsh deleted the charlie/toml-order branch March 5, 2026 15:28
carljm added a commit that referenced this pull request Mar 5, 2026
@carljm
Merge branch 'main' into cjm/remove-container-literal-unknown
461f4c9 
* main:
  Update conformance suite commit hash (#23746)
  conformance.py: Collapse the summary paragraph when nothing changed (#23745)
  [ty] Make inferred specializations line up with source types more better (#23715)
  Bump 0.15.5 (#23743)
  [ty] Render all changed diagnostics in conformance.py (#23613)
  [ty] Split deferred checks out of `types/infer/builder.rs` (#23740)
  Discover markdown files by default in preview mode (#23434)
  [ty] Use `HasOptionalDefinition` for `except` handlers (#23739)
  [ty] Fix precedence of `all` selector in TOML configurations (#23723)
  [ty] Make `all` selector case sensitive (#23713)
  [ty] Add a diagnostic if a `TypeVar` is used to specialize a `ParamSpec`, or vice versa (#23738)
  [ty] Override home directory in ty tests (#23724)
  [ty] More type-variable default validation (#23639)
  [ty] Validate bare ParamSpec usage in type annotations, and support stringified ParamSpecs as the first argument to `Callable` (#23625)
  [ty] Add `all` selector to ty.json's `schema` (#23721)
  [ty] Add quotes to related issues links (#23720)
  [ty] Fix panic on incomplete except handlers (#23708)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

@ibraheemdev ibraheemdev Awaiting requested review from ibraheemdev ibraheemdev is a code owner

@Gankra Gankra Awaiting requested review from Gankra Gankra is a code owner

Assignees

@MichaReiser MichaReiser

Labels

configuration Related to settings and configuration ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix precedence of ALL selector in TOML configurations

4 participants

@charliermarsh @MichaReiser @carljm @AlexWaygood