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

Overload ast.parse to recognize that mode=exec means Module return. #3039

Merged
merged 2 commits into from Jun 17, 2019

Conversation

carljm
Copy link
Member

@carljm carljm commented Jun 8, 2019

With mode="exec" (which is the default mode), ast.parse always returns an ast.Module. It's irritating to always have to cast this.

Verified this change with a small file:

import ast

def foo() -> ast.Module:
    return ast.parse("a = 1")

This raises an error (expected Module, got AST) on typeshed master, passes with this PR.
Also added explicit mode="exec", same results.
With mode="eval", error on both master and this PR.

@carljm
Copy link
Member Author

carljm commented Jun 8, 2019

I tried taking this one step further to also recognize that mode="single" always returns ast.Interactive, but I can't seem to avoid mypy telling me that Literal["exec"] and Literal["single"] are "overlapping types with incompatible return types." Which doesn't make sense to me; maybe a bug in mypy needs fixing first? Anyway, since this partial version is better than the status quo for the much more common case of "exec" mode, I still think it should be merged.

Also, I think that the linter that enforces that parameter defaults in stub files must be ... is outdated with literal types. In this case, I'm able to make it happy because mypy just assumes an overload match if the parameter is omitted even without knowing the real default. I guess this means the workaround of putting the default value overload always first should work? It still seems to me that in the presence of Literal types, it's reasonable and more comprehensible to include the default in the stub, as in principle it is relevant to the typechecker, and relying on "default overload first" seems excessively implicit. I opened PyCQA/flake8-pyi#28 to track this.

@srittau srittau added the reason: inexpressable Closed, because this can't be expressed within the current type system label Jun 13, 2019
@srittau
Copy link
Collaborator

srittau commented Jun 13, 2019

Literal is not yet supported in typeshed, see #2913.

@carljm
Copy link
Member Author

carljm commented Jun 14, 2019

Per recent typing-sig thread, sounds like it's time for that to change :) (FWIW it's already clear that it doesn't cause trouble for pytype, because otherwise typeshed pytype tests would not pass).

@srittau srittau removed the reason: inexpressable Closed, because this can't be expressed within the current type system label Jun 14, 2019
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, just one optional suggestion.

@@ -23,9 +24,18 @@ class NodeTransformer(NodeVisitor):
_T = TypeVar('_T', bound=AST)

if sys.version_info >= (3, 8):
@overload
def parse(source: Union[str, bytes], filename: Union[str, bytes] = ..., mode: Literal["exec"] = ...,
type_comments: bool = ..., feature_version: int = ...) -> Module: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: You could also add the following overload, since exec is the default mode:

    @overload
    def parse(source: Union[str, bytes], filename: Union[str, bytes] = ..., *,
              type_comments: bool = ..., feature_version: int = ...) -> Module: ...

Similar for the Python 3.7 version.

(Also a style nit: Please don't add empty lines between overloads.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I didn't get back to this sooner, thanks for the merge! Regarding this comment, I tried adding that overload and IIRC mypy complained about overlapping overloads. It seems that implicitly due to ordering of overloads, mypy already treats this as the "default" one. (I think the clearer way to express this to typecheckers would be to actually include the default value of the optional argument in the stub).

@srittau srittau merged commit fcb97fe into python:master Jun 17, 2019
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.

None yet

2 participants