-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[red-knot] Add internal documentation on kinds of types #13878
base: main
Are you sure you want to change the base?
Conversation
f11e55c
to
94bdbdb
Compare
94bdbdb
to
809739e
Compare
I wondered whether these docs would be better placed somewhere like https://typing.readthedocs.io/en/latest/. But I'm not sure it would be a great fit for that site, because:
|
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 writing this up! The core ideas look great, I think there are a few issues in some wordings.
I updated the PR: I abandoned the attempt to describe a generalised concept of "closed types"; instead, I now just attempt to describe final nominal types (classes explicitly decorated wtih |
I'm looking forward to read this but don't wait on my review :) |
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 good! Most of my comments are minor nits.
Co-authored-by: Carl Meyer <[email protected]>
196f77b
to
d1febbc
Compare
I've once again reworked the final section of this substantially, because I realised my last draft still had some inaccuracies. For example, I wrote that:
which is pretty obviously wrong when you think about enums: My conclusion from all this conversation is that while the concept of final classes might be useful, the concept of final types actually isn't that useful at all (and the name is somewhat misleading in some ways). So I scrapped the section defining a concept of "final types" and replaced it with a section describing "final classes". I'd appreciate it if you could give the last section another once-over @carljm! |
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 first several sections of this are really excellent, I have only the most minor of wording nits.
The last section is improved, but I still feel it suffers from a significant "so what?" factor, in that it spends IMO too many words discussing things that I don't think are particularly relevant to red-knot behavior. But this isn't a blocking concern.
... # x can be narrowed to `None` | ||
``` | ||
|
||
All Python singleton types are also sealed types and final types; nearly all are single-value types. |
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 still a reference to "final type" here, which may no longer make sense given later changes.
```py | ||
from enum import Enum, auto | ||
|
||
class Animals(Enum): |
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.
minor nit, not related to the core point: I think using plurals is generally not great naming practice for either enums or classes generally, suggest Animal
instead
For any two classes `X` and `Y`, if `X` is `@final` and `X` is not a subclass of `Y`, | ||
the intersection of the types `X & Y` is equivalent to `Never`, the empty set. | ||
This therefore means that for a type defined as being "all instances of the final class `X`", | ||
there are fewer ways of subtyping such a type than for most types in Python. |
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 still feel like this section buries the lede in a major way.
From red-knot's perspective, by far the most important thing about final class types is that we know exactly what methods and attributes they define, with what signatures (including especially dunder methods), and we know there can't be infinite possible subclasses overriding those methods and attributes. But we don't even mention that here, beyond a very vague reference in this phrase to "fewer ways of subtypign"; instead we go on to discuss (at relative length, with an example) the edge case of how some final class types can still have proper subtypes, although it remains unclear in the text why the existence or non-existence of proper subtypes should matter to red-knot or the reader.
|
||
Many singleton and sealed types are associated with final classes: the classes | ||
`types.EllipsisType`, `types.NotImplementedType`, `types.NoneType`, `bool`, | ||
and all enum classes are final classes. However, there are also many singleton | ||
and sealed types (some representable, but some merely theoretical) | ||
that cannot be defined as "all instances of a specific final class `X`". | ||
In the above enum, `Literal[Animals.LION]` is a singleton type and a sealed type, | ||
but would not fit this definition. |
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 would probably eliminate this entire paragraph. It's confusing, and really not clear what important point it makes.
Co-authored-by: Carl Meyer <[email protected]>
Co-authored-by: Carl Meyer <[email protected]>
Summary
This PR adds purely internal documentation to assist us, as red-knot developers. By having a common reference with precise definitions for certain terms, there's a reduced risk of misunderstanding.
Test Plan
pre-commit run -a