Change the typing spec around string references - #2144
davidhalter wants to merge 27 commits into
Conversation
|
@JelleZijlstra Could you please pre-review this? What do you think about this spec change? |
|
I think I have integrated all the changes. Is it time to open an issue on the Typing Council’s issue tracker asking for a decision? |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
carljm
left a comment
There was a problem hiding this comment.
One wording nit, one formatting nit, and one conformance suite nit :) But overall this looks great to me.
rchen152
left a comment
There was a problem hiding this comment.
Looks good to me - much more consistent and clearly specified than before
carljm
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks @davidhalter for getting this clarified.
|
I have integrated all of Carl's suggestions. I will update the conformance tests as soon as the typing council approves this change. If I update it now we probably just run into merge conflicts, since especially pyrefly changes a lot. @carljm Please let me know if you think something needs more work. |
|
Hi, can someone explain the intent of this change to me? Given, under python 3.14: does this change propose that it would be impossible for |
|
Yes |
|
are you going to change the behavior of edit: the pep is pep-749 |
|
I prefer "simple", although I'd also be okay with "compat". Given the concerns Carl raised, I was going to try implementing it in Pyrefly to make sure I didn't run into any showstopping issues before offering an opinion, but if it's already how ty works, I'm much less worried on that front. (I'll still implement it in Pyrefly as soon as I can and report back if I run into unanticipated problems, but I don't think we need to wait on that.) |
e7eb340 to
434caf2
Compare
434caf2 to
927aa61
Compare
|
I ran the tests with all the type checkers and added notes. |
|
@carljm , I think you're the only one who has not ticked off python/typing-council#51. I guess that there won't be more feedback here, so I feel like if you're ok with this change, we should be able to merge. |
carljm
left a comment
There was a problem hiding this comment.
Did another review pass on this.
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
carljm
left a comment
There was a problem hiding this comment.
Couple remaining nits, but looks great, thank you!
the only comment that came with this review has been addressed
|
Ok now it should be ready. At least now this file shows how different the type checkers think about forward references: None of the type checkers agree on the errors 😄 There are probably a few new notes that might not be great. Will merge after a last OK from Carl. Thanks for all the feedback! |
| @@ -0,0 +1,26 @@ | |||
| conformant = "Partial" | |||
| notes = """ | |||
| Names in annotations that refer to definitions after them are not resolved in the precence of `from __future__ import annotations` | |||
There was a problem hiding this comment.
Could we apply the same wording correction here as in the Pyrefly notes: “Forward references to nested classes are not resolved”? The module-level ClassA and ClassC forward references succeed, so this still overstates the limitation. (Also, "precence" is currently mis-spelled.)
| @@ -0,0 +1,21 @@ | |||
| conformant = "Partial" | |||
| notes = """ | |||
| Forward references to nested classes are not resolved | |||
There was a problem hiding this comment.
Could we also add a short note covering the newly exposed shadowing failures? The results now record missing errors for str: str = "" and z: int = 0 before def int (lines 37 and 39), but the notes only describe nested-class failures. Something like “Resolves some class annotations to outer names instead of shadowing class bindings” would cover those cases. Zuban's notes have the same omission.
I added this after the discussion here: https://discuss.python.org/t/annotation-string-references-in-class-scope-in-conformance-tests/105439
I'm not 100% sure about the wording, but I hope the direction is fine. I would like to gather some feedback before presenting this to the typing council.
Please also merge #2139 before this pull request. Otherwise it will be very hard to update Zuban's conformance test results in this pull request.