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

Should have a way to annotate an Optional carryover method. #3295

Closed
euresti opened this issue May 1, 2017 · 12 comments
Closed

Should have a way to annotate an Optional carryover method. #3295

euresti opened this issue May 1, 2017 · 12 comments
Assignees
Labels

Comments

@euresti
Copy link
Contributor

euresti commented May 1, 2017

Edited with a better example.

Let's suppose you have a function that can take an Optional and returns something based on that or None if it's None.

from typing import overload, Optional

def bar(foo: Optional[int]) -> Optional[str]:
   if foo is None:
       return None
   else:
       return str(foo)

x = None  # type: Optional[int]

reveal_type(bar(None))  # Line 18
reveal_type(bar(x))
reveal_type(bar('foo'))

There's one problem with this being annotated this way. If you pass a Non-Optional you get returned an Optional[str] even though it's not possible. So I thought the new @overload stuff might help here but it doesn't:

from typing import overload, Optional

@overload
def bar(foo: int) -> str:
    ...

@overload
def bar(foo: Optional[int]) -> Optional[str]:
    ...

def bar(foo: Optional[int]) -> Optional[str]:
   if foo is None:
       return None
   else:
       return str(foo)

x = None  # type: Optional[int]

reveal_type(bar(None))
reveal_type(bar(x))
reveal_type(bar('foo'))

Sadly that gives me an error and the wrong types:

tt.py:4: error: Overloaded function signatures 1 and 2 overlap with incompatible return types
tt.py:19: error: Revealed type is 'Union[builtins.int, builtins.None]'
tt.py:20: error: Revealed type is 'Any'
tt.py:21: error: Revealed type is 'Any'

I also tried None as the argument to the 2nd overload. That got me closer:

tt.py:19: error: Revealed type is 'Union[builtins.int, builtins.None]'
tt.py:20: error: Revealed type is 'Any'
tt.py:21: error: Revealed type is 'builtins.str'

I would love to be able to specify this as it would make a lot of our code easier to type. Maybe there's a way to do it with a TypeVar but I couldn't figure it out.

@gvanrossum
Copy link
Member

And for anyone thinking that maybe type variables provide the solution, the 'str' type in the input is unrelated to the 'str' type in the output -- the real-world code from which this was derived has bytes -> str.

Also this reminds me of #1436.

@refi64
Copy link
Contributor

refi64 commented May 1, 2017

IIRC you can do:

OptStr = TypeVar('OptStr', str, Optional[str])

# You might be able to instead do:
OptStr = TypeVar('OptStr', bound=Optional[str])
# but I'm not sure...

def bar(foo: OptStr) -> OptStr:
    # ...

That's actually how AnyStr is defined in the stubs.

@gvanrossum
Copy link
Member

Our messages crossed, there is no type correspondence between arg and return except when arg is None.

@euresti
Copy link
Contributor Author

euresti commented May 1, 2017

Oops. Looks like I simplified my example too much. I edited the original so that future readers don't have to read all the comments about how my example was wrong. Sorry!

@sixolet
Copy link
Collaborator

sixolet commented May 1, 2017

I don't see any reason not to relax the overloading rules here; it feels like they're overly restrictive in this case. Specifically, since str is a subtype of Optional[str] and the valid calls with a int argument are a proper subset of the valid calls with an Optional[int] argument, then we should allow this particular type of overlapping signature.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented May 1, 2017

I think this should be done like this:

@overload
def bar(x: None) -> None:
    ...
@overload
def bar(x: int) -> str:
    ...
def bar(x: Optional[int]) -> Optional[str]:
    if x is None:
        return None
    return str(x)

However, it does not work. I get this:

x: Optional[int]

reveal_type(bar(1)) # OK, builtins.str
reveal_type(bar(None)) # Any + a spurious error: "bar" does not return a value
reveal_type(bar(x)) # Any, no error reported on this line

It looks like we have two independent bugs here. The spurious error "bar" does not return a value seems to be specific for None, so let us consider arbitrary types:

class A: ...
class B: ...
class C: ...

@overload
def foo(x: A) -> A: ...
@overload
def foo(x: B) -> C: ...
def foo(x: Union[A, B]) -> Union[A, C]:
    if isinstance(x, A):
        return x
    return C()

Here A plays the role of NoneType. With this code I get the following:

z: Union[A, B]

reveal_type(foo(A())) # OK, __main__.A
reveal_type(foo(B())) # OK, __main__.C
reveal_type(foo(z)) # Any, no error reported

The fact that Any is silently inferred instead of Union[A, C] seems to me like a bug in @overload implementation.

@ilevkivskyi ilevkivskyi added bug mypy got something wrong priority-0-high labels May 1, 2017
@gvanrossum
Copy link
Member

Yes, you called it. Thanks @ilevkivskyi .

@sixolet
Copy link
Collaborator

sixolet commented May 1, 2017

I don't think we should infer the Union[A, C] return type for foo(z), and I think we should error in this case -- it seems like the implementation type should not factor in to the typechecking of an overload, otherwise you wouldn't be able to use the overloads to narrow specifically the types your function accepted. (You should remain able to make an implementation that takes (*args: Any, **kwargs: Any) and narrow the types you'll accept by defining an overload series).

Doing the type math to calculate that a type union of two arguments would result in a return type of a type union of two return types seems chancy and hard for the user to understand.

We should, on the other hand, allow you to define a third

@overload
def foo(x: Union[A, B]) -> Union[A, C]: ...

That would make everything typecheck the way we want it to.

@ilevkivskyi
Copy link
Member

@sixolet

it seems like the implementation type should not factor in to the typechecking of an overload

Inferred Union[A, C] in my example has nothing to do with the implementation type, I just added some type in implementation to typecheck the body.

Doing the type math to calculate that a type union of two arguments would result in a return type of a type union of two return types seems chancy and hard for the user to understand.

There is a general "policy" now to not infer union types, but this is moving now towards inferring unions in simple cases, this was discussed with @JukkaL recently in the context of Instance joins. TypeScript for example always infers unions in the latter case, and people don't complain much about this. On the contrary, TypeScript does not support union math for overloads and people seem to complain a lot about this.

We can consider the option of the third overload you proposed (at least we should not prohibit it, there are already few open issues about order of overloads and overlapping types). Then later we could add some simple union math. Anyway, silently inferring Any is not a good idea in any case.

@JukkaL JukkaL self-assigned this May 2, 2017
@JukkaL
Copy link
Collaborator

JukkaL commented May 2, 2017

The fact that Any is silently inferred instead of Union[A, C] seems to me like a bug in @overload implementation.

Yes, this is a known bug. It looks like it's basically the same as #3256.

JukkaL added a commit that referenced this issue May 2, 2017
Previously `None` was not considered as more precise than
`Optional[x]`, which was obviously incorrect. Fixed the
implementation of type precision checking to match the
description, how that proper subtype checking works.

Fixes the original example in #3295.
JukkaL added a commit that referenced this issue Sep 6, 2017
Previously `None` was not considered as more precise than
`Optional[x]`, which was obviously incorrect. Fixed the
implementation of type precision checking to match the
description, now that proper subtype checking is implemented.

Fixes the original example in #3295.
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Apr 2, 2018
This commit adds support for very basic and simple union math when
calling overloaded functions, resolving python#4576.

One thing led to another, and this ended up accidentally fixing or
touching on several different overload-related issues. In particular,
I believe this pull request:

1.  Fixes the bug (?) where calling overloaded functions can sometimes
    silently infer a return type of 'Any'

2.  Changes the semantics of how mypy handles overlapping functions,
    which I believe is currently under discussion in python/typing#253

Although this change is functional and mergable, I was planning on
polishing it more -- adding more tests, fleshing out the union math
behavior, etc.

However, I think these are sort of big changes and wanted to check in
and make sure this pull request is actually welcome/is a good idea.
If not, let me know, and I'd be happy to abandon it.

---

Details on specific changes made:

1.  The new algorithm works by modifying checkexpr.overload_call_targets
    to return all possible matches, rather then just one.

    We start by trying the first matching signature. If there was some
    error, we (conservatively) attempt to union all of the matching
    signatures together and repeat the typechecking process.

    If it doesn't seem like it's possible to combine the matching
    signatures in a sound way, we end and just output the errors we
    obtained from typechecking the first match.

    The "signature-unioning" code is currently deliberately very
    conservative. I figured it was better to start small and attempt to
    handle only basic cases like python#1943 and relax the restrictions later
    as needed. For more details on this algorithm, see the comments in
    checkexpr.union_overload_matches.

2.  This change incidentally resolves any bugs related to how calling
    an overloaded function can sometimes silently infer a return type
    of Any. Previously, if a function call caused an overload to be
    less precise then a previous one, we gave up and returned a silent
    Any.

    This change removes this case altogether and only infers Any if
    either (a) the caller arguments explicitly contains Any or (b) if
    there was some error.

    For example, see python#3295 and python#1322 -- I believe this pull request touches
    on and maybe resolves (??) those two issues.

3.  As a result, I needed to fix a few parts of mypy that were
    relying on this "silently infer Any" behavior -- see the changes in
    checker.py and semanal.py. Both files were using expressions of the
    form `zip(*iterable)`, which ended up having a type of `Any` under
    the old algorithm. The new algorithm will instead infer
    `Iterable[Tuple[Any, ...]]` which actually matches the stubs in
    typeshed.

4.  These changes cause the attr stubs in `test-data/unit/lib-stub` to
    no longer work. It seems that the stubs both here and in typeshed
    were both also falling prey to the 'silently infer Any' bug: code
    like `a = attr.ib()` typechecked not because they matched the
    signature of any of the overloads, but because that particular call
    caused one or more overloads to overlap, which made mypy give up and
    infer Any.

    I couldn't find a clean way of fixing the stubs to infer the correct
    thing under this new behavior, so just gave up and removed the
    overloads altogether. I think this is fine though -- it seems like
    the attrs plugin infers the correct type for us anyways, regardless
    of what the stubs say.

    If this pull request is accepted, I plan on submitting a similar
    pull request to the stubs in typeshed.

4.  This pull request also probably touches on
    python/typing#253. We still require the
    overloads to be written from the most narrow to general and disallow
    overlapping signatures.

    However, if a *call* now causes overlaps, we try the "union"
    algorithm described above and default to selecting the first
    matching overload instead of giving up.
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Apr 2, 2018
This commit adds support for very basic and simple union math when
calling overloaded functions, resolving python#4576.

One thing led to another, and this ended up accidentally fixing or
touching on several different overload-related issues. In particular,
I believe this pull request:

1.  Fixes the bug (?) where calling overloaded functions can sometimes
    silently infer a return type of 'Any'

2.  Changes the semantics of how mypy handles overlapping functions,
    which I believe is currently under discussion in python/typing#253

Although this change is functional and mergable, I was planning on
polishing it more -- adding more tests, fleshing out the union math
behavior, etc.

However, I think these are sort of big changes and wanted to check in
and make sure this pull request is actually welcome/is a good idea.
If not, let me know, and I'd be happy to abandon it.

---

Details on specific changes made:

1.  The new algorithm works by modifying checkexpr.overload_call_targets
    to return all possible matches, rather then just one.

    We start by trying the first matching signature. If there was some
    error, we (conservatively) attempt to union all of the matching
    signatures together and repeat the typechecking process.

    If it doesn't seem like it's possible to combine the matching
    signatures in a sound way, we end and just output the errors we
    obtained from typechecking the first match.

    The "signature-unioning" code is currently deliberately very
    conservative. I figured it was better to start small and attempt to
    handle only basic cases like python#1943 and relax the restrictions later
    as needed. For more details on this algorithm, see the comments in
    checkexpr.union_overload_matches.

2.  This change incidentally resolves any bugs related to how calling
    an overloaded function can sometimes silently infer a return type
    of Any. Previously, if a function call caused an overload to be
    less precise then a previous one, we gave up and returned a silent
    Any.

    This change removes this case altogether and only infers Any if
    either (a) the caller arguments explicitly contains Any or (b) if
    there was some error.

    For example, see python#3295 and python#1322 -- I believe this pull request touches
    on and maybe resolves (??) those two issues.

3.  As a result, I needed to fix a few parts of mypy that were
    relying on this "silently infer Any" behavior -- see the changes in
    checker.py and semanal.py. Both files were using expressions of the
    form `zip(*iterable)`, which ended up having a type of `Any` under
    the old algorithm. The new algorithm will instead infer
    `Iterable[Tuple[Any, ...]]` which actually matches the stubs in
    typeshed.

4.  These changes cause the attr stubs in `test-data/unit/lib-stub` to
    no longer work. It seems that the stubs both here and in typeshed
    were both also falling prey to the 'silently infer Any' bug: code
    like `a = attr.ib()` typechecked not because they matched the
    signature of any of the overloads, but because that particular call
    caused one or more overloads to overlap, which made mypy give up and
    infer Any.

    I couldn't find a clean way of fixing the stubs to infer the correct
    thing under this new behavior, so just gave up and removed the
    overloads altogether. I think this is fine though -- it seems like
    the attrs plugin infers the correct type for us anyways, regardless
    of what the stubs say.

    If this pull request is accepted, I plan on submitting a similar
    pull request to the stubs in typeshed.

4.  This pull request also probably touches on
    python/typing#253. We still require the
    overloads to be written from the most narrow to general and disallow
    overlapping signatures.

    However, if a *call* now causes overlaps, we try the "union"
    algorithm described above and default to selecting the first
    matching overload instead of giving up.
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Apr 2, 2018
This commit adds support for very basic and simple union math when
calling overloaded functions, resolving python#4576.

As a side effect, this change also fixes a bug where calling overloaded
functions can sometimes silently infer a return type of 'Any' and
slightly modifies the semantics of how mypy handles overlaps in
overloaded functions.

Details on specific changes made:

1.  The new algorithm works by modifying checkexpr.overload_call_targets
    to return all possible matches, rather then just one.

    We start by trying the first matching signature. If there was some
    error, we (conservatively) attempt to union all of the matching
    signatures together and repeat the typechecking process.

    If it doesn't seem like it's possible to combine the matching
    signatures in a sound way, we end and just output the errors we
    obtained from typechecking the first match.

    The "signature-unioning" code is currently deliberately very
    conservative. I figured it was better to start small and attempt to
    handle only basic cases like python#1943 and relax the restrictions later
    as needed. For more details on this algorithm, see the comments in
    checkexpr.union_overload_matches.

2.  This change incidentally resolves any bugs related to how calling
    an overloaded function can sometimes silently infer a return type
    of Any. Previously, if a function call caused an overload to be
    less precise then a previous one, we gave up and returned a silent
    Any.

    This change removes this case altogether and only infers Any if
    either (a) the caller arguments explicitly contains Any or (b) if
    there was some error.

    For example, see python#3295 and python#1322 -- I believe this pull request touches
    on and maybe resolves (??) those two issues.

3.  As a result, this caused a few errors in mypy where code was
    relying on this "silently infer Any" behavior -- see the changes in
    checker.py and semanal.py. Both files were using expressions of the
    form `zip(*iterable)`, which ended up having a type of `Any` under
    the old algorithm. The new algorithm will instead infer
    `Iterable[Tuple[Any, ...]]` which actually matches the stubs in
    typeshed.

4.  Many of the attrs tests were also relying on the same behavior.
    Specifically, these changes cause the attr stubs in
    `test-data/unit/lib-stub` to no longer work. It seemed that expressions
    of the form `a = attr.ib()` were evaluated to 'Any' not because of a
    stub, but because of the 'silent Any' bug.

    I couldn't find a clean way of fixing the stubs to infer the correct
    thing under this new behavior, so just gave up and removed the
    overloads altogether. I think this is fine though -- it seems like
    the attrs plugin infers the correct type for us anyways, regardless
    of what the stubs say.

    If this pull request is accepted, I plan on submitting a similar
    pull request to the stubs in typeshed.

4.  This pull request also probably touches on
    python/typing#253. We still require the
    overloads to be written from the most narrow to general and disallow
    overlapping signatures.

    However, if a *call* now causes overlaps, we try the "union"
    algorithm described above and default to selecting the first
    matching overload instead of giving up.
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Apr 11, 2018
This commit adds support for very basic and simple union math when
calling overloaded functions, resolving python#4576.

As a side effect, this change also fixes a bug where calling overloaded
functions can sometimes silently infer a return type of 'Any' and
slightly modifies the semantics of how mypy handles overlaps in
overloaded functions.

Details on specific changes made:

1.  The new algorithm works by modifying checkexpr.overload_call_targets
    to return all possible matches, rather then just one.

    We start by trying the first matching signature. If there was some
    error, we (conservatively) attempt to union all of the matching
    signatures together and repeat the typechecking process.

    If it doesn't seem like it's possible to combine the matching
    signatures in a sound way, we end and just output the errors we
    obtained from typechecking the first match.

    The "signature-unioning" code is currently deliberately very
    conservative. I figured it was better to start small and attempt to
    handle only basic cases like python#1943 and relax the restrictions later
    as needed. For more details on this algorithm, see the comments in
    checkexpr.union_overload_matches.

2.  This change incidentally resolves any bugs related to how calling
    an overloaded function can sometimes silently infer a return type
    of Any. Previously, if a function call caused an overload to be
    less precise then a previous one, we gave up and returned a silent
    Any.

    This change removes this case altogether and only infers Any if
    either (a) the caller arguments explicitly contains Any or (b) if
    there was some error.

    For example, see python#3295 and python#1322 -- I believe this pull request touches
    on and maybe resolves (??) those two issues.

3.  As a result, this caused a few errors in mypy where code was
    relying on this "silently infer Any" behavior -- see the changes in
    checker.py and semanal.py. Both files were using expressions of the
    form `zip(*iterable)`, which ended up having a type of `Any` under
    the old algorithm. The new algorithm will instead infer
    `Iterable[Tuple[Any, ...]]` which actually matches the stubs in
    typeshed.

4.  Many of the attrs tests were also relying on the same behavior.
    Specifically, these changes cause the attr stubs in
    `test-data/unit/lib-stub` to no longer work. It seemed that expressions
    of the form `a = attr.ib()` were evaluated to 'Any' not because of a
    stub, but because of the 'silent Any' bug.

    I couldn't find a clean way of fixing the stubs to infer the correct
    thing under this new behavior, so just gave up and removed the
    overloads altogether. I think this is fine though -- it seems like
    the attrs plugin infers the correct type for us anyways, regardless
    of what the stubs say.

    If this pull request is accepted, I plan on submitting a similar
    pull request to the stubs in typeshed.

4.  This pull request also probably touches on
    python/typing#253. We still require the
    overloads to be written from the most narrow to general and disallow
    overlapping signatures.

    However, if a *call* now causes overlaps, we try the "union"
    algorithm described above and default to selecting the first
    matching overload instead of giving up.
Michael0x2a added a commit to Michael0x2a/mypy that referenced this issue Apr 23, 2018
This commit adds support for very basic and simple union math when
calling overloaded functions, resolving python#4576.

As a side effect, this change also fixes a bug where calling overloaded
functions can sometimes silently infer a return type of 'Any' and
slightly modifies the semantics of how mypy handles overlaps in
overloaded functions.

Details on specific changes made:

1.  The new algorithm works by modifying checkexpr.overload_call_targets
    to return all possible matches, rather then just one.

    We start by trying the first matching signature. If there was some
    error, we (conservatively) attempt to union all of the matching
    signatures together and repeat the typechecking process.

    If it doesn't seem like it's possible to combine the matching
    signatures in a sound way, we end and just output the errors we
    obtained from typechecking the first match.

    The "signature-unioning" code is currently deliberately very
    conservative. I figured it was better to start small and attempt to
    handle only basic cases like python#1943 and relax the restrictions later
    as needed. For more details on this algorithm, see the comments in
    checkexpr.union_overload_matches.

2.  This change incidentally resolves any bugs related to how calling
    an overloaded function can sometimes silently infer a return type
    of Any. Previously, if a function call caused an overload to be
    less precise then a previous one, we gave up and returned a silent
    Any.

    This change removes this case altogether and only infers Any if
    either (a) the caller arguments explicitly contains Any or (b) if
    there was some error.

    For example, see python#3295 and python#1322 -- I believe this pull request touches
    on and maybe resolves (??) those two issues.

3.  As a result, this caused a few errors in mypy where code was
    relying on this "silently infer Any" behavior -- see the changes in
    checker.py and semanal.py. Both files were using expressions of the
    form `zip(*iterable)`, which ended up having a type of `Any` under
    the old algorithm. The new algorithm will instead infer
    `Iterable[Tuple[Any, ...]]` which actually matches the stubs in
    typeshed.

4.  Many of the attrs tests were also relying on the same behavior.
    Specifically, these changes cause the attr stubs in
    `test-data/unit/lib-stub` to no longer work. It seemed that expressions
    of the form `a = attr.ib()` were evaluated to 'Any' not because of a
    stub, but because of the 'silent Any' bug.

    I couldn't find a clean way of fixing the stubs to infer the correct
    thing under this new behavior, so just gave up and removed the
    overloads altogether. I think this is fine though -- it seems like
    the attrs plugin infers the correct type for us anyways, regardless
    of what the stubs say.

    If this pull request is accepted, I plan on submitting a similar
    pull request to the stubs in typeshed.

4.  This pull request also probably touches on
    python/typing#253. We still require the
    overloads to be written from the most narrow to general and disallow
    overlapping signatures.

    However, if a *call* now causes overlaps, we try the "union"
    algorithm described above and default to selecting the first
    matching overload instead of giving up.
@Michael0x2a
Copy link
Collaborator

Update: I think this is almost fixed? The two code samples @ilevkivskyi provided almost typecheck as desired on master, except that we still have the spurious "bar does not return a value" error message.

Some related discussion here.

@ilevkivskyi
Copy link
Member

I propose to close this issue. The "does not return a value" error is unrelated, we can continue the discussion in the issue you mention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants