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] annotated assignments without RHS in stubs should still be bindings #16264

Open
carljm opened this issue Feb 19, 2025 · 4 comments
Open
Labels
red-knot Multi-file analysis & type inference

Comments

@carljm
Copy link
Contributor

carljm commented Feb 19, 2025

Description

Otherwise, we get into trouble when a special form (say Protocol) is used within the global scope of the same stub file where it is defined, because that's just a local use, which only respects bindings.

This doesn't affect special forms that are used in annotation position (e.g. ClassVar) because all annotation expressions are lazy in stub files, meaning they use the public type.

@carljm
Copy link
Contributor Author

carljm commented Feb 21, 2025

We probably also want to do this generally in stubs, including in class body scopes, because people often omit right-hand-sides from annotated assignments in stubs, so we can't assume that x: int in a class body in a stub is an instance-only attribute.

Perhaps in an ideal world, the recommended approach in stubs would be to include class-level defaults (or at least ... as RHS) so that type checkers can distinguish pure-instance variables from instance-or-class variables. But since this isn't the current practice, we probably need to adapt.

This cropped up as causing a false positive in tomllib, see #16036 (comment)

@mishamsk
Copy link
Contributor

btw. in one of the early versions of #16036 I actually had logic that special cased stubs & ABC's, but @sharkdp asked to remove it here #16036 (comment) - I have the code stashed, including the ABC wiring - so again, just saying, I'd be happy to re-wire the boundness/declaredness.

@carljm
Copy link
Contributor Author

carljm commented Feb 21, 2025

I think @sharkdp was right that we should handle ClassVar specially (which the PR now does). And I think we will probably want to implement the special case for stubs at a deeper layer, since it should impact both our interpretation of class vs instance attributes in class bodies, and also our interpretation of RHS-less annotated assignments at module level (as discussed in the OP here.)

By "a deeper layer" I mean that already when we create the initial Definitions in building the semantic index, we should always consider an annotated assignment in a stub with no RHS to be both a binding and a declaration, rather than just a declaration. And then in type inference we should record it as both a binding and a declaration of the annotated type. This reflects the fact that in general, in a stub file, x: int is used to imply x: int = ... where ... is something of type int.

I think implementing this generalized approach will also help with resolution of the symbol boundness-vs-declaredness issue. There's a TODO comment in symbol_by_id that says this needs design work, and stubs might need to behave differently, but I think this issue captures the precise way in which stubs should behave differently -- if we special-case stubs in Definition creation in the way described above, we shouldn't need to special-case them in how we resolve symbols.

@mishamsk
Copy link
Contributor

By "a deeper layer" I mean that already when we create the initial Definitions in building the semantic index

Oh, I'd love to implement this way. I haven't touched semantic index so far, so will be a great opportunity to take my relationship with it to the next level!

There's a TODO comment

Yeah, I've seen that from the very beginning ;-)

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

No branches or pull requests

2 participants