Fix "Implicit type aliases not recognised with PEP 604 + import cycles" (but only for stubs)

[AlexWaygood]

Description

I think this PR gets rid of all the errors in #11887 -- but I'm not sure #11887 should be closed, as this will only fix the issue for stub files. Cyclic imports are likely to be far more common in typeshed than elsewhere, though, so it may still be worthwhile. (Other users of mypy may also find it easier to make use of PEP 613, as well, whereas typeshed is currently unable to do so — see #4913.)

Essentially, the error in #11887 is caused by mypy being unable to tell whether a | expression represents a type alias for a union-type, or some other arbitrary runtime object. But the problem just doesn't exist for .pyi stub files, because there are no arbitrary runtime objects. An expression using the bitwise | operator in a stub file can only be a type alias.

Test Plan

Manual testing confirms that this gets rid of all the reported errors in #11887.

I have no idea how to create a regression test for this kind of thing -- any guidance on that would be hugely appreciated!

[AlexWaygood]

Cc. @hauntsaninja (lmk if this "solution" is hopelessly naive!)

[sobolevn]

I have no idea how to create a regression test for this kind of thing -- any guidance on that would be hugely appreciated!

I think that a proper place for this kind of test is pythoneval.test

Take a look at cases that have multiple [file ...] specs. Like this one: testNewAnalyzerTypedDictInStub_newsemanal
This way you can create an import cycle.

[AlexWaygood]

I have no idea how to create a regression test for this kind of thing -- any guidance on that would be hugely appreciated!

I think that a proper place for this kind of test is pythoneval.test

Take a look at cases that have multiple [file ...] specs. Like this one: testNewAnalyzerTypedDictInStub_newsemanal This way you can create an import cycle.

Great, I'll take a look — thanks! 😀

[AlexWaygood]

(Marking this as a draft for now, as I'm still struggling to create a minimal repro of the bug, that's suitable for use as a regression test. I'm working on it.)

[AlexWaygood]

@sobolevn, could you possibly give me a hand with this, if you have the time? I've finally managed to narrow this down to a minimal repro (see the code snippet I posted in #11887), but I'm struggling to figure out how to integrate the repro into mypy's test suite ☹

[AlexWaygood]

(Also, super-basic question — what's the command I need to use to run a single test case?)

[sobolevn]

could you possibly give me a hand with this, if you have the time?

Sure! I spend all my days on open-source, so I have all the time 😆

So, my thoughts are:

1. Don't use real module names. It only complicates things, because _typeshed is so special. I think names like a, b, c, etc are more useful here. At least they won't shadow other mypy fixtures.
2. It might look something like:

[case testTypeshedAliasInNestedModules]
import zero
import third

[file zero.pyi]
import first
from third import mystr
WriteableBuffer = first.mmap
ReadableBuffer = mystr | WriteableBuffer

[file first.pyi]
class mmap: ...

[file second.pyi]
# second.pyi: empty file

[file third.py]
import last
from zero import ReadableBuffer
class mystr: ...
Foo: ReadableBuffer

[file fourth.py]
# fourth.pyi: empty file

[file last.pyi]
import second
import fourth
[builtins fixtures/bool.pyi]
[out]

Unfortunatelly, this one passes (it is quite hard to understand this).

Or you can schedule a call if you wish to chat at any time: https://calendly.com/sobolevn/

(Also, super-basic question — what's the command do I need to use to run a single test case?)

I placed this test in check-type-alias.test and used: pytest mypy/test/testcheck.py::TypeCheckSuite::check-type-aliases.test::testTypeshedAliasInNestedModules

[AlexWaygood]

Woohooo, I managed to add a regression test! I simulated the way that all modules implicitly do from builtins import * at the top of every module 😀 to run the test in isolation, run pytest mypy/test/testpythoneval.py::PythonEvaluationSuite::pythoneval.test::testImplicit604TypeAliasWithCyclicImport

[hauntsaninja]

Awesome! Thanks for picking away at this, it's a hard first issue to solve :-)

Three nits:

1. I think we can reduce the test case a little bit, something like the following should work:

[case testImplicit604TypeAliasWithCyclicImport]
# flags: --python-version 3.10
from was_builtins import foo
reveal_type(foo)  # N: Revealed type is "Union[builtins.str, was_builtins.mmap]"

[file was_builtins.pyi]
class mmap: ...
from was_typeshed import ReadableBuffer
foo: ReadableBuffer

[file was_typeshed.pyi]
import was_builtins
WriteableBuffer = was_builtins.mmap
ReadableBuffer = str | WriteableBuffer

2. Can we move this to test-data/unit/check-union-or-syntax.test?
3. Can we add an "xfail" test for this with .py extensions? I believe you can just suffix the test case name with -xfail and it'll magically work.

I'm happy to merge this if it unblocks typeshed. That said, clearly we'll need another solution, so at the least it might be worth leaving a TODO comment with the issue link, so we can tidy things up when we have a comprehensive solution.
I'm also a little worried about false negatives, e.g. I'm not fully sure what mypy will do if you do various things like: X = 1 | 2; a: X in stubs. That said, I'm not overly concerned, since if you ignore the clearer intention, this is already somewhat of a problem with how we do PEP 613.

My favourite way of running a single mypy test is pytest -k 604TypeAlias -n0 (where the n0 means we don't do multiprocessing).

[AlexWaygood]

I'm also a little worried about false negatives

Yeah, I'm with you there. Would there be a way to parameterise the tests Jukka added in #10770 so that they run twice — once with .py extensions, and once with .pyi extensions? I think that might reduce the risk of this causing weird behaviour elsewhere (but I'd rather not just write out duplicate tests for files with .pyi extensions, if at all possible).

I'll have a go at reducing the test case further, but it's been pretty darn difficult to get it down to this size 😖.

I also want to check whether this fixes #11098 in stub files (and add a regression test, if so).

[AlexWaygood]

Okay, I have:

1. Reduced the size of the test case.
2. Moved the test to test-data/unit/check-union-or-syntax.test.
3. Added an x-fail test for .py files, and a TODO comment.
4. Tested to see whether it fixes (🐞) False positive "error: Type application has too many types (1 expected)" on tuple when annotated as a type or in a TypeAlias with new union syntax (PEP 604) #11098 for stub files, as well as Implicit type aliases not recognised with PEP 604 + import cycles #11887 (it doesn't, sadly -- it only fixes Implicit type aliases not recognised with PEP 604 + import cycles #11887).

[hauntsaninja]

Awesome, thank you!

[AlexWaygood]

Awesome, thank you!

Massive thanks to you and @sobolevn for helping me with this!