Skip to content
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
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me but I was confused by some of the wording

Comment on lines 360 to 361
``A``. In a class method, the precise type of the first argument
cannot be represented using the available type notation.
Copy link
Contributor

@stroxler stroxler Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure what this last sentence means.

I think type[Self] is a fairly precise type - it's a type wrapping a type variable bound above by type[<current-class>] but can be narrowed on callsites where we know more.

That seems as precise as I can imagine being? And I don't think it's any less precise than the type of self, which behaves similarly.

Copy link
Collaborator Author

@hauntsaninja hauntsaninja Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sentence is from PEP 484, it confused me as well. I thought it was maybe talking about the usual thing where args to cls(...) are hopeless, but I don't really know. Maybe we just remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sentence makes no sense to me either, I think we should remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the last sentence, I think it looks reasonable.

I do think the spec should eventually be updated to mandate that self has the implied type Self and cls has the implied type type[Self]. Many important use cases involving the Self type break if this isn't the case. In retrospect, PEP 673 (which introduced the Self type) should have specified this IMO. But if you'd prefer to hold off on that change, then your proposed wording seems fine.

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying this!

Comment on lines +356 to +358
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]``.
Copy link
Member

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 for Self 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 the Self 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 the Self spec to handle this in depth.

Suggested change
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]``.
assumed to have the type :ref:`Self <self>`, and for class methods ``type[Self]``.

Copy link
Member

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 ).

Copy link
Member

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.)

Copy link
Member

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.

Comment on lines +355 to +358
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]``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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]``.
If the argument is not annotated, then for instance methods it may be
inferred to have either the type of the containing class, or the type :ref:`Self
<self>`. For class methods it may be inferred to have either the type object
type corresponding to the containing class object, or ``type[Self]``.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants