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

Add unbound typevar check #13166

Merged
merged 23 commits into from
Jul 19, 2022

Conversation

AurelienJaquier
Copy link
Contributor

Description

This should fix Issue #13061 .

Mypy should now raise an error when it encounters unbound plain TypeVar.
It should not get triggered when the return type contains a TypeVar (e.g. List[T]).
It should not get triggered when a same TypeVar is present in the scope (e.g. inner function returns T, and T is an argument of outer function).

Test Plan

5 tests were added:

  • a plain unbound typevar triggering the new error
  • a typevar inner function not triggering the error
  • a function returning an iterable of typevars, not triggering the error
  • a plain bound typevar, not triggering the error
  • nested bound typevars, not triggering the error

We also changed the other functions triggering our error for the tests to pass.

This is our 1st contribution to Mypy. Please, guide us if there is anything we can improve in this PR.

This PR was made with @anilbey

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! (assuming the tests pass)

@anilbey
Copy link
Contributor

anilbey commented Jul 17, 2022

Good news! 🎉

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Could you add a test for upper bounded and constrained type vars?

I would also avoid opening PRs to projects that add type: ignore, since they likely won't be able to merge the PR until there's a new version of mypy. Besides, fixes are much preferred to adding type-ignores. If we just want to type-ignore most instances of this error, it'd be a sign that we shouldn't add it ;-)

mypy/checker.py Outdated
for argtype in typ.arg_types:
argtype.accept(arg_type_visitor)

if typ.ret_type not in arg_type_visitor.arg_types and typ.ret_type in typ.variables:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we micro optimise this by first checking if typ.ret_type in typ.variables and only then doing the collection of arg types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

addressed in the latest push

def f(a: Union[int, T], b: str) -> T:
...

def g(a: Callable[..., T], b: str) -> T:
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add one more test case with something more nested, say List[Union[Callable[..., Tuple[T]]]]

Copy link
Contributor

Choose a reason for hiding this comment

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

Done with the last push.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jul 17, 2022

If the typevar is upper bounded or constrained, we could actually suggest using the upper bound or the union of the constraints as the return type. This seems very common, e.g. see the pandas, spark, etc cases.

@ilevkivskyi
Copy link
Member

Btw, this reminds me some people use returning a plain unbound type var with values as a way to emulate unsafe unions. But as many of you know I don't like unsafe unions, so I am fine with breaking such hacks.

@ilevkivskyi
Copy link
Member

@anilbey @AurelienJaquier it would be great if you could implement some of the suggestions, but if you don't have time or want to do it in a separate PR it is also OK, please let me know.

@anilbey
Copy link
Contributor

anilbey commented Jul 18, 2022

Thanks @hauntsaninja for the feedback.
@ilevkivskyi I just added the deeper nested tests and micro optimisation. For upper bounded and constrained types, I will need a bit of more time.

@ilevkivskyi
Copy link
Member

OK, you can make those other changes in a separate PR. I will merge this when the tests pass.

@ilevkivskyi
Copy link
Member

@anilbey Unfortunately one of the tests you added is failing, probably you need to add a builtins fixture that includes tuple (you can see how it is done in other tests).

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

pegen (https://github.com/we-like-parsers/pegen)
+ src/pegen/parser.py:25: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]
+ src/pegen/parser.py:45: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

pandera (https://github.com/pandera-dev/pandera)
+ pandera/engines/engine.py:184: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

paasta (https://github.com/yelp/paasta)
+ paasta_tools/utils.py:3941: error: A function returning TypeVar should receive at least one argument containing the same Typevar
+ paasta_tools/utils.py:3948: error: A function returning TypeVar should receive at least one argument containing the same Typevar

anyio (https://github.com/agronholm/anyio)
+ src/anyio/from_thread.py:122: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

vision (https://github.com/pytorch/vision)
+ torchvision/prototype/datasets/utils/_internal.py:89: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

spark (https://github.com/apache/spark)
+ python/pyspark/ml/wrapper.py:267: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

tornado (https://github.com/tornadoweb/tornado)
+ tornado/queues.py:384: error: A function returning TypeVar should receive at least one argument containing the same Typevar
+ tornado/queues.py:421: error: A function returning TypeVar should receive at least one argument containing the same Typevar

schemathesis (https://github.com/schemathesis/schemathesis)
+ src/schemathesis/auth.py: note: In member "get" of class "AuthProvider":
+ src/schemathesis/auth.py:36: error: A function returning TypeVar should receive at least one argument containing the same Typevar

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/ext/commands/commands.py:1046: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]
+ steam/ext/commands/commands.py:1077: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

core (https://github.com/home-assistant/core)
+ homeassistant/helpers/singleton.py:29: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

Tanjun (https://github.com/FasterSpeeding/Tanjun)
+ tanjun/dependencies/data.py:321: error: A function returning TypeVar should receive at least one argument containing the same Typevar  [type-var]

@anilbey
Copy link
Contributor

anilbey commented Jul 18, 2022

OK, you can make those other changes in a separate PR. I will merge this when the tests pass.

Alright, sounds good

@ilevkivskyi ilevkivskyi merged commit 2c152e6 into python:master Jul 19, 2022
@AurelienJaquier AurelienJaquier deleted the add-unbound-typevar-check branch July 20, 2022 06:52
intgr added a commit to intgr/mypy that referenced this pull request Sep 19, 2022
Changed `Typevar` -> `TypeVar` in the error message from python#13166.

I was testing the mypy 0.980 pre-release builds and noticed that the error message was using inconsistent capitalization.
hauntsaninja pushed a commit that referenced this pull request Sep 19, 2022
Changed `Typevar` -> `TypeVar` in the error message from #13166.

I was testing the mypy 0.980 pre-release builds and noticed that the
error message was using inconsistent capitalization.
hauntsaninja added a commit to hauntsaninja/mypy that referenced this pull request Sep 25, 2022
Also don't complain about other TypeVarLikeTypes

Implements python#13166 (comment)
hauntsaninja added a commit that referenced this pull request Sep 26, 2022
Also don't complain about other TypeVarLikeTypes

Implements
#13166 (comment)
ilevkivskyi pushed a commit that referenced this pull request Oct 2, 2022
Also don't complain about other TypeVarLikeTypes

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

Successfully merging this pull request may close these issues.

4 participants