-
Notifications
You must be signed in to change notification settings - Fork 243
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
Conformance tests and spec change for generic type erasure #1589
Conversation
@@ -333,15 +333,15 @@ argument(s) is substituted. Otherwise, ``Any`` is assumed. Example:: | |||
|
|||
class Node(Generic[T]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The paragraph above could probably be improved. For instance, without the change I make here, pyright would infer Node()
to be Node[None]
, and I think that should be legal.
That said, handling of default values for generics in general is underspecified (mypy handles them poorly, but in part because it wasn't clear what the behaviour should be python/mypy#3737)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this part of the spec was written, mypy defaulted to "implicit-optional", which would give you the same effect as adding | None
. Since then, the spec was updated to recommend against "implicit-optional", but this code sample wasn't updated. I'd be in favor of just updating the spec to fix the sample.
p.x # Ok (evaluates to None) | ||
Node[int]().x # Ok (evaluates to None) | ||
p.x # Ok (evaluates to int) | ||
Node[int]().x # Ok (evaluates to int) | ||
p.x = 1 # Ok, but assigning to instance attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These just seem straight up incorrect to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. Looks like a spec bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to spec/generics.rst LGTM.
@hauntsaninja, is this PR ready to merge, or do you have additional work to do on it? |
This is ready to merge, I just wanted to leave open for a few days since I change the meaning of the spec. (And I still owe tests for "Abstract generic types" and "Type variables with an upper bound") |
No description provided.