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] Infer precise types for len() calls #14599

Merged
merged 14 commits into from
Dec 4, 2024

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #14598.

Test Plan

Markdown tests.

Copy link
Contributor

github-actions bot commented Nov 26, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Nov 26, 2024
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.

Thank you!! A few comments.

@InSyncWithFoo InSyncWithFoo requested a review from carljm November 27, 2024 08:24
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.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.

This looks quite close! Thanks for your patience with all the review comments.

I think it makes sense to leave the diagnostics as TODO here, because it will be a lot easier to emit these diagnostics once #14760 lands (we won't have to return some indicator such that inference emits the diagnostic, we'll be able to just emit it directly where we find the issue.)

crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
crates/red_knot_python_semantic/src/types.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Member

@carljm I'm not sure if #14760 will land. It's unclear to me how suppressions would work with accumulators whereas it's clear to me how they would work in our existing model.

@carljm
Copy link
Contributor

carljm commented Dec 4, 2024

@MichaReiser Ok, let's discuss those details elsewhere. The only part that's relevant here is to say that I think accumulators will be super useful in type checking and make a lot of things much simpler and easier, so I think we really want/need it, even if it requires more work on handling suppressions.

@carljm carljm dismissed AlexWaygood’s stale review December 4, 2024 19:15

Looks like all comments in this review were addressed.

@carljm
Copy link
Contributor

carljm commented Dec 4, 2024

Thanks! And great find on the string-literal-unpacking issue. Merging.

@carljm carljm merged commit 155d34b into astral-sh:main Dec 4, 2024
21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the rk-len branch December 4, 2024 19:29
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.

[red-knot] Infer precise types for len() calls
6 participants