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

gh-104549: Set __module__ on TypeAliasType #104550

Merged
merged 10 commits into from
May 18, 2023

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented May 16, 2023

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I can't review the C code, but I much prefer this behaviour to the behaviour on main! Manual testing also didn't reveal any bugs.

Maybe it's also worth testing that TypeAliasType.__module__ == 'typing'? (I.e., accessing __module__ on the class itself rather than instances of the class.) That's the only comment I have!

@Eclips4
Copy link
Member

Eclips4 commented May 16, 2023

One nit about tests: may it be useful to define a type alias in another module, and check something like assertEqual(x.Alias.__module__, x) where x is a some module.
Otherwise, C code looks fine, but I have one question. Should typealias_module be able to return None, and should it be tested?

@JelleZijlstra
Copy link
Member Author

@AlexWaygood unfortunately this no longer works, I get

test test_type_aliases failed -- Traceback (most recent call last):
  File "/Users/jelle/py/cpython/Lib/test/test_type_aliases.py", line 211, in test_module
    self.assertEqual(TypeAliasType.__module__, "typing")
AssertionError: <attribute '__module__' of 'typing.TypeAliasType' objects> != 'typing'

This seems to be an inescapable consequence of how heap types work (

if (type->tp_flags & Py_TPFLAGS_HEAPTYPE) {
), so not sure how to get around it.

@Eclips4 good call about tests in another module, I'll add some.

As for the module being NULL, apparently that can happen in various exotic situations where there is no calling function or that function has no module. Not 100% sure how to reproduce that.

@AlexWaygood
Copy link
Member

This seems to be an inescapable consequence of how heap types work

That's a shame :( This inconsistency is a little unfortunate:

>>> from typing import TypeAliasType, TypeVar
>>> TypeAliasType.__module__
<attribute '__module__' of 'typing.TypeAliasType' objects>
>>> TypeVar.__module__
'typing'
>>> type T = int | str
>>> T.__module__
'__main__'
>>> TypeVar("T").__module__
'__main__'

@AlexWaygood
Copy link
Member

That's a shame :( This inconsistency is a little unfortunate:

(But if there's no way of resolving that, then this is still definitely an improvement on the status quo :)

@JelleZijlstra
Copy link
Member Author

Possible solutions:

  • Make TypeAliasType into a static type. I think there is no strong reason for it to be a heap type, unlike with Generic and TypeVar; I made it a heap type for consistency with the other types in the file.
  • Give it a TP_MANAGED_DICT and put the __module__ in the dict; this is how TypeVar works, but it feels wasteful, and I don't see a need to allow setting arbitrary attributes on type aliases.

@AlexWaygood
Copy link
Member

  • Give it a TP_MANAGED_DICT and put the __module__ in the dict; this is how TypeVar works, but it feels wasteful, and I don't see a need to allow setting arbitrary attributes on type aliases.

Yeah, I agree that that allowing arbitrary setting of attributes seems unnecessary

@JelleZijlstra
Copy link
Member Author

oops, messed up the merge

@JelleZijlstra JelleZijlstra marked this pull request as ready for review May 18, 2023 04:57
@carljm carljm added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 18, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @carljm for commit 7898e78 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label May 18, 2023
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Assuming the refleak buildbots are happy, this LGTM.

@JelleZijlstra JelleZijlstra merged commit b9dce3a into python:main May 18, 2023
33 checks passed
@JelleZijlstra JelleZijlstra deleted the typealiasmod branch May 18, 2023 22:56
carljm added a commit to carljm/cpython that referenced this pull request May 18, 2023
* main:
  pythongh-74690: Don't set special protocol attributes on non-protocol subclasses of protocols (python#104622)
  pythongh-104623: Update Windows installer to use SQLite 3.42.0 (python#104625)
  pythongh-104050: Add more type annotations to Argument Clinic (python#104628)
  pythongh-104629: Don't skip test_clinic if _testclinic is missing (python#104630)
  pythongh-104549: Set __module__ on TypeAliasType (python#104550)
  pythongh-104050: Improve some typing around `default`s and sentinel values (python#104626)
  pythongh-104146: Remove unused vars from Argument Clinic (python#104627)
  pythongh-104615: don't make unsafe swaps in apply_static_swaps (python#104620)
  pythonGH-104484: Add case_sensitive argument to `pathlib.PurePath.match()` (pythonGH-104565)
  pythonGH-96803: Document and test new unstable internal frame API functions (pythonGH-104211)
  pythonGH-104580: Don't cache eval breaker in interpreter (pythonGH-104581)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants