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

Allow better tuple type aliases, fix "Type application" error #12134

Closed
wants to merge 3 commits into from
Closed

Allow better tuple type aliases, fix "Type application" error #12134

wants to merge 3 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Feb 6, 2022

I am also testing how new mypy_primer and mypy_comment actions work: #12125

I expect "No change" comment to appear.

Closes #11098

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.

Cool! Could you also add a test for type alias my_tuple = tuple; my_tuple[int, float] and for use of cast?

Also IIRC python eval actually runs code with the python interpreter. Would it make more sense for this to be in check-python310.test?

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

How is that related? 🤔

 _______________________ testTupleWithDifferentArgsPy310 ________________________
[gw0] linux -- Python 3.10.2 /home/runner/work/mypy/mypy/.tox/py/bin/python
data: /home/runner/work/mypy/mypy/test-data/unit/pythoneval.test:1590:
/home/runner/work/mypy/mypy/mypy/test/testpythoneval.py:44: in run_case
    test_python_evaluation(testcase, os.path.join(self.cache_dir.name, '.mypy_cache'))
/home/runner/work/mypy/mypy/mypy/test/testpythoneval.py:108: in test_python_evaluation
    assert_string_arrays_equal(adapt_output(testcase), output,
E   AssertionError: Invalid output (/home/runner/work/mypy/mypy/test-data/unit/pythoneval.test, line 1590)
----------------------------- Captured stderr call -----------------------------
Expected:
  _testTupleWithDifferentArgsPy310.py:12: error: Invalid type: try using Literal[1] instead? (diff)
  _testTupleWithDifferentArgsPy310.py:13: error: Unexpected "..." (diff)
Actual:
  message.pyi:13: error: Cannot determine type of "Any" (diff)
  _testTupleWithDifferentArgsPy310.py:12: error: Invalid type: try using Literal[1] instead? (diff)
  _testTupleWithDifferentArgsPy310.py:13: error: Unexpected "..." (diff)

@AlexWaygood
Copy link
Member

message.pyi:13: error: Cannot determine type of "Any" (diff)

Uh, that looks like python/typeshed#7019... Maybe syncing typeshed again might help??

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

I think that we should fix it instead of just hiding. I am looking into it 🙂

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

It happens here:

mypy/mypy/checkexpr.py

Lines 274 to 275 in 48dc990

if not var.is_ready and self.chk.in_checked_function():
self.chk.handle_cannot_determine_type(var.name, context)

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

I reduced email/message.pyi to this:

from typing import Any

_HeaderType = Any

class Message:
    def __getitem__(self, name: str) -> _HeaderType: ...

And it still happens.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

It took me literally two hours of debugging. I was able to reproduce this problem only in my newly written test. That's it. Nothing else worked.

Somehow I figured it out (at least I hope so).

@AlexWaygood
Copy link
Member

It took me literally two hours of debugging. I was able to reproduce this problem only in my newly written test. That's it. Nothing else worked.

Somehow I figured it out (at least I hope so).

Fantastic work - well done! But, bizarre that mypy suddenly started reporting this error recently, after it was fine with this line for so long 🤯

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

Thanks, but I still have to fix tuple error, it is also quite cryptic 😒

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 6, 2022

mypy_primer seemed to find a similar issue: https://github.com/aio-libs/aiohttp/blob/f78c128f52f4f7e55ebaa8ed0aa764031e98dfe8/aiohttp/web.py#L283

@sobolevn
Copy link
Member Author

sobolevn commented Feb 7, 2022

Ok, here's what is going on. We have a correct test failure:

________________________ testTypeAliasWithBuiltinTuple _________________________
[gw1] linux -- Python 3.8.12 /home/runner/work/mypy/mypy/.tox/py/bin/python
data: /home/runner/work/mypy/mypy/test-data/unit/check-generic-alias.test:243:
/home/runner/work/mypy/mypy/mypy/test/testcheck.py:140: in run_case
    self.run_case_once(testcase)
/home/runner/work/mypy/mypy/mypy/test/testcheck.py:227: in run_case_once
    assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line))
E   AssertionError: Unexpected type checker output (/home/runner/work/mypy/mypy/test-data/unit/check-generic-alias.test, line 243)
----------------------------- Captured stderr call -----------------------------
Expected:
  main:6: error: Incompatible types in assignment (expression has type "Tuple...
  main:10: error: Incompatible types in assignment (expression has type "Tupl...
  main:12: note: Revealed type is "builtins.tuple[builtins.int*, ...]" (diff)
Actual:
  main:6: error: Incompatible types in assignment (expression has type "Tuple...
  main:10: error: Incompatible types in assignment (expression has type "Tupl...
  main:12: note: Revealed type is "builtins.tuple[<nothing>, ...]" (diff)

Alignment of first line difference:
  E: ...led type is "builtins.tuple[builtins.int*, ...]"
  A: ...led type is "builtins.tuple[<nothing>, ...]"

Why? Because tuple[int, int]() call gets the original tuple.__new__ signature def [Tco] (iterable: typing.Iterable[Tco] =) -> builtins.tuple[Tco, ...] and tries to replace type vars (Tco) with actual type var values (int, int). apply_generic_arguments does that. It also asserts that the number of original args matches the number of vars to replace.

Right now I skip this step, that's why the test fails.

But, tuple is special, because it is variadic. We need to change this logic with a proper special case.
The problem is: I don't know how to do that.

Should it be def [Tco] (iterable: typing.Iterable[Union[tuple_args]] =) -> builtins.tuple[*tuple_args]? Or something else?

What pyright does in this case? @erictraut I would appreciate your help.

@AlexWaygood
Copy link
Member

What pyright does in this case? @erictraut I would appreciate your help.

@erictraut any thoughts on this? It looks like pyright handles this kind of thing much better than mypy at the moment — would be great to get your advice, if you've got the time :)

@erictraut
Copy link

erictraut commented Feb 13, 2022

Sorry @AlexWaygood, I missed your earlier question.

I'm not clear on which case you're referring to in your question, but I'll attempt to answer and you can tell me if I misinterpreted your question.

For non-tuple generic classes, pyright maintains a fixed-size list of type arguments once the class is specialized. the length of this list is determined by the fixed number of type parameters in the generic class.

For tuples, pyright maintains a separate (variable-sized) list of type arguments. This is in addition to the fixed-sized list, which corresponds to the type argument for the _T_co type parameter in the tuple class. For example, if it sees tuple[str, int, T], the variable-sized type argument list contains the types [str, int, T] and the fixed-sized list (which has one entry corresponding to _T_co) contains the union of these types [str | int | T].

For the variable-sized tuple type argument list, pyright also records (for each entry) whether the type has a corresponding ... (i.e. whether it represents zero or more of that type). Prior to PEP 646, an entry with a ... could appear only by itself within a tuple because tuple[int, str, ..., None] was not legal. But now with PEP 646, it's legal to spell tuple[int *tuple[str, ...], None] or tuple[int, *Ts, None] where Ts is a TypeVarTuple.

I hope that helps.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 14, 2022

@sobolevn Would you be able to fix the failing tests and the merge conflict so that we can include this in the 0.950 release (#12579)? I can also have a look at finishing this up.

1 similar comment
@JukkaL
Copy link
Collaborator

JukkaL commented Apr 14, 2022

@sobolevn Would you be able to fix the failing tests and the merge conflict so that we can include this in the 0.950 release (#12579)? I can also have a look at finishing this up.

@sobolevn
Copy link
Member Author

@JukkaL sorry, I am not very active right now. I have time to read the inbox, but I don't have much time to actually write code 😢

I hope I will be able to solve all the things in 1-2 months.

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2022

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

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/web.py:283: error: Unused "type: ignore[assignment]" comment

@hauntsaninja hauntsaninja reopened this Jul 28, 2022
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 4, 2022

@sobolevn I'd love to have this fix included in 1.0. If you are busy, I can look into finishing this.

@sobolevn
Copy link
Member Author

sobolevn commented Nov 4, 2022

@JukkaL I would totally appreciate some help on this issue! 👍

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 25, 2022

I created #14184 as an updated version of this PR.

@JukkaL JukkaL closed this Nov 25, 2022
JukkaL added a commit that referenced this pull request Nov 25, 2022
Fix type aliases like these:
```
T = tuple[int, str]
```
Type applications involving fixed-length tuples still don't fully work.
The inferred type is a variable-length tuple when constructing a tuple
using a type application, e.g. `tuple[int, str]((1, ""))`. This seems a
pretty low-priority issue, whereas the type alias use case seems common.

Most of the work was by @sobolevn originally in #12134. I just finished
it up.

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