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

Make joins of callables respect positional parameter names #4920

Merged
merged 3 commits into from Apr 28, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Collaborator

Michael0x2a commented Apr 17, 2018

This commit fixes #2777 -- specifically, it enhances joining callables to erase the names of positional arguments as necessary.

For example, consider the following program:

def f(x: int) -> int: ...
def g(y: int) -> int: ...

lst = [f, g]

Previously mypy would treat the final line as an error since 'f' and 'g' have different types due to the different parameter names.

Now, mypy infers that lst has type list[def (int) -> int], effectively erasing the parameter name from the inferred type.

This commit does not attempt change how handle keyword-only arguments are currently handled.

Make joins of callables respect positional parameter names
This commit fixes #2777 --
specifically, it enhances joining callables to erase the names of
positional arguments as necessary.

For example, consider the following program:

    def f(x: int) -> int: ...
    def g(y: int) -> int: ...

    lst = [f, g]

Previously mypy would treat the final line as an error since 'f'
and 'g' have different types due to the different parameter names.

Now, mypy infers that `lst` has type `list[def (int) -> int]`,
effectively erasing the parameter name from the inferred type.

This commit does not attempt to handle keyword-only arguments.
@msullivan

Looks good to me!

Couple small things.

ret_type=join_types(t.ret_type, s.ret_type),
fallback=fallback,
name=None)
def combine_arg_names(t: CallableType, s: CallableType) -> List[Optional[str]]:
# Note: assumes is_similar_type(t, s) is true

This comment has been minimized.

@msullivan

msullivan Apr 24, 2018

Collaborator

Add a docstring explaining what is going on and why it is necessary.

s = {1, 2, *(3, 4)}
t = {1, 2, *s}
reveal_type(s) # E: Revealed type is 'builtins.set[builtins.int*]'
reveal_type(t) # E: Revealed type is 'builtins.set[builtins.int*]'
[builtins fixtures/set.pyi]
[case testListLiteralWithFunctionsErasesNames]

This comment has been minimized.

@msullivan

msullivan Apr 24, 2018

Collaborator

Maybe add a test for the join_similar_callables path by joining functions whose argument types differ but have a nontrivial meet. (For example, if the arguments are Union[bytes, float] and Union[str, int], the argument for the joined function should be int)

@Michael0x2a

This comment has been minimized.

Collaborator

Michael0x2a commented Apr 27, 2018

@msullivan -- sorry for the delay! I made the changes you suggested.

@msullivan

A couple small doc nits but I think this looks good.

I'm being kind of picky about the docs partially because this kind of code (meets and joins in particular) always takes me a while to remember enough context for, which is a shame because they are underdocumented in every language implementation I've ever worked on ;)

(I also can never remember which is meet and which is join without looking it up. It just never sticks. I don't know why.)

and 's' had the signature (a: int, b: str, z: str) -> None, the
join of their argument names would be ["a", "b", None].
This information is then used to compute more accurate joins for

This comment has been minimized.

@msullivan

msullivan Apr 28, 2018

Collaborator

Meets and joins, right?

This comment has been minimized.

@Michael0x2a

Michael0x2a Apr 28, 2018

Collaborator

I think it's just joins -- this pull request doesn't touch meet.py.

(iirc, the meet of two types 't' and 's' would be some type that's basically a valid subtype of both. I couldn't think of a way of creating a callable that would be a valid subtype of two callables with different arg names without doing something messy with keyword arguments, so didn't attempt to modify that file.)

@@ -391,7 +391,17 @@ def combine_similar_callables(t: CallableType, s: CallableType) -> CallableType:
def combine_arg_names(t: CallableType, s: CallableType) -> List[Optional[str]]:
# Note: assumes is_similar_type(t, s) is true
"""Takes two callables and produces the meet of their argument names.

This comment has been minimized.

@msullivan

msullivan Apr 28, 2018

Collaborator

Extreme nit: this calls this operation a meet in one place and a join in another, and should use the same term twice. I think it is more of a meet than a join (since it is like an intersection, which are meets?). Just saying "combination" or something fine also.

This comment has been minimized.

@Michael0x2a

Michael0x2a Apr 28, 2018

Collaborator

Oh whoops, that's embarrassing -- I didn't mean to use the word 'meet' here...

But yeah, I agree it's confusing, especially since this function isn't even manipulating types. I reworded the comment completely and just used the phrase "compatible with" which is hopefully better.

@msullivan

Great! Thanks!

@msullivan msullivan merged commit a370550 into python:master Apr 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:make-callable-joins-understand-different-param-names branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment