Skip to content

Conversation

dr-carlos
Copy link

@dr-carlos dr-carlos commented Sep 3, 2025

Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks good

Co-authored-by: sobolevn <[email protected]>
@python-cla-bot
Copy link

python-cla-bot bot commented Sep 3, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

…fined generic tests

Removed global overriding builtin test as it seemingly wasn't compatible with `get_annotations()`.
@dr-carlos
Copy link
Author

Note that this PR also inherently fixes builtins overriding globals in this particular code path. I was previously unable to come up with a test for this without using the undocumented ForwardRef() constructor. I have since done some thinking, and maybe I am missing something obvious, but all I could come up with was an (extremely contrived) test:

from annotationlib import Format, get_annotations

class C:
	y: alias[memoryview, undef]

mv = __builtins__["memoryview"]
del __builtins__["memoryview"]
fwdref = get_annotations(C, format=Format.FORWARDREF)["y"]

__builtins__["memoryview"] = mv
generic = fwdref.evaluate(
	format=Format.FORWARDREF,
	globals={"memoryview": int},
	locals={"alias": dict}
)
self.assertNotIsInstance(generic, ForwardRef)
self.assertIs(generic.__origin__, dict)
self.assertEqual(len(generic.__args__), 2)
self.assertIs(generic.__args__[0], int)
self.assertIsInstance(generic.__args__[1], ForwardRef)

It feels very unlikely this would happen in real code, and I'm not sure if it's a good idea to test given that "Since this is an implementation detail, it may not be used by alternate implementations of Python." (https://docs.python.org/3/library/builtins.html), and given that __builtins__ seems to be either a module object or a dictionary depending on where you run the code.

@JelleZijlstra
Copy link
Member

Good point, I think it's fine to skip the test in that case.

Technical issue: your two PRs #138430 and #138075 will conflict with each other, and I won't be able to backport them to 3.14 until after 3.14.0 final comes out. So I'm planning to leave them open for now and land them once 3.14.0 final is out so I can sort out the merge conflicts and merge the backports without having to disrupt the RC process.

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.

4 participants