Handle type[...] same as typing.Type[...] in classmethod 1st arg

[erikkemperman]

Fixes #15296

When looking at the first argument of a classmethod, the semantic analyzer has special handling if it is typing.Type[...]. However, as of PEP 585, we can also write builtins.type[...]. This PR adds that case, if the signature is from a stub file, if the file imports annotations from __future__, or if the python version is 3.9 or later.

The relevant tests I could find use the typing.Type syntax, and I suppose that that should remain the case until 3.9 is the lowest supported python version. For now, I verified that this patch has the desired effect of passing both classes in the repro I gave in issue #15296, but please let me know if more is needed.

[erikkemperman]

PS The logic of when to allow this was carried over from fragments I found in typeanal.py:

mypy/mypy/typeanal.py

Lines 553 to 555 in b8f8d80

	elif fullname == "typing.Type" or (
	fullname == "builtins.type"
	and (self.always_allow_new_syntax or self.options.python_version >= (3, 9))

mypy/mypy/typeanal.py

Lines 214 to 216 in b8f8d80

	self.always_allow_new_syntax = self.api.is_stub_file or self.api.is_future_flag_set(
	"annotations"
	)

[JelleZijlstra]

That seems like a lot of duplication; maybe we should add a helper to return whether something is valid as Type[]/type[]?

Also, please add a new test.

[erikkemperman]

@JelleZijlstra Thanks for taking a look! I tried do deduplicate a little but I'm not sure if I like this better and / or if this is what you meant. Will look at adding a test once the actual code seems satisfactory.

[erikkemperman]

Yeah, on second thought, I am not happy with my initial attempt at deduplication, and I think I should revert it. I don't think that I should be adding any surface area to the semantic analyzer API just to abstract away a few common patterns (they're not even duplications, really).

Trying to find a better way to do it / place to put it, I keep running into things that seem to me to be incomplete, e.g. the map of type aliases and its inverse in nodes.py. I guess I still don't grok this codebase sufficiently...

I will try to find some time to look into this a bit more, probably starting with a bunch of tests based off the specification of the PEP itself. But for now I would propose going back to my initial commit, which fixes quite precisely the bug I encountered and nothing else. Does that make sense?

[erikkemperman]

I have added a bunch of tests, but perhaps I went I bit overboard. I made a new check-method-1st-arg-pep585-type.test file in which I added variants of all the unit tests which contain method(s) where the first argument is annotated with typing.Type[...], adding the python 3.9 flag and changing to builtins.type[...].

Just to be clear, not all of these new tests were failing before the change I am proposing to make here in semanal.py. If you would prefer to have just the ones that actually failed then that's no problem of course, just let me know and I will reduce the new unit tests to just those cases.

PS Apologies for the force-push, I realize that isn't always convenient for reviewers...

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[hauntsaninja]

The change itself looks great, thanks for fixing this!

I agree that the tests are a little overzealous and your suggestion of pruning down would be good to do.

[erikkemperman]

@hauntsaninja Thanks for reviewing! Yes, indeed, I went a bit overboard with tests -- I just wanted to be quite sure I wasn't breaking anything. I've removed all but the two tests that were actually failing without this proposed patch to semanal.py.

[erikkemperman]

PS I just did the same overzealous-test-reduction for another PR that I opened a while back -- here... In case you were actively searching for more stuff to review :-)