Skip to content

gh-113255: Clarify docs for typing.reveal_type #113286

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

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

note35
Copy link
Contributor

@note35 note35 commented Dec 19, 2023

Why?

The current comment mentioned reveal_type() is supported by other type checkers, and with this we can run without runtime errors. This may mislead readers the fact that CPython's reveal_type() does not necessary to infer the same type as other type checkers (even though runtime is mentioned many times, but it's not obvious).

Initial proposal

The comment by Carl Meyer resolves this confusion. So I suggest adding the comment (with a bit modification to make it concise) to the doc.


📚 Documentation preview 📚: https://cpython-previews--113286.org.readthedocs.build/

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.

Thanks! Some initial feedback:

@corona10 corona10 added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Dec 19, 2023
@carljm
Copy link
Member

carljm commented Dec 19, 2023

I wonder if we can get the same benefit even more concisely by tweaking the existing documentation for this function to be clearer. Specifically, I think the first sentence ("Reveal the inferred static type of an expression.") is potentially misleading, and lends itself to the confusion you had, because it can easily be read to imply that that is what the function does at runtime.

I can see two potential ways to adjust this. One would be to simply move the qualifier "inferred static" from the opening sentence to the second paragraph, so that it reads like this:

Reveal the type of an expression.

When a static type checker encounters a call to this function, it emits a diagnostic with the inferred static type of the argument.

Alternatively, if we want to continue to feature the static type checker use as the primary purpose (which it is), we could clarify the opening sentence like this instead (using a similar form as already used in the documentation of assert_type in this same file):

Ask a static type checker to reveal the statically inferred type of an expression.

Which makes it clear that we are here describing the effect on static type checkers.

I think a clarification like one of the above, plus the existing clear use of the term "runtime type" below, might be sufficient to avoid this misunderstanding, without needing to add an entirely new paragraph. I find the newly added paragraph in the current PR cumbersome because a) the first sentence basically repeats what was just said immediately above, and b) the rest of it goes into details about specifically how runtime types and static types can differ, which is a larger topic that I don't think the documentation for reveal_type needs to dive into.

If we do still want an additional clarification at the end, I would just add one sentence, something like this:

The runtime type may be different from (more or less specific than) the type statically inferred by a type checker.

What do you both think? @AlexWaygood @note35

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.

Thanks @note35, this is looking much better now! Could you also make equivalent changes to the docstring here?

cpython/Lib/typing.py

Lines 3304 to 3317 in 5a7cc66

"""Reveal the inferred type of a variable.
When a static type checker encounters a call to ``reveal_type()``,
it will emit the inferred type of the argument::
x: int = 1
reveal_type(x)
Running a static type checker (e.g., mypy) on this example
will produce output similar to 'Revealed type is "builtins.int"'.
At runtime, the function prints the runtime type of the
argument and returns it unchanged.
"""

@note35
Copy link
Contributor Author

note35 commented Dec 20, 2023

@carljm Thanks for the comments. I basically agree with you and consider assert_type is better explained. Newest revision contains the modification based on your suggestion.

Just one humble insight I want to share for both assert_type and modified reveal_type, maybe Python doc should put runtime first, and type checker second as a supplement. While I don't know if it makes sense to change now, most users use them because other static type checkers use them BEFORE 3.11. In the future, we can expect usages are done without being static type checked, they may cause another confusion that why the doc prioritize type checker's usage, then the actual CPython's usage, while those new usages need only python's runtime type.

@note35
Copy link
Contributor Author

note35 commented Dec 20, 2023

Thanks @note35, this is looking much better now! Could you also make equivalent changes to the docstring here?

cpython/Lib/typing.py

Lines 3304 to 3317 in 5a7cc66

"""Reveal the inferred type of a variable.
When a static type checker encounters a call to ``reveal_type()``,
it will emit the inferred type of the argument::
x: int = 1
reveal_type(x)
Running a static type checker (e.g., mypy) on this example
will produce output similar to 'Revealed type is "builtins.int"'.
At runtime, the function prints the runtime type of the
argument and returns it unchanged.
"""

Done. I made a change for only the first two lines of the comment, please let me know if it's sufficient.

@AlexWaygood AlexWaygood changed the title gh-113255: Append more description to reveal_type() function. gh-113255: Clarify docs for typing.reveal_type Dec 20, 2023
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.

LGTM, thanks! @carljm, anything more from you?

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.

Looks great, thanks all!

@AlexWaygood AlexWaygood merged commit 11ee912 into python:main Dec 20, 2023
@miss-islington-app
Copy link

Thanks @note35 for the PR, and @AlexWaygood for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 20, 2023
(cherry picked from commit 11ee912)

Co-authored-by: Kir <[email protected]>
Co-authored-by: AlexWaygood <[email protected]>
@miss-islington-app
Copy link

Sorry, @note35 and @AlexWaygood, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 11ee912327ef51100d2a6b990249f25b6b1b435d 3.11

@bedevere-app
Copy link

bedevere-app bot commented Dec 20, 2023

GH-113323 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Dec 20, 2023
AlexWaygood added a commit that referenced this pull request Dec 20, 2023
…113323)

gh-113255: Clarify docs for `typing.reveal_type` (GH-113286)
(cherry picked from commit 11ee912)

Co-authored-by: Kir <[email protected]>
Co-authored-by: AlexWaygood <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Dec 20, 2023

GH-113326 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 20, 2023
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Dec 20, 2023
AlexWaygood added a commit that referenced this pull request Dec 20, 2023
ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants