Repository navigation
Make it an error to use a class-attribute type var outside a type - #3105
Conversation
Previously, e9d28a0 fixed a crash when you tried to access a class-attribute type variable. The test in that commit involved assigning a variable the value of the typevar. It resulted in no crash, but rather treating the variable as being an instance of the type the typevar bound to, later, which is incorrect. Instead, this PR treats such an assignment as an error, and gives you the same message as when you try to alias a typevar directly. Also test a *correct* alias with the typevar access method in question -- it works.
| if isinstance(node, RefExpr): | ||
| # Note that this misses the case where someone tried to use a | ||
| # class-referenced type variable as a type alias. It's easier to catch | ||
| # that one in checkmemeber.py |
There was a problem hiding this comment.
small typo: checkmember.py
There was a problem hiding this comment.
meme-ber. Thank you.
|
I'm not exactly sure about the wording of the error here, suggestions welcome. |
| def f(self, x: T) -> T: | ||
| A = C.T | ||
| return x | ||
| L = List[C.T] # a valid type alias |
There was a problem hiding this comment.
Would doing y: C.T = x be valid?
There was a problem hiding this comment.
Yes, and good point -- that's not "as a parameter to a type" that's "as a type"
How to express "Don't try and use this at runtime"?
There was a problem hiding this comment.
I guess "Is only valid when used as a type or a parameter to a type in a type alias" covers all the bases, but is wicked confusing if I don't already know exactly what it's talking about.
There was a problem hiding this comment.
Some time ago we had "... is invalid in runtime context" for subscripted generics. Maybe it is better to say "... can not be used in runtime context, only in type context".
There was a problem hiding this comment.
Oh! How about
- "Type variable 'C.T' is not a valid expression"
- "Type variable 'C.T' cannot be used as an expression"
?
There was a problem hiding this comment.
The second option sounds good.
There was a problem hiding this comment.
Ok, rolling with @ilevkivskyi's suggestion for now.
There was a problem hiding this comment.
... as I incur more and more test changes as I can't decide. I think I like how it is now.
|
Thanks for the PR! LGTM |
4 participants
Previously, e9d28a0 fixed a crash when you tried to access a class-attribute
type variable. The test in that commit involved assigning a variable the value
of the typevar. It resulted in no crash, but rather treating the variable as
being an instance of the type the typevar bound to, later, which is incorrect.
For example (and this is the test defined in e9d28a0, modified)
Instead, this PR treats such an assignment as an error -- any reference to such a classvar tvar outside trying to construct a type out of it is an error.
Also test a correct alias with the typevar access method in question -- it
works.