-
Notifications
You must be signed in to change notification settings - Fork 238
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
Permit inferring Self for unannotated self #1860
Draft
hauntsaninja
wants to merge
3
commits into
python:main
Choose a base branch
from
hauntsaninja:permit-self
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -27,14 +27,10 @@ general type possible, or ignored, by any type checker. | |||||||||||||||||
It is recommended but not required that checked functions have | ||||||||||||||||||
annotations for all arguments and the return type. For a checked | ||||||||||||||||||
function, the default annotation for arguments and for the return type | ||||||||||||||||||
is ``Any``. An exception is the first argument of instance and | ||||||||||||||||||
class methods. If it is not annotated, then it is assumed to have the | ||||||||||||||||||
type of the containing class for instance methods, and a type object | ||||||||||||||||||
type corresponding to the containing class object for class methods. | ||||||||||||||||||
For example, in class ``A`` the first argument of an instance method | ||||||||||||||||||
has the implicit type ``A``. In a class method, the precise type of | ||||||||||||||||||
the first argument cannot be represented using the available type | ||||||||||||||||||
notation. | ||||||||||||||||||
is ``Any``. An exception to the above is the first argument of | ||||||||||||||||||
instance and class methods (conventionally named ``self`` or ``cls``), | ||||||||||||||||||
which type checkers should assume to have an appropriate type, as per | ||||||||||||||||||
:ref:`annotating-methods`. | ||||||||||||||||||
|
||||||||||||||||||
(Note that the return type of ``__init__`` ought to be annotated with | ||||||||||||||||||
``-> None``. The reason for this is subtle. If ``__init__`` assumed | ||||||||||||||||||
|
@@ -353,10 +349,14 @@ types cannot be specified:: | |||||||||||||||||
Annotating instance and class methods | ||||||||||||||||||
------------------------------------- | ||||||||||||||||||
|
||||||||||||||||||
In most cases the first argument of class and instance methods | ||||||||||||||||||
does not need to be annotated, and it is assumed to have the | ||||||||||||||||||
type of the containing class for instance methods, and a type object | ||||||||||||||||||
type corresponding to the containing class object for class methods. | ||||||||||||||||||
In most cases the first argument of instance and class methods | ||||||||||||||||||
(conventionally named ``self`` or ``cls``) does not need to be annotated. | ||||||||||||||||||
|
||||||||||||||||||
If the argument is not annotated, then for instance methods it is | ||||||||||||||||||
assumed to have the type of the containing class or :ref:`Self | ||||||||||||||||||
<self>`, and for class methods the type object type corresponding to | ||||||||||||||||||
the containing class object or ``type[Self]``. | ||||||||||||||||||
Comment on lines
+355
to
+358
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
In addition, the first argument in an instance method can be annotated | ||||||||||||||||||
with a type variable. In this case the return type may use the same | ||||||||||||||||||
type variable, thus making that method a generic function. For example:: | ||||||||||||||||||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't think it is accurate to make an explicit equivalence here between the type
Self
and "the type of the containing class". This is not actually what the specification forSelf
says:Self
is not "the type of the containing class," it is "a typevar with a bound of the containing class". This distinction is important for how theSelf
type is handled when used in other arguments, and in inheritance cases.I suggest we simply don't attempt a mini-definition of the meaning of
Self
here, and allow theSelf
spec to handle this in depth.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 interpreted this as not an attempt to define Self, but a way to permit type checkers to use an alternative to inferring Self. A type checker may either infer
self
to be of the type of the containing class (as mypy does: https://mypy-play.net/?mypy=latest&python=3.12&gist=ab4b370dd76d1d446bde35bca7de7bf6 ), or of type Self (as pyright does: https://pyright-play.net/?strict=true&code=MYGwhgzhAEAaBcAoaLoBMCmAzaBbDALgBYAUEGIWAlNALQB80AcgPYB2GSq30AThgDcMYEAH0CATwAOGMhWpA ).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.
Oh! I see how that could be the interpretation of the "or" here. If that's the intention, then I suggest we make it a bit more explicit. (I'll make a separate suggested edit, since it would involve one more line than I included in this one.)
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.
Personally I think inferring
Self
is the better option here, and we could also consider just specifying that, but I see that may be a bigger change than was intended in this PR.