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

Use supertype context for variable type inference #13494

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

ilevkivskyi
Copy link
Member

Fixes #8796
Fixes #4547

The fix is straightforward in some sense, but it has also high potential for fallout, because previously mypy missed some actual errors (plus this may change type inference in some valid cases, as type inference is in general very fragile).

Note I also removed type inference from semanal.py, it messed up some corner cases (because it effectively made variables annotated), also I think it is a wrong place for type inference, even in simple cases.

Let's see what mypy_primer will show us.

cc @JukkaL for when you are back from vacation.

@ilevkivskyi
Copy link
Member Author

Btw if there will be a big fallout, one possible option is to limit this to invariant builtin collections and literal types (it will be quite unprincipled, but also looking at the issues, it looks like only these cases usually cause troubles).

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@ilevkivskyi
Copy link
Member Author

Hm, the biggest fallout seem to be from removing the type inference for simple literals in semanal.py. This is because we don't defer top-levels in type-checker. TBH I don't know why, maybe we can actually start deferring them? Otherwise it is kind-of inconsistent.

@sobolevn
Copy link
Member

Related #10870

@ilevkivskyi
Copy link
Member Author

ilevkivskyi commented Aug 24, 2022

OK, so it looks like not deferring top-levels is kind of hacky way to compensate for lack of proper NameError checking. Also I tried to enable it and although it is not hard it is still non-trivial, and better be done in a separate PR (probably after we add a support for NameError detection). For now I will try to restore the special-casing of literals I deleted, but only at module scope. Let's see what this will give us.

@sobolevn Thanks, I will add test cases from that issue (and PR), if they pass.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/context.py:124: error: Need type annotation for "__var__"
- src/prefect/context.py:228: error: Need type annotation for "__var__"
- src/prefect/context.py:246: error: Need type annotation for "__var__"
- src/prefect/context.py:264: error: Need type annotation for "__var__"
- src/prefect/context.py:281: error: Need type annotation for "__var__"

scikit-learn (https://github.com/scikit-learn/scikit-learn)
+ sklearn/covariance/_graph_lasso.py:848: error: List item 0 has incompatible type "Type[Integral]"; expected "str"
+ sklearn/covariance/_graph_lasso.py:848: error: List item 1 has incompatible type "None"; expected "str"

ibis (https://github.com/ibis-project/ibis)
- ibis/common/tests/test_grounds.py:513: error: Need type annotation for "__instances__"
- ibis/common/tests/test_grounds.py:517: error: Need type annotation for "__instances__"

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/worker.py:34: error: Cannot determine type of "LOG_FORMAT"  [has-type]
+ aiohttp/worker.py:224: error: Returning Any from function declared to return "str"  [no-any-return]

class A:
x: List[str]

class B(A):
Copy link
Member

Choose a reason for hiding this comment

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

Do we care about cases like

class A:
    x: List[str]

class B:
    x: List[int]

class C(A, B):
     x = []

in the scope of this PR?
I think that it is already covered by other checks, but do we need to have extra features integration test / error message check?

I think it falls into # Multiple incompatible candidates, cannot use any of them as context. branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is a test below (we will give usual "need annotation" error and infer list[Any]).

@ilevkivskyi
Copy link
Member Author

OK, so both new mypy_primer errors are technically correct, although a bit unfortunate. For scikit-learn there is this:

class B:
    _attrs = {"foo": ["bar"]}  # Indented type is Dict[str, Any], but no annotation
class C(B):
    _attrs = {"baz": [1, 2]}  # New error here

This is however consistent with situation when both assignments appear in the same scope. The error in aiohttp is cuased by two classes in an import cycle, with one referencing attribute of another.

@ilevkivskyi ilevkivskyi requested a review from sobolevn August 24, 2022 12:09
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Another great feature! Thank you!

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