[ty] Deterministic ordering of types by sharkdp · Pull Request #18091 · astral-sh/ruff

sharkdp · 2025-05-14T10:06:01Z

Summary

Our type ordering/normalization previously relied on PartialOrd/Ord instances that were auto-generated for salsa::interned structs. These implementations compare by salsa ID, which is not necessarily deterministic (depends on when a particular type has been interned, and can therefore depend on file checking order).

Besides not being deterministic, it was also incorrect. When comparing two equal unions A1 | B1 and A2 | B2, it was possible for one union to be ordered incorrectly (e.g. if both A1 and B1 were interned nominal instances), leading to incorrect type-relation results.

Writing these ordering functions by hand is exhausting and error-prone. This is why I only went so far as to fix the linked bug. Much more work would be involved to implement this for structs like FunctionType or CallableType. We should probably look into auto-generating these methods somehow?

Related salsa discussion

fixes astral-sh/ty#369

Test Plan

Ran the following command for a while and saw no flaky behavior (on the MRE from the linked ticket):

while true; do ~/.cargo-target/release/ty check --output-format concise 2>&1 | rg '(Found|checks)'; done

github-actions · 2025-05-14T11:55:58Z

`mypy_primer` results

Changes were detected when running on open source projects

hydra-zen (https://github.com/mit-ll-responsible-ai/hydra-zen)
- error[invalid-return-type] src/hydra_zen/wrapper/_implementations.py:945:16: Return type does not match returned value: expected `DataClass_`, found `@Todo(unsupported type[X] special form) | (((...) -> Any) & dict[Unknown, Unknown]) | (DataClass_ & dict[Unknown, Unknown]) | (list[Any] & dict[Unknown, Unknown]) | dict[Any, Any] | (((...) -> Any) & list[Unknown]) | (DataClass_ & list[Unknown]) | list[Any] | (dict[Any, Any] & list[Unknown])`
+ error[invalid-return-type] src/hydra_zen/wrapper/_implementations.py:945:16: Return type does not match returned value: expected `DataClass_`, found `@Todo(unsupported type[X] special form) | (((...) -> Any) & dict[Unknown, Unknown]) | (DataClass_ & dict[Unknown, Unknown]) | (list[Any] & dict[Unknown, Unknown]) | dict[Any, Any] | (((...) -> Any) & list[Unknown]) | (DataClass_ & list[Unknown]) | list[Any]`
- error[type-assertion-failure] tests/annotations/declarations.py:956:5: Argument does not have asserted type `PBuilds[@Todo(Support for `typing.TypeAlias`)] | StdBuilds[@Todo(Support for `typing.TypeAlias`)]`
- error[type-assertion-failure] tests/annotations/declarations.py:961:5: Argument does not have asserted type `PBuilds[@Todo(Support for `typing.TypeAlias`)] | StdBuilds[@Todo(Support for `typing.TypeAlias`)]`
- error[type-assertion-failure] tests/annotations/declarations.py:969:5: Argument does not have asserted type `FullBuilds[@Todo(Support for `typing.TypeAlias`)] | PBuilds[@Todo(Support for `typing.TypeAlias`)] | StdBuilds[@Todo(Support for `typing.TypeAlias`)]`
- error[type-assertion-failure] tests/annotations/declarations.py:980:5: Argument does not have asserted type `FullBuilds[@Todo(Support for `typing.TypeAlias`)] | PBuilds[@Todo(Support for `typing.TypeAlias`)] | StdBuilds[@Todo(Support for `typing.TypeAlias`)]`
- Found 649 diagnostics
+ Found 645 diagnostics

crates/ty_python_semantic/src/types/generics.rs

crates/ty_python_semantic/src/types/class.rs

MichaReiser · 2025-05-14T15:31:09Z

Hmm, this is indeed painful and also increases the work necessary to normalize types (because it's now necessary to perform a deep comparison).

Our type ordering/normalization previously relied on PartialOrd/Ord instances that were auto-generated for salsa::interned structs. These implementations compare by salsa ID, which is not necessarily deterministic (depends on when a particular type has been interned, and can therefore depend on file checking order).

I wonder if this is actually a problem. I'm not sure if it will be when we start garbage collecting interned values but is it today?

sharkdp · 2025-05-14T17:41:52Z

I wonder if this is actually a problem. I'm not sure if it will be when we start garbage collecting interned values but is it today?

I don't understand? astral-sh/ty#369 (comment) definitely feels like an actual problem to me. It's not just non-deterministic, it's also incorrect (sometimes).

sharkdp · 2025-05-14T19:00:06Z

also increases the work necessary to normalize types (because it's now necessary to perform a deep comparison).

The benchmarks seem completely neutral: https://codspeed.io/astral-sh/ruff/branches/david%2Fstable-ordering

AlexWaygood · 2025-05-14T19:49:42Z

I'm not sure I fully understand yet whether the problem is:

That the ordering isn't stable between runs or
That some part of ty incorrectly assumes that the ordering will be stable between runs

AlexWaygood · 2025-05-14T19:49:58Z

Besides not being deterministic, it was also incorrect. When comparing two equal unions A1 | B1 and A2 | B2, it was possible for one union to be ordered incorrectly (e.g. if both A1 and B1 were interned nominal instances), leading to incorrect type-relation results.

I don't fully understand this point. Could you possibly give an example?

dhruvmanila · 2025-05-14T19:43:01Z

crates/ty_python_semantic/src/semantic_index/symbol.rs

        }
    }
+
+    pub(crate) fn ordering(self, db: &'db dyn Db, other: Self) -> std::cmp::Ordering {


I guess this would be tricky to enforce unless salsa avoids adding PartialOrd, Ord to the interned structs as pointed out by Micha and in the linked salsa discussion.

dhruvmanila · 2025-05-14T19:51:31Z

crates/ty_python_semantic/src/types/generics.rs

    }
+
+    pub(crate) fn ordering(self, db: &'db dyn Db, other: Self) -> Ordering {
+        self.variables(db)


Do we need to implement .ordering for TypeVarInstance as it's interned?

MichaReiser · 2025-05-14T20:01:23Z

I wonder if this is actually a problem. I'm not sure if it will be when we start garbage collecting interned values but is it today?

I don't understand? astral-sh/ty#369 (comment) definitely feels like an actual problem to me. It's not just non-deterministic, it's also incorrect (sometimes).

I'm mainly questioning whether we need to override everywhere. We definitely should override it if it otherwise leads to incorrect ordering. I don't think we have to override in cases where it's only about determinism because ids are then a very convenient and fast way to arbitrarily sort items (if the only goal is that they have a fixed ordering but don't require a specific semantic ordering)

It's not quite clear to me what you're fixing in this pr (at least one case seems semantic)

carljm · 2025-05-15T02:28:57Z

I think our prior assumption was that ordering of types need only be deterministic within a given run of ty, because we should always normalize types as needed to ensure that type ordering differences don't have semantic effects. So if we fix the specific bugs where we fail to normalize or allow type ordering differences to make a semantic difference, that should make it unnecessary to establish a universally consistent type ordering.

sharkdp · 2025-05-15T10:29:39Z

that should make it unnecessary to establish a universally consistent type ordering.

Until we implement persistent caching :-)

MichaReiser · 2025-05-15T10:33:44Z

that should make it unnecessary to establish a universally consistent type ordering.

Until we implement persistent caching :-)

Haha, fair. Although salsa might solve this if the rematerialized structs retain their relative order

sharkdp added the ty Multi-file analysis & type inference label May 14, 2025

sharkdp marked this pull request as ready for review May 14, 2025 11:56

sharkdp requested review from AlexWaygood, carljm and dcreager as code owners May 14, 2025 11:56

MichaReiser reviewed May 14, 2025

View reviewed changes

crates/ty_python_semantic/src/types/generics.rs Outdated Show resolved Hide resolved

MichaReiser reviewed May 14, 2025

View reviewed changes

crates/ty_python_semantic/src/types/class.rs Outdated Show resolved Hide resolved

sharkdp added 3 commits May 14, 2025 20:24

[ty] Deterministic ordering of types

3a76557

Fix violated invariant

d998bea

Address review comments

6e83ce9

sharkdp force-pushed the david/stable-ordering branch from cd6a2d3 to 6e83ce9 Compare May 14, 2025 18:24

dhruvmanila approved these changes May 14, 2025

View reviewed changes

sharkdp marked this pull request as draft May 14, 2025 20:10

sharkdp mentioned this pull request May 15, 2025

[ty] Fix normalization of unions containing instances parameterized with unions #18112

Merged

sharkdp closed this May 15, 2025

Conversation

sharkdp commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented May 14, 2025

Uh oh!

sharkdp commented May 14, 2025

Uh oh!

sharkdp commented May 14, 2025

Uh oh!

AlexWaygood commented May 14, 2025

Uh oh!

AlexWaygood commented May 14, 2025

Uh oh!

dhruvmanila May 14, 2025

Choose a reason for hiding this comment

Uh oh!

dhruvmanila May 14, 2025

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented May 14, 2025

Uh oh!

carljm commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharkdp commented May 15, 2025

Uh oh!

MichaReiser commented May 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sharkdp commented May 14, 2025 •

edited

Loading

github-actions bot commented May 14, 2025 •

edited

Loading

`mypy_primer` results

carljm commented May 15, 2025 •

edited

Loading