Skip to content
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

[red-knot] Add type inference for loop variables inside comprehension scopes #13251

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

This adds type inference for loop variables inside comprehension scopes. The type-inference tests are passing, but it seems to currently trigger a panic in the corpus tests.

Async comprehensions will require different logic, so for now loop variables in these are inferred as Unknown still. In order to distinguish between async comprehension definitions and synchronous comprehensions definitions, a new is_async field is added to ComprehensionDefinitionKind and various other structs for tracking comprehension definitions.

Test Plan

cargo test -p red_knot_python_semantic and cargo test -p red_knot_workspace

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Sep 5, 2024
@AlexWaygood AlexWaygood changed the title [WIP] Add type inference for loop variables inside comprehension scopes [WIP] [red-knot] Add type inference for loop variables inside comprehension scopes Sep 5, 2024
Copy link

codspeed-hq bot commented Sep 5, 2024

CodSpeed Performance Report

Merging #13251 will not alter performance

Comparing alex/comprehension-inference (86aacad) with main (ac720cd)

Summary

✅ 32 untouched benchmarks

@AlexWaygood AlexWaygood force-pushed the alex/comprehension-inference branch 2 times, most recently from 3cfa295 to b3c0e4f Compare September 5, 2024 11:37
@MichaReiser
Copy link
Member

Just FYI: I think the benchmark is panicking now. I see some std::io::Write show up in the benchmarks.

@AlexWaygood
Copy link
Member Author

lots of things are currently broken here 😆 I just figured I'd make a draft PR to show where I'm at (and in case it's obvious to anybody else what the cause of the panics is)

@MichaReiser
Copy link
Member

lots of things are currently broken here 😆 I just figured I'd make a draft PR to show where I'm at (and in case it's obvious to anybody else what the cause of the panics is)

Haha, okay. I was shocked by the performance regression, so I had to take a quick look at why it was that bad. I was relieved to see that it was probably caused by the benchmark crashing.

@AlexWaygood
Copy link
Member Author

I was shocked by the performance regression, so I had to take a quick look at why it was that bad. I was relieved to see that it was probably caused by the benchmark crashing.

Oh the PR also currently adds a dbg!() call in quite a hot location so that I can see what's going on more clearly. I assume that doesn't help 😄

@AlexWaygood AlexWaygood force-pushed the alex/comprehension-inference branch 2 times, most recently from 2a04e2f to ef4caf1 Compare September 9, 2024 19:22
@AlexWaygood AlexWaygood changed the title [WIP] [red-knot] Add type inference for loop variables inside comprehension scopes [red-knot] Add type inference for loop variables inside comprehension scopes Sep 9, 2024
@AlexWaygood AlexWaygood marked this pull request as ready for review September 9, 2024 19:22
Copy link
Contributor

github-actions bot commented Sep 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood
Copy link
Member Author

Haha, okay. I was shocked by the performance regression, so I had to take a quick look at why it was that bad. I was relieved to see that it was probably caused by the benchmark crashing.

All gone now 🎉

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks great!!

crates/red_knot_python_semantic/src/types/infer.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

All of the tests! Looks excellent, merge away

@AlexWaygood AlexWaygood enabled auto-merge (squash) September 9, 2024 20:20
@AlexWaygood
Copy link
Member Author

Thanks very much for the help debugging this!

@AlexWaygood AlexWaygood merged commit 6f53aaf into main Sep 9, 2024
18 checks passed
@AlexWaygood AlexWaygood deleted the alex/comprehension-inference branch September 9, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants