Skip to content

[ty] Stop testing the (brittle) constraint set display implementation#21743

Merged
sharkdp merged 3 commits intomainfrom
dcreager/fire-display
Dec 2, 2025
Merged

[ty] Stop testing the (brittle) constraint set display implementation#21743
sharkdp merged 3 commits intomainfrom
dcreager/fire-display

Conversation

@dcreager
Copy link
Member

@dcreager dcreager commented Dec 1, 2025

The Display implementation for constraint sets is brittle, and deserves a rethink. But later! It's perfectly fine for printf debugging; we just shouldn't be writing mdtests that depend on any particular rendering details. Most of these tests can be replaced with an equivalence check that actually validates that the behavior of two constraint sets are identical.

@dcreager dcreager added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Dec 1, 2025
Comment on lines +10578 to +10583
ast::CmpOp::Eq => Some(
left.constraints(self.db()).iff(self.db(), right.constraints(self.db()))
),
ast::CmpOp::NotEq => Some(
left.constraints(self.db()).iff(self.db(), right.constraints(self.db())).negate(self.db())
),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an important bit that lets us replace all of the reveal_type tests with static_asserts. The ConstraintSet.__eq__ implementation now checks whether two constraint sets are equivalent, not whether they are identical.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 1, 2025

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 1, 2025

mypy_primer results

Changes were detected when running on open source projects
beartype (https://github.com/beartype/beartype)
- beartype/claw/_package/clawpkgtrie.py:66:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieBlacklist]'> | <class 'dict[str, Divergent]'>`
- beartype/claw/_package/clawpkgtrie.py:247:29: warning[unsupported-base] Unsupported class base with type `<class 'dict[str, PackagesTrieWhitelist]'> | <class 'dict[str, Divergent]'>`
- Found 498 diagnostics
+ Found 496 diagnostics

No memory usage changes detected ✅

Copy link
Contributor

@sharkdp sharkdp 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 guess the major drawback of this is that static_assert will only give you a success/failure feedback, whereas a failing reveal_type tests gives you both display representations (one in the error message, one in the test assertion).

We could follow the route of many other test frameworks and introduce something like a static_assert_eq(left, right) function that would show the display representation of the types of both arguments in case of a failure. That might be even nicer than reveal_type tests because we could properly align the left and right hand side types in the terminal.

I'll merge this PR to unblock the typeshed update. (no, that was the other PR)

@sharkdp sharkdp merged commit cf41964 into main Dec 2, 2025
43 checks passed
@sharkdp sharkdp deleted the dcreager/fire-display branch December 2, 2025 08:17
@dcreager
Copy link
Member Author

dcreager commented Dec 2, 2025

I'll merge this PR to unblock the typeshed update. (no, that was the other PR)

The other PR was blocked on this one, though, so thank you regardless!

We could follow the route of many other test frameworks and introduce something like a static_assert_eq(left, right) function that would show the display representation of the types of both arguments in case of a failure. That might be even nicer than reveal_type tests because we could properly align the left and right hand side types in the terminal.

That's a good idea, I opened astral-sh/ty#1719 to track it.

dcreager added a commit that referenced this pull request Dec 2, 2025
* origin/main: (67 commits)
  Move `Token`, `TokenKind` and `Tokens` to `ruff-python-ast` (#21760)
  [ty] Don't confuse multiple occurrences of `typing.Self` when binding bound methods (#21754)
  Use our org-wide Renovate preset (#21759)
  Delete `my-script.py` (#21751)
  [ty] Move `all_members`, and related types/routines, out of `ide_support.rs` (#21695)
  [ty] Fix find-references for import aliases (#21736)
  [ty] add tests for workspaces (#21741)
  [ty] Stop testing the (brittle) constraint set display implementation (#21743)
  [ty] Use generator over list comprehension to avoid cast (#21748)
  [ty] Add a diagnostic for prohibited `NamedTuple` attribute overrides (#21717)
  [ty] Fix subtyping with `type[T]` and unions (#21740)
  Use `npm ci --ignore-scripts` everywhere (#21742)
  [`flake8-simplify`] Fix truthiness assumption for non-iterable arguments in tuple/list/set calls (`SIM222`, `SIM223`) (#21479)
  [`flake8-use-pathlib`] Mark fixes unsafe for return type changes (`PTH104`, `PTH105`, `PTH109`, `PTH115`) (#21440)
  [ty] Fix auto-import code action to handle pre-existing import
  Enable PEP 740 attestations when publishing to PyPI (#21735)
  [ty] Fix find references for type defined in stub (#21732)
  Use OIDC instead of codspeed token (#21719)
  [ty] Exclude `typing_extensions` from completions unless it's really available
  [ty] Fix false positives for `class F(Generic[*Ts]): ...` (#21723)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants