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

Annotated: propose better terminology #1769

Merged
merged 11 commits into from
Jun 25, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Jun 6, 2024

Copy link
Collaborator

@erictraut erictraut 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 doing this. I think the term "metadata" is useful here, and it improves the readability of the spec.

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved

* Multiple type annotations are supported (``Annotated`` supports variadic
Annotated[BaseType, Metadata1, Metadata2, ...]
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I mentioned in discourse, I have misgivings about the term "base type" here. I'd prefer if we simply used the term "type". The problem with "base type" is that it implies that the type is modified by the metadata. First, this isn't the case for static type checkers. Second, metadata doesn't need to (and often doesn't in practice) refer to the type; it can apply to the symbol that the annotation is applied to — or something else completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's important for clarity to have some term that specifically refers to what I call the base type. Would using "inner type" address your concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think "inner type" is much better than "base type". I think it reads fine without giving it an adjective. Annotated allows one to specify a "type" and some metadata that may or may not have anything to do with the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would find it difficult to explain how PEP 746 works without some term specifically for the base type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option: We could call it the "base expression". That actually works better than type because the "base type" may be an annotation expression, not a type expression (e.g., x: Annotated[Final[int], "some metadata"] is legal).

Copy link
Member

Choose a reason for hiding this comment

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

I like "inner type" or "wrapped type" for this. I agree with Eric in not particularly liking "base type"

Copy link
Collaborator

Choose a reason for hiding this comment

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

annotation expressions can be wrapped in Annotated[] according to the grammar I wrote

Oh, I didn't catch that. I think that's a mistake. The first argument to Annotated must be a valid type. This is spelled out clearly later in the spec. The expression Final[int] is not a valid type, so it doesn't make sense for it to be used in Annotated. I think should be fixed this in the grammar, and type checkers should enforce this rule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the part of the spec that says that the first argument to Annotated must be a valid type is out-of-date. It looks like that restriction was relaxed in this issue. Elsewhere, the spec says that Final may be wrapped by Annotated (near the bottom of this section) and that Required, NotRequired, and Annotated can appear in any nesting order (here).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes, I forgot about those provisions elsewhere in the spec. So yes, it sounds like we have an inconsistency here. I think it would be cleaner to allow only type expressions as the first type argument to Annotated, but I guess that ship has sailed, and it would be disruptive to undo that decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've updated pyright to honor Final when it's nested in Annotated: microsoft/pyright#8097.

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved

* Multiple type annotations are supported (``Annotated`` supports variadic
Annotated[BaseType, Metadata1, Metadata2, ...]
Copy link
Member

Choose a reason for hiding this comment

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

I like "inner type" or "wrapped type" for this. I agree with Eric in not particularly liking "base type"

docs/spec/qualifiers.rst Show resolved Hide resolved
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.

This is definitely clearer, thank you!

I would prefer to be a bit more precise in our usage of the word "type" in the typing spec. A type specifies a set of possible runtime values. A type expression specifies a type. An annotation expression specifies more than just a type. int is a type; Final[int] is not a type, it's an annotation (though it does include a type, as well as additional qualifiers). So I would not prefer to use the term "base type" for the leftmost thing in Annotated, or to casually refer to Annotated as wrapping a "type".

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Show resolved Hide resolved
@JelleZijlstra JelleZijlstra added the Typing Council decision Needs to be approved by the Typing Council. Do not merge until approved. label Jun 15, 2024
@JelleZijlstra JelleZijlstra marked this pull request as ready for review June 15, 2024 03:58
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks below, but basically this looks great to me. I still prefer "wrapped expression" or "underlying expression" to "base expression", but I think "base expression" is also fine

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
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.

This looks good to me!

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@erictraut erictraut left a comment

Choose a reason for hiding this comment

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

I left a few minor comments. I don't feel that strongly about any of them. If you decide to ignore them for this PR, that's OK with me.

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
@@ -272,33 +283,54 @@ details of the syntax:
SmallInt = Annotated[int, ValueRange(0, 100)]
SmallInt(1) # Type error

:pep:`593` and an earlier version of this specification used the term
"annotations" instead of "metadata" for the extra arguments to
``Annotated``. The term "annotations" is deprecated to avoid confusion
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it a bit odd to say that the "term is deprecated". The typing spec is the source of truth here, so is it necessary to clarify that it deviates in terminology from the PEP or from earlier versions of the spec? We don't do this elsewhere in the spec where we have clarified or updated terminology.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's helpful in cases where another term was previously in use. I'm not sure there is any comparable situation elsewhere in the spec.

docs/spec/qualifiers.rst Outdated Show resolved Hide resolved
@JelleZijlstra
Copy link
Member Author

The Typing Council has signed off, so I'm merging this.

@JelleZijlstra JelleZijlstra merged commit ff6bee3 into python:main Jun 25, 2024
5 checks passed
@JelleZijlstra JelleZijlstra deleted the annotated branch June 25, 2024 14:46
@erictraut
Copy link
Collaborator

Thanks @JelleZijlstra. I scanned through the changes again to see if they required any modifications or additions to the conformance tests, but I didn't see any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing Council decision Needs to be approved by the Typing Council. Do not merge until approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants