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] remove wrong typevar attribute implementations #14540

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Nov 22, 2024

Summary

In #14182 I added "precise" per-TypeVar type inference for attributes of a typevar (__bound__, __constraints__, __default__). This wasn't motivated so much by the real-world need for this precise inference, as it was by wanting to test the correctness of our internal representation of the typevar, via an mdtest.

The implementation I added was unfortunately just wrong. The problem is that our internal representation cares about the bound/constraints/default inferred as a type expression, while the runtime attributes of the TypeVar object will return it as a value expression.

In the simplest case, this means that for a typevar [T: A], while we might store the bound as the Instance type A, the value of the __bound__ attribute should be typed as type[A].

Beguiled by the simple case, I slapped on a .to_meta_type() and called it a day. But this is just wrong in more complex cases; the meta-type is not always the same thing as the "type form" type. For example, the meta-type of A | B is type[A] | type[B], but the type-form type in this case would be an instance of types.UnionType.

In a future world with PEP 747 (TypeForm), where we have a typevar with a bound of A | B it would be correct for us to infer its __bound__ attribute as TypeForm[A | B], offering a nicer solution to this problem.

In the meantime, I think we should just follow the lead of other type checkers in falling back to the general typeshed types for these attributes, and I should bite the bullet and write a Rust test instead of an mdtest when I want to test internal representations of types that don't (yet, since we haven't implemented generics) have a visible effect in the type system.

Test Plan

Re-wrote an mdtest as a Rust test.

@carljm carljm added the red-knot Multi-file analysis & type inference label Nov 22, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@carljm carljm merged commit 4ba847f into main Nov 22, 2024
20 checks passed
@carljm carljm deleted the cjm/remove branch November 22, 2024 21:17
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.

2 participants