Skip to content

Add more ast.parse() mode overrides #6522

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 3 commits into from
Dec 10, 2021

Conversation

lancelote
Copy link
Contributor

Got a report about incomplete ast.parse() stub in PyCharm: PY-49663.

ast.parse() has four modes affecting the return type (see docs for ast.parse() and compile())

mode return type note
exec Module
eval Expression
func_type FunctionType Python >= 3.8
single Interactive

The fix looks trivial but mypy doesn't like the extra overrides (as was discovered in #3039) and complains about

Overloaded function signatures X and Y overlap with incompatible return types

I would appreciate any help while I am struggling to make it right. Atm looks like a bug or rather a limitation in mypy, or I am not there yet 🤔

eval -> Expression
func_type -> FunctionType
single -> Interactive
@JelleZijlstra
Copy link
Member

I think the problem is the = ... on the Literal overloads: that makes a call that omits the argument match these overloads. The Literal overloads should probably omit the default.

@github-actions

This comment has been minimized.

We need (simplified) to cover all cases in Python >= 3.8

- 1 case: parse(filename: str = ...)
- 4 cases: parse(filename: str, mode: Literal[...])
- 4 cases: parse(*, mode: Literal[...])
@lancelote
Copy link
Contributor Author

@JelleZijlstra Thank you! 🙇 I think I finally wrapped my mind around this following your comment and docs. Please correct me if my approach is mistaken and you meant a different thing.

So for Python >= 3.8 we need (simplified to consider only filename and mode parameters) the following 9 overrides

  • 1 override: parse(filename: str = ...)
  • 4 overrides with different literals: parse(filename: str, mode: Literal[...])
  • 4 overrides with different literals: parse(*, mode: Literal[...])

This way no two override clash and all the possible cases of calling parse() are covered, e.g.

r0 = parse()
r1 = parse("")
r2 = parse("", "exec")
r3 = parse(mode="exec")
r4 = parse("", mode="exec")
r5 = parse(filename="")
r6 = parse(filename="", mode="exec")

@github-actions

This comment has been minimized.

@lancelote lancelote marked this pull request as ready for review December 9, 2021 08:32
stdlib/ast.pyi Outdated
filename: str | bytes = ...,
mode: Literal["eval"] = ...,
filename: str | bytes,
mode: Literal["exec"],
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why you needed to split this from the previous overload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse(filename: str = ..., mode: Literal[...]) is impossible as non-default argument cannot follow the default one. If we add = ... to mode we will return to the situation of the overlapping signatures. If we just remove the first overload (parse(filename: str = ...)) than e.g. r0 = parse() is not allowed. Did I misunderstand your idea?

@lancelote
Copy link
Contributor Author

Simplified visualization of the overloads and the matching calls

parse(filename: str | bytes = ...)
  ├── parse()
  ├── parse("")
  └── parse(filename="")

parse(filename: str | bytes, mode: Literal["exec"])
  ├── parse("", "exec")
  ├── parse("", mode="exec")
  └── parse(filename="", mode="exec")

parse(*, mode: Literal["exec"])
  └── parse(mode="exec")

@AlexWaygood
Copy link
Member

You can do it more simply, like this:

if sys.version_info >= (3, 8):
    @overload
    def parse(
        source: str | bytes,
        filename: str | bytes = ...,
        mode: Literal["exec"] = ...,
        *,
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> Module: ...
    @overload
    def parse(
        source: str | bytes,
        filename: str | bytes,
        mode: Literal["eval"],
        *,
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> Expression: ...
    @overload
    def parse(
        source: str | bytes,
        filename: str | bytes,
        mode: Literal["func_type"],
        *,
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> FunctionType: ...
    @overload
    def parse(
        source: str | bytes,
        filename: str | bytes,
        mode: Literal["single"],
        *,
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> Interactive: ...
    @overload
    def parse(
        source: str | bytes,
        *,
        mode: Literal["eval"],
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> Expression: ...
    @overload
    def parse(
        source: str | bytes,
        *,
        mode: Literal["func_type"],
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> FunctionType: ...
    @overload
    def parse(
        source: str | bytes,
        *,
        mode: Literal["single"],
        type_comments: bool = ...,
        feature_version: None | int | _typing.Tuple[int, int] = ...,
    ) -> Interactive: ...

else:
    @overload
    def parse(source: str | bytes, filename: str | bytes = ..., mode: Literal["exec"] = ...) -> Module: ...
    @overload
    def parse(source: str | bytes, filename: str | bytes, mode: Literal["eval"]) -> Expression: ...
    @overload
    def parse(source: str | bytes, filename: str | bytes, mode: Literal["single"]) -> Interactive: ...

Tests:

# These fail at runtime, should fail mypy also
parse()
parse(mode="exec")
parse(filename="")
parse(filename="", mode="exec")

# Revealed type of all of these is '_ast.Module'
reveal_type(parse(""))
reveal_type(parse(source=""))
reveal_type(parse("", "exec"))
reveal_type(parse("", mode="exec"))
reveal_type(parse(source="", mode="exec"))

You can run this in isolation on MyPy playground here.

@lancelote
Copy link
Contributor Author

@AlexWaygood Thank you, I understand my mistake now 🤦

@AlexWaygood
Copy link
Member

@AlexWaygood Thank you, I understand my mistake now 🤦

No worries, it's a monster of a function to type!!

The default case should have both `filename` and `mode` as keyword
parameters, this way one can omit separate overloads for exec mode
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 739a052 into python:master Dec 10, 2021
@JelleZijlstra
Copy link
Member

Thank you!

@lancelote lancelote deleted the ast-parse-modes branch December 10, 2021 17:25
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.

3 participants