Skip to content

[ty] Pull types on synthesized Python files created by mdtest #18539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 12, 2025

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 7, 2025

Summary

Our "corpus tests" run an AST visitor that attempts to exhaustively check that each sub-expression in an AST has a type inferred for it. This is a great way to catch potential panics that might happen when you're hovering over an expression in an IDE, for example, since ty will try to retrieve the type of that node so that the IDE can show a nice tooltip to the user.

The corpus tests have a problem, though. There are a number of typing symbols to which ty applies heavy special casing. It's quite common for us to forget to store a type for a subexpression when inferring types for these heavily special-cased symbols, especially in cases where these symbols are being used invalidly. Our corpus of Python snippets that we run the "corpus tests" over doesn't provide great coverage for these symbols, so it's easy for these mistakes to go unnnoticed.

We do have a corpus of Python snippets that do have good coverage for these symbols, however, and which even have pretty good coverage of these symbols being used invalidly: the Python files embedded inside our mdtests! Running the PullTypesVisitor over all snippets in our Markdown files should give us a much higher level of confidence that we're not forgetting to store types for any subexpressions in any files.

This PR adds a step to mdtest that runs the PullTypesVisitor over every synthesized file created by the mdtest framework. There are six existing mdtest suites that fail this new step; these Markdown files have been added to a KNOWN_PULL_TYPES_FAILURES list in the ty_test crate.

Test Plan

cargo test

@AlexWaygood AlexWaygood added testing Related to testing Ruff itself ty Multi-file analysis & type inference labels Jun 7, 2025
@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch from a172553 to 92631b8 Compare June 7, 2025 18:35
Copy link
Contributor

github-actions bot commented Jun 7, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch from 92631b8 to 331b000 Compare June 7, 2025 18:39
Copy link
Contributor

github-actions bot commented Jun 7, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@AlexWaygood AlexWaygood marked this pull request as ready for review June 7, 2025 18:50
@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch from 331b000 to 1a73898 Compare June 9, 2025 11:14
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.

This is a great idea!

Let me know if I should take a look at the catch unwind thingy

@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch from 1a73898 to eac35f7 Compare June 11, 2025 17:54
@AlexWaygood AlexWaygood marked this pull request as draft June 11, 2025 17:55
@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch 2 times, most recently from 56d9ab2 to 9be052f Compare June 11, 2025 19:48
@AlexWaygood AlexWaygood force-pushed the alex/pulltypes-mdtest branch from 9be052f to 377eb7d Compare June 11, 2025 19:50
@AlexWaygood
Copy link
Member Author

Okay, this has now been reimplemented as a "proper mdtest feature", that can be disabled on a per-test basis by adding the <!-- pull-types:skip --> directive to a test.

This is what it looks like if the step fails with an unexpected error:

image

And this is what an unexpected success looks like:

image

@AlexWaygood AlexWaygood marked this pull request as ready for review June 11, 2025 19:53
@AlexWaygood AlexWaygood requested a review from MichaReiser June 11, 2025 19:53
@@ -50,6 +50,7 @@ strum_macros = { workspace = true }
[dev-dependencies]
ruff_db = { workspace = true, features = ["testing", "os"] }
ruff_python_parser = { workspace = true }
ty_python_semantic = { workspace = true, features = ["testing"] }
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I'm surprised that a crate is allowed to depend on itself.

@AlexWaygood AlexWaygood merged commit 324e5cb into main Jun 12, 2025
35 checks passed
@AlexWaygood AlexWaygood deleted the alex/pulltypes-mdtest branch June 12, 2025 09:32
dcreager added a commit that referenced this pull request Jun 12, 2025
* main:
  [ty] Add some "inside string" tests for `object.<CURSOR>` completions
  [ty] Pull types on synthesized Python files created by mdtest (#18539)
  Update Rust crate anstyle to v1.0.11 (#18583)
  [`pyupgrade`] Fix `super(__class__, self)` detection in UP008 (super-call-with-parameters) (#18478)
  [ty] Generate the top and bottom materialization of a type (#18594)
  `SourceOrderVisitor` should visit the `Identifier` part of the `PatternKeyword` node (#18635)
  Update salsa (#18636)
  [ty] Update mypy_primer doc (#18638)
  [ty] Improve support for `object.<CURSOR>` completions
  [ty] Add `CoveringNode::find_last`
  [ty] Refactor covering node representation
  [ty] Infer the Python version from `--python=<system installation>` on Unix (#18550)
  [`flake8-return`] Fix `RET504` autofix generating a syntax error (#18428)
  Fix incorrect salsa `return_ref` attribute (#18605)
  Move corpus tests to `ty_python_semantic` (#18609)
  [`pyupgrade`] Don't offer fix for `Optional[None]` in non-pep604-annotation-optional (`UP045)` or non-pep604-annotation-union (`UP007`) (#18545)
  [`pep8-naming`] Suppress fix for `N804` and `N805` if the recommend name is already used (#18472)
  [`ruff`] skip fix for `RUF059` if dummy name is already bound (unused-unpacked-variable) (#18509)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to testing Ruff itself ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants