Change the typing spec around string references - #2144
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 |
|
The non-buggy modern alternative is |
Ah perfect; that sounds like a good alternative to |
|
Generally the reference should be what happens if you don't stringify the annotation in 3.14+. Annotations are now always lazily evaluated. |
|
Perfect, thanks. I retried using If the intended reference is the non-stringified 3.14 results, then this PR matches the intended runtime behavior in 3.14 onward, which is great.
[1] using this code, if anyone wants to check my homework ;) I added |
|
@rchen152 Yes, I think that's a good summary of my suggestion above: to match the runtime behavior of 3.14 exactly, assuming that The alternative proposal on the table we could call "simple" -- that proposal is that all annotations (including stringified ones) should be resolved the way they would resolve at runtime if they were non-stringified annotations on 3.14+. To the extent that stringified annotations (mostly) eventually go away, the two proposals will converge, since they differ only in the handling of stringified annotations. The advantage of "simple" is that there is a single consistent model of name resolution for type checkers to implement. The disadvantages are:
The "compat" model preserves backwards-compatibility and full consistency with runtime behavior, allowing users to migrate to the new name-resolution semantics on their own schedule (by moving from stringified annotations to non-stringified PEP 647/747 in 3.14+). The cost is more complexity in type checker implementations. It increasingly seems to me that "compat" is the choice that better serves users of the type system. (There is a third possible choice, which is to fully embrace the backwards-incompatible change by also changing the runtime behavior of |
|
What's the consensus here between Carl's simple and compat proposal? I'm personally for "simple", because I don't like the complexity of I would like to move forward with this, because currently it feels like there are 6 competing ways between type checkers how we see forward references and I'm happy to move on with the majority of the council here. |
|
I am happy to move forward with either, if authors of other type checkers are OK with a backwards-incompatible change to their handling of stringified annotation forward references in the ambiguous cases. ty already implements "simple", so that's less work for me :) |
|
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.
| The string literal should contain a syntactically valid Python expression | ||
| (i.e., ``compile(lit, '', 'eval')`` should succeed) that is a valid | ||
| :term:`annotation expression`. Regardless of the Python version used, names | ||
| within the expression are looked up in the same way as they would be looked up | ||
| at runtime in Python 3.14 and higher if the annotation was not enclosed in a | ||
| string literal. Thus, name lookup follows general rules (e.g., the current | ||
| function, class, or module scope first, and the builtin scope last), but names | ||
| defined later within the same scope can be used in an earlier annotation. |
There was a problem hiding this comment.
I think we should explicitly specify that the same rules apply to annotations implicitly stringified by from __future__ import annotations. And this should be tested in the conformance suite, too.
There was a problem hiding this comment.
I added it to the spec. Please review. I will copy the tests/annotations_forward_refs.py to tests/annotations_forward_refs_future.py and add notes once we agree on the rest in this PR.
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
Co-authored-by: Carl Meyer <carl@oddbird.net>
|
|
||
| The presence of the import `from __future__ import annotations` must not | ||
| influence type checking. Annotations must be resolved in the exact same way as | ||
| if the import was not present. |
There was a problem hiding this comment.
Sorry, I should have been clearer in my last comment. I do not believe that this is the equivalence we should specify. Part of the point of using from __future__ import annotations is to enable forward references in type annotations, since the annotations are stringified at runtime and won't error. I think the equivalence that we should specify (and test) is that from __future__ import annotations causes all annotations to behave the same way as if they were explicitly stringified, using end-of-scope resolution.
For a pre-3.14 target, this should work with from __future__ import annotations but fail without it:
class C:
x: B
class B: ...There was a problem hiding this comment.
So this would only change the resolution for pre-3.14 targets? I assume that would just mean that the tests marked with # E?: Runtime error prior to 3.14: requires quotes must not be errors I assume?
Sorry for asking for clarification again. My draft for the spec would be:
from __future__ import annotations
----------------------------------
If `from __future__ import annotations` is present in a file, annotations are always resolved at the end of the scope, as they are in Python 3.14 and later.
What do you think?
Co-authored-by: Carl Meyer <carl@oddbird.net>
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.