-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
PEP 767: Annotating Read-Only Attributes #4127
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks, this will be a great improvement to the type system!
peps/pep-9999.rst
Outdated
@@ -0,0 +1,546 @@ | |||
PEP: 9999 | |||
Title: Classes & protocols: Read-only attributes | |||
Author: <REQUIRED: list of authors' real names and optionally, email addrs> |
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.
I guess we forgot to update the "real names" part here. It's fine to say something like "Enegg [email protected]" (#4058). I'll look into updating the template.
peps/pep-9999.rst
Outdated
in older versions of Python. Mechanisms inspecting annotations may behave incorrectly | ||
when encountering ``ReadOnly``; in particular, the ``@dataclass`` decorator | ||
which `looks for <https://docs.python.org/3/library/dataclasses.html#class-variables>`_ | ||
``ClassVar`` will incorrectly treat ``ReadOnly[ClassVar[...]]`` as an 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.
Add a recommendation that users write ClassVar[ReadOnly[...]]
to avoid this?
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.
Sure.
I'd assume the wording of
We recommend that in such circumstances
ReadOnly
is placed as the inner type qualifier, likeClassVar[ReadOnly[...]]
Is it ok to use "we" (the authors) within the PEP? Or perhaps "It is recommended ..." would be better?
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.
"Authors should prefer ClassVar[ReadOnly[...]]
over ReadOnly[ClassVar[...]]
..." ?
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.
I'd write something like "To avoid issues with runtime introspection, use ClassVar[ReadOnly[...]]
instead of ReadOnly[ClassVar[...]]
.
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.
I assumed the recommendation should apply in the general case 🤔
"To avoid issues with introspection, prefer nesting ReadOnly
within other type qualifiers - use ClassVar[ReadOnly[...]]
instead of ReadOnly[ClassVar[...]]
."
Though it might give the wrong idea that you're always required to nest the type, or that one should put it within a generic that is not a type qualifier
peps/pep-9999.rst
Outdated
instead, they perform initialization in ``__new__`` or classmethods. The proposed | ||
feature won't be useful to them. | ||
|
||
OTOH, allowing assignment within ``__new__`` (and/or classmethods) could open way |
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.
Expand this abbreviation
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.
I have assumed I could be less formal in the Open Issues
section, my bad 😄
peps/pep-9999.rst
Outdated
|
||
This feature should also be supported by ``Final`` attributes. Specifically, | ||
``Final`` attributes initialized in a class body **should no longer** imply ``ClassVar``, | ||
and should remain assignable to within ``__init__``. |
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.
What is the motivation for this change? I'm opposed to changing the semantics of final, since this seems to have some problematic side effects. Currently class-level final attributes can be used to define constants, where the literal value is known statically:
class Foo:
FOO: Final = 5
BAR: Final = 6
def __init__(...): ...
x: Literal[5] = Foo.FOO # OK
(Mypyc even treats Foo.FOO
as a compile-time constant at runtime, replacing references to Foo.FOO
with 5.)
If we make the change as suggested in the PEP draft, it seems to imply that one of these is true:
- To infer the type of a class-level final attribute, a type checker will need to combine types from assignments in two scopes: the class body and the
__init__
method. - The code above would start generating a type error (since the inferred type of
Foo.FOO
would likely have to beint
if we must allow an arbitrary__init__
), breaking backward compatibility.
I think (2) above is not reasonable due to the backward compatibility break.
I also argue that (1) would also be quite a drastic change, since it adds "spooky action at a distance" where two assignments in two different parts of a file impact the type of an attribute. Here's what I'm thinking about:
class Foo
foo: Final = 5 # No longer can use initializer to infer type
# assume lots of other stuff
# ...
def __init__(self, foo: int | None) -> None:
if foo is not None:
self.foo = foo # This assignment means that "foo" has type "int"
I don't think there is a precedent for the above kind of type inference behavior, and it would be quite complex to implement in mypy at least. Also, to preserve backward compatibility, it would require new kind of type inference logic at least in mypy, again because there is no precedent for this kind of type inference behavior. This seems unnecessarily complicated considering that ReadOnly
seems to be a better fit for the above use case.
In summary, I think that this change seems out of place in the PEP, and increases implementation complexity significantly without a clear benefit that I can see.
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.
I also don't think there is a strong need for Final
and ReadOnly
to behave the same way, and don't feel we need to make this change to Final
in this PEP.
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.
I didn't initially plan on touching Final
. I might have got the wrong idea, but @erictraut's initial feedback seemed to suggest changing the spec of Final
to allow assignment at class level and within __init__
1:
I’m not convinced that we should allow double initialization — in both the class body and in init. Is there a good reason to allow both initialization techniques within the same class? Interestingly, mypy allows this for Final but pyright does not. The typing spec is currently silent on whether this should be allowed. Regardless of where we land on this decision, it would be good for this PEP to provide clarity for both ReadOnly and Final.
...hence the change. Likely should've discussed this more thoroughly.
Footnotes
-
Under the precondition that
ReadOnly
ships with the feature ↩
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.
Eric's feedback definitely did suggest clarifying the spec for Final
in this PEP, though it didn't advocate for any particular conclusion or change.
My feeling is that we don't need to change the behavior of Final
to make it the same as ReadOnly
; that causes churn, and I think it's OK if they behave differently. I see this difference as deriving naturally from their different meaning; Final
is really supposed to mean "this is the only assignment to this name, it is final", whereas ReadOnly
just means "once an instance of this class is constructed, this attribute can't be assigned to."
I (weakly) feel also that it's a bit out of scope for this PEP to clarify existing underspecified aspects of Final
, when those could just as well be clarified in a separate PR to the typing spec, approved by the typing council. But I'm not strongly opposed to this. If we do it, I would suggest collecting everything in this PEP related to Final
in a single section, which presents a comparison of the semantics of Final
and ReadOnly
, and any clarifications we want to make to the specification of Final
, all together.
peps/pep-9999.rst
Outdated
effectively making it a class variable. | ||
Type checkers may warn or suggest explicitly marking the attribute as a ``ClassVar``. | ||
|
||
Type checkers should warn on read-only attributes which may be left uninitialized |
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.
Type checkers should warn on read-only attributes which may be left uninitialized | |
Type checkers may warn on read-only attributes which may be left uninitialized |
Making sure that attributes are initialized is not an easy problem. While Python has not really been trying to do it much, Typescript has been trying to do it for a long time. A search in TypeScript issues for "strictPropertyInitialization" yields 98 open issues and 540 closed issues.
TypeScript has an annotation to assert that something is initialized when the type checker can't see it, and Python doesn't, so Python is less-equipped to handle this.
I would like it if type checkers started trying to move towards this. But I think making it a requirement in this PEP is not the right way to push towards it.
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.
This looks like it'll be a great addition!
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.
Thanks for your work on the PEP! Some things to sort out, but overall it looks strong to me.
peps/pep-9999.rst
Outdated
To my understanding, properly detecting this problem would require type checkers | ||
to keep track of the "level of initialization" of an object. |
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.
As discussed on Discourse, I think in practice this could be simplified to "was the object directly acquired from a call to super-type __new__
", which I think is reasonable to track.
And it could also be an option to allow type checker to accept this unsoundness, in exchange for a simpler implementation.
peps/pep-9999.rst
Outdated
This issue doesn't seem to impact ``__init__``, since it's rather uncommon to | ||
ever rebind ``self`` within it to any other object, and type checkers could | ||
flag the action as whole. |
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.
This should probably also mention "calling __init__
on an existing instance" as another action that would wrongly bypass readonly, if not flagged by type checkers.
peps/pep-9999.rst
Outdated
``Final`` in protocols | ||
---------------------- | ||
|
||
It's been `suggested <https://discuss.python.org/t/expanding-readonly-to-normal-classes-protocols/67359/45>`_ | ||
to clarify in this PEP whether ``Final`` should be supported by protocols. |
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.
It would be good to clear this up, but it seems out of scope for this PEP, and like something that could be handled with a PR to the spec and approval by the typing council; it doesn't need a PEP.
peps/pep-9999.rst
Outdated
|
||
.. [#invalid_typevar] | ||
The implied type variable is not valid in this context; it has been used for | ||
the ease of demonstration. See `ClassVar <https://typing.readthedocs.io/en/latest/spec/class-compat.html#classvar>`_. |
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.
What are we seeing ClassVar
for here?
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.
There's this section in the linked spec:
Note that a ClassVar parameter cannot include any type variables, regardless of the level of nesting: ClassVar[T] and ClassVar[list[set[T]]] are both invalid if T is a type variable.
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.
Ah, I see. To be honest, I think this whole footnote can be deleted. There is nothing in the section that it is footnoting that defines T
as specifically a typevar, requiring this disclaimer; T is just a stand-in name for "some arbitrary type name". This is very typical in typing documentation/spec and doesn't require further comment, IMO.
Co-authored-by: Jelle Zijlstra <[email protected]> Co-authored-by: Carl Meyer <[email protected]>
…ave a type, better immutability examples, yeet changes to Final
========== | ||
|
||
The Python type system lacks a single concise way to mark an :term:`attribute` read-only. | ||
This feature is present in other statically and gradually typed languages |
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.
I've changed "object-oriented" to "statically and gradually typed", hopefully it's not a misnomer
Assignment to a read-only attribute can only occur in the class declaring the attribute. | ||
There is no restriction to how many times the attribute can be assigned to. | ||
The assignment must be allowed in the following contexts: | ||
|
||
1. In ``__init__``, on the instance received as the first parameter (likely, ``self``). | ||
2. In ``__new__``, on instances of the declaring class created via a call | ||
to a super-class' ``__new__`` method. | ||
3. At declaration in the body of the class. | ||
|
||
Additionally, a type checker may choose to allow the assignment: | ||
|
||
1. In ``__new__``, on instances of the declaring class, without regard | ||
to the origin of the instance. | ||
(This choice trades soundness, as the instance may already be initialized, | ||
for the simplicity of implementation.) | ||
2. In ``@classmethod``\ s, on instances of the declaring class created via | ||
a call to the class' or super-class' ``__new__`` method. | ||
|
||
Note that a child class cannot assign to any read-only attribute of a parent class | ||
in any of the aforementioned contexts, unless the attribute is redeclared. |
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.
@carljm how do you like this?
Basic requirements (all PEP Types)
pep-NNNN.rst
), PR title (PEP 123: <Title of PEP>
) andPEP
headerAuthor
orSponsor
, and formally confirmed their approvalAuthor
,Status
(Draft
),Type
andCreated
headers filled out correctlyPEP-Delegate
,Topic
,Requires
andReplaces
headers completed if appropriate.github/CODEOWNERS
for the PEPStandards Track requirements
Python-Version
set to valid (pre-beta) future Python version, if relevantDiscussions-To
andPost-History
📚 Documentation preview 📚: https://pep-previews--4127.org.readthedocs.build/