Skip to content

Optional generic not accepted with --strict-optional #2678

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

Closed
FuegoFro opened this issue Jan 11, 2017 · 2 comments · Fixed by #5699
Closed

Optional generic not accepted with --strict-optional #2678

FuegoFro opened this issue Jan 11, 2017 · 2 comments · Fixed by #5699
Assignees
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-type-variables topic-union-types

Comments

@FuegoFro
Copy link
Contributor

When running with --strict-optional, mypy complains about the last statement here. However, this seems like it should work, both because it does work when --strict-optional is left out, and because the revealed type looks like it should match.

from typing import Optional, Type, TypeVar


class BaseThing: pass
class ChildThing(BaseThing): pass


ThingClass = TypeVar("ThingClass", bound=BaseThing)


def get_thing(thing_class: Type[ThingClass]) -> Optional[ThingClass]:
    """ Get the thing with the given class. Stub implementation. """
    return None

thing = None  # type: Optional[ChildThing]

# Fails, complaining about `Type argument 1 of "get_thing" has incompatible value "Optional[ChildThing]"`
thing = get_thing(ChildThing)

# Revealed type is `Union[scratch_1.ChildThing*, builtins.None]`, which seems like it should 
# match Optional[ChildThing].
reveal_type(get_thing(ChildThing))
@gvanrossum
Copy link
Member

Hm, I wonder if the problem is due to a bad interaction between the ThingClass TypeVar and the Optional from the context? I think it ought to infer that if the context is Optional[ChildThing] and the return type (to be matched to that context) is Optional[ThingClass], then it must solve this to ThingClass==ChildThing, but it is then not correctly concluding that the argument type must be Type[ChildThing].

@ddfisher
Copy link
Collaborator

This problem isn't Optional-specific -- it'll happen with any Union. For example:

from typing import *

class A: pass
class B(A): pass


T = TypeVar("T", bound=A)

def f(x: Type[T]) -> Union[T, str]:
    return "foo"

x = None  # type: Union[B, str]

reveal_type(x)
reveal_type(f(B))
x = f(B)
test.py:14: error: Revealed type is 'Union[test.B, builtins.str]'
test.py:15: error: Revealed type is 'Union[test.B*, builtins.str]'
test.py:16: error: Type argument 1 of "f" has incompatible value "Union[B, str]"

We've known for a while that TypeVars and Unions don't play well together. There's been a bit of spot-patching so far, but I think we're at the point where a larger careful rethinking makes sense.

@JukkaL JukkaL added bug mypy got something wrong priority-1-normal labels Jan 16, 2017
@JukkaL JukkaL added false-positive mypy gave an error on correct code topic-union-types labels May 19, 2018
ilevkivskyi added a commit that referenced this issue Oct 2, 2018
Fixes #4872 
Fixes #3876
Fixes #2678 
Fixes #5199 
Fixes #5493 
(It also fixes a bunch of similar issues previously closed as duplicates, except one, see below).

This PR fixes a problems when mypy commits to soon to using outer context for type inference. This is done by:
* Postponing inference to inner (argument) context in situations where type inferred from outer (return) context doesn't satisfy bounds or constraints.
* Adding a special case for situation where optional return is inferred against optional context. In such situation, unwrapping the optional is a better idea in 99% of cases. (Note: this doesn't affect type safety, only gives empirically more reasonable inferred types.)

In general, instead of adding a special case, it would be better to use inner and outer context at the same time, but this a big change (see comment in code), and using the simple special case fixes majority of issues. Among reported issues, only #5311 will stay unfixed.
TV4Fun pushed a commit to TV4Fun/mypy that referenced this issue Oct 4, 2018
Fixes python#4872 
Fixes python#3876
Fixes python#2678 
Fixes python#5199 
Fixes python#5493 
(It also fixes a bunch of similar issues previously closed as duplicates, except one, see below).

This PR fixes a problems when mypy commits to soon to using outer context for type inference. This is done by:
* Postponing inference to inner (argument) context in situations where type inferred from outer (return) context doesn't satisfy bounds or constraints.
* Adding a special case for situation where optional return is inferred against optional context. In such situation, unwrapping the optional is a better idea in 99% of cases. (Note: this doesn't affect type safety, only gives empirically more reasonable inferred types.)

In general, instead of adding a special case, it would be better to use inner and outer context at the same time, but this a big change (see comment in code), and using the simple special case fixes majority of issues. Among reported issues, only python#5311 will stay unfixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-type-variables topic-union-types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants