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

Mypy should be careful when using outer context for inference #4872

Closed
ilevkivskyi opened this issue Apr 9, 2018 · 6 comments · Fixed by #5699
Closed

Mypy should be careful when using outer context for inference #4872

ilevkivskyi opened this issue Apr 9, 2018 · 6 comments · Fixed by #5699
Assignees
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code needs discussion priority-0-high

Comments

@ilevkivskyi
Copy link
Member

There are several scenarios, when mypy inference leads to false positives. Namely, if the outer type context (return context) is too wide, mypy uses it and fails immediately. Instead it should try the inner context (argument context) before giving up. For example:

T = TypeVar('T', bound=Dict[str, Any])

def f(arg: T) -> T:
    ...
def outer(x: Mapping[str, Any]) -> None:
    ...
d: Dict[str, Any]

m: Mapping[str, Any] = f(d)  # Value of type variable "T" of "f" cannot be "Mapping[str, Any]"
outer(f(d))  # Value of type variable "T" of "f" cannot be "Mapping[str, Any]"

There are similar scenarios with union types, for example:

S = TypeVar('S', bound=int)

def g(arg: S) -> Optional[S]:
    ...

x: Optional[int]
x = g(42)  # Value of type variable "S" of "g" cannot be "Optional[int]"

etc. What is interesting, the failure doesn't happen if there are only simple types present in the bound and in the context, to reproduce one needs either generics, union types, or other "complex" types.

@ilevkivskyi
Copy link
Member Author

I set priority to high because I encountered this twice recently. If there is no simple solution we can downgrade to normal.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 9, 2018

A fix might be easy enough -- if the type inferred from the outer context is not compatible with type variable bounds, we leave the value of the type variable indeterminate (similar to what happens if there's no outer context) and continue type checking.

Another idea would be to remove union items not compatible with the bound from the inferred type variable value, and to map generic types not compatible with the band down to the bound, if possible. This idea feels a bit ad hoc though.

I don't remember ever encountering this issue though.

@ilevkivskyi
Copy link
Member Author

#5049 Shows the same problem with unbound type variables which may be harder to fix, because in that case Optional[int] is a valid inferred type (because Optional[Optional[int]] is Optional[int]), just not the most precise.

@jcrada
Copy link

jcrada commented Jun 26, 2018

Hi,

I believe I have the same problem. Please find below another example to replicate.

import unittest
from typing import Optional, Type, TypeVar


class A:
    def __init__(self) -> None:
        print("A: Constructing...")


class B:
    def __init__(self) -> None:
        print("B: Constructing...")

    def do_something(self) -> None:
        print("B: Doing something")


class Experimental(object):
    T = TypeVar('T', 'A', 'B')

    def create_component_a(self) -> Optional['A']:
        return self.create_component(A)
        # error: Value of type variable "T" of "create_component" of
        # "Experimental" cannot be "Optional[A]

    def create_component(self, cls: Type[T]) -> Optional[T]:
        result = cls()
        if hasattr(result, "do_something"):
            getattr(result, "do_something")()
        return result


class TestFllImporter(unittest.TestCase):
    def test_experimental(self) -> None:
        optional_a: Optional[A] = None
        reveal_type(optional_a)
        # error: Revealed type is 'Union[tests.test_experimental.A, None]'

        a = A()
        reveal_type(a)
        # error: Revealed type is 'tests.test_experimental.A'

        constructed_a = Experimental().create_component_a()
        reveal_type(constructed_a)
        # error: Revealed type is 'Union[tests.test_experimental.A, None]'

        component_a = Experimental().create_component(A)
        reveal_type(component_a)
        # error: Revealed type is 'Union[tests.test_experimental.A*, None]'


if __name__ == '__main__':
    unittest.main()

Any thoughts?

Btw, what does the * in # error: Revealed type is 'Union[tests.test_experimental.A*, None]' means?

Thanks!

@ilevkivskyi
Copy link
Member Author

Star means "inferred" (in contrast to explicitly declared).

@jhbuhrman
Copy link

jhbuhrman commented Sep 4, 2018

On request of @ilevkivskyi, after discussed this on python/typing - Gitter, another example:

#  pylint: disable=missing-docstring
import re
from typing import Text, Iterable

def get_words(text: Text) -> Iterable[Text]:
    return (word.lower() for word in filter(None, re.split(r'\W+', text)))

assert list(get_words("Ham, Spam, Eggs!")) == ['ham', 'spam', 'eggs']

This results in:

../../Library/Preferences/PyCharm2018.2/scratches/try_filter_on_none.py:6: error: Value of type variable "AnyStr" of "split" cannot be "Optional[str]"

The workarounds discovered so far are:

  • add a cast around the split function, as in cast(List[str], re.split(r'\W+', text))
  • change the function parameter of filter from None into bool
  • insert a typeshed-like statement (for the sake of testing):
    def filter(function: Optional[Callable[[_T], Any]], iterable: Iterable[_T]) -> Iterator[_T]: ...
    

(mypy 0.620, Python 3.7.0)

@ilevkivskyi ilevkivskyi self-assigned this Sep 18, 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 needs discussion priority-0-high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants