bpo-46333: Honor `module` parameter in ForwardRef #30536

aha79 · 2022-01-11T15:28:19Z

The module parameter carries semantic information about the forward ref.
Forward refs are different if they refer to different module even if they
have same name. This affects eq, repr and hash and methods,
which were previously not honoring the module parameter.

https://bugs.python.org/issue46333

The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have same name. This affects __eq__, __repr__ and __hash__ and methods.

AlexWaygood

Welcome to CPython! You can add a NEWS entry to your PR using https://blurb-it.herokuapp.com/ :)

Lib/test/test_typing.py

Lib/typing.py

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood · 2022-01-11T16:00:17Z

Since this is your first contribution, you should also feel free to add your name to https://github.com/python/cpython/blob/main/Misc/ACKS (this is entirely optional, only if you'd like to).

gvanrossum · 2022-01-11T18:19:54Z

@Fidget-Spinner I hope you can review and merge this PR?

Fidget-Spinner · 2022-01-12T13:13:28Z

@Fidget-Spinner I hope you can review and merge this PR?

@gvanrossum Sure. Leave it to me.

Fidget-Spinner

Great work! We're almost ready to merge, I just have some minor concerns.

Lib/typing.py

Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst

Fidget-Spinner · 2022-01-12T13:24:44Z

Lib/test/test_typing.py

@@ -2611,6 +2611,10 @@ def test_forward_equality(self):
        fr = typing.ForwardRef('int')
        self.assertEqual(fr, typing.ForwardRef('int'))
        self.assertNotEqual(List['int'], List[int])
+        self.assertNotEqual(fr, typing.ForwardRef('int', module=__name__))


Hmm, I wonder if this test is run directly, will __name__ be `main``? Wouldn't that make the test pass incorrectly? (Throwing this question out there because I'm not too sure myself). Maybe we should be safe and give it a distinct name?

The test is to ensure that ForwardRef with module set is different from ForwardRef with module not set (That was previsously not the case). The actual value of module (or __name__) is not relevant. Under the hood the comparison is None vs str

My bad, thanks for pointing that out. I mistakenly assumed it would grab a default value from the current scope to populate module in things like ClassVar['int'] due to the _type_check/_type_convert function (I'm now shocked that it doesn't and only does that for TypedDict).

Yes, unfortunately the current module is not captured automatically.
The current scope/module is typicallly obtained by get_type_hints because a class knows its module (and so all ForwardRefs in member annotations can be correctly resolved. But each forward Ref does originally not know its module).
However for type aliases this does not work, and there one needs the explicit module.

For example

Json = Union[ List['Json'], Dict[str,'Json'], int, float, bool, None ]

cannot be exported (and expect to work) unless one sets the module for each forward ref.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

AlexWaygood · 2022-01-12T14:07:39Z

@Fidget-Spinner, do you see this one as more of a bugfix that should be backported, or more of an enhancement/behaviour change? (This is just for my clarification, @aha79 — I'm fairly new to the triage team 🙂)

Fidget-Spinner · 2022-01-12T14:26:53Z

@Fidget-Spinner, do you see this one as more of a bugfix that should be backported, or more of an enhancement/behaviour change? (This is just for my clarification, @aha79 — I'm fairly new to the triage team 🙂)

@AlexWaygood this is a grey area.

On one hand, generally, anything dealing with __eq__ and __hash__ in typing.py == bug. typing.py relies heavily on an LRU cache when subscripting things (see _tp_cache), so in the past, any hash/equality comparisons would mean incorrect values when using almost anything in typing.py, as Andreas rightly pointed out.

However, this specific module parameter is used only for differentiating different-module TypedDicts. TypedDict doesn't use the type cache, and different module TypedDict will automatically have different hash() values. So I don't know of anyone who's affected unless you use ForwardRef directly. But that's explicitly discouraged as an internal detail as I mentioned on the bug tracker.

I'm inclined to accept the PR without the __repr__ change, and I will likely backport it like that since it fixes something (albeit in a use case not really supported by us).

AlexWaygood · 2022-01-12T14:31:25Z

@AlexWaygood this is a grey area.

Thanks, that's really clarifying! It might be worth noting that @sobolevn proposes in #29158 (not yet merged) to use the module parameter of typing.ForwardRef to solve a bug relating to dataclasses and get_type_hints.

AlexWaygood · 2022-01-14T09:05:36Z

Btw @aha79 — could I ask if you've signed the CLA? For legal reasons, it may be difficult for CPython to merge a PR from you if you haven't 🙂

aha79 · 2022-01-14T09:09:51Z

Btw @aha79 — could I ask if you've signed the CLA? For legal reasons, it may be difficult for CPython to merge a PR from you if you haven't 🙂

Yes, sorry. I completely understand that. I clarify with my organization (This will be ok, but may take a few more days)

AlexWaygood · 2022-01-14T09:12:51Z

Yes, sorry. I completely understand that. I clarify with my organization (This will be ok, but may take a few more days)

Of course! No rush :)

aha79 · 2022-02-09T08:38:38Z

Yes, sorry. I completely understand that. I clarify with my organization (This will be ok, but may take a few more days)

Of course! No rush :)

@AlexWaygood The CLA is now signed. Sorry that it took a while.
Should I undo the __ repr__ change as @Fidget-Spinner suggests? Not sure if this is a final descision.

AlexWaygood · 2022-02-09T08:51:35Z

@AlexWaygood The CLA is now signed. Sorry that it took a while.

Thanks, no worries!

Should I undo the __ repr__ change as Fidget-Spinner suggests?

To me it makes sense to split off the __repr__ changes into a separate PR: the rest of the PR is clearly backportable, but the __repr__ changes seem like more of an enhancement.

Fidget-Spinner is currently on a break from open source — @JelleZijlstra, if you've got the time, do you have any thoughts on this PR?

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

aha79 · 2022-02-11T08:40:54Z

Could you possibly add some tests for the changes to __hash__, as well as the changes to __eq__? (test_forward_equality_hash() looks like a good candidate for where tests might be added.)

There were already tests for __eq__. But I added some for __hash__ as well (even it would not strictly be required that __hash__ changes with module - inclusion of module in __hash__ just avoids hash collisions but does not change behavior)

AlexWaygood · 2022-02-11T08:47:21Z

Sorry, I think I was unclear — my bad :)

We now have two test methods for testing ForwardRef.__hash__ — the one you've just added, and the existing one here:

cpython/Lib/test/test_typing.py

Line 2648 in 9dd2f8f

def test_forward_equality_hash(self):

.

Could you move the new tests for ForwardRef.__hash__ that you've just added (which look good) into the existing test method?

AlexWaygood · 2022-02-11T08:48:36Z

There were already tests for __eq__.

I didn't mean to imply otherwise!

aha79 · 2022-02-11T09:58:13Z

Sorry, I think I was unclear — my bad :)

We now have two test methods for testing ForwardRef.__hash__ — the one you've just added, and the existing one here:

cpython/Lib/test/test_typing.py

Line 2648 in 9dd2f8f

def test_forward_equality_hash(self):

.
Could you move the new tests for ForwardRef.__hash__ that you've just added (which look good) into the existing test method?

Sorry I misunderstood. I move/adapted the tests to the right place

AlexWaygood

Looks good to me. Feel free to open a new PR for the __repr__ changes (you can link to the same BPO issue).

AlexWaygood · 2022-02-11T15:53:58Z

@gvanrossum could we possibly get the full test suite run on this PR, please? :)

gvanrossum · 2022-02-11T20:45:17Z

@AlexWaygood / @JelleZijlstra Is this ready to merge?

AlexWaygood · 2022-02-11T20:46:44Z

@AlexWaygood / @JelleZijlstra Is this ready to merge?

Looks that way to me :)

JelleZijlstra · 2022-02-11T22:30:48Z

Yes I think it's good. It can wait though, hopefully I can merge it myself next week :)

miss-islington · 2022-02-17T03:28:21Z

Thanks @aha79 for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have the same name. This affects the `__eq__`, `__repr__` and `__hash__` methods. Co-authored-by: Andreas Hangauer <andreas.hangauer@siemens.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 6e7b813) Co-authored-by: aha79 <34090357+aha79@users.noreply.github.com>

bedevere-bot · 2022-02-17T03:28:47Z

GH-31379 is a backport of this pull request to the 3.10 branch.

The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have the same name. This affects the `__eq__`, `__repr__` and `__hash__` methods. Co-authored-by: Andreas Hangauer <andreas.hangauer@siemens.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 6e7b813) Co-authored-by: aha79 <34090357+aha79@users.noreply.github.com>

bedevere-bot · 2022-02-17T03:28:52Z

GH-31380 is a backport of this pull request to the 3.9 branch.

…H-31379) The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have the same name. This affects the `__eq__`, `__repr__` and `__hash__` methods. Co-authored-by: Andreas Hangauer <andreas.hangauer@siemens.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 6e7b813) Co-authored-by: aha79 <34090357+aha79@users.noreply.github.com> Automerge-Triggered-By: GH:JelleZijlstra

The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have the same name. This affects the `__eq__`, `__repr__` and `__hash__` methods. Co-authored-by: Andreas Hangauer <andreas.hangauer@siemens.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 6e7b813) Co-authored-by: aha79 <34090357+aha79@users.noreply.github.com>

bpo-46333: Honor module parameter in ForwardRef

980ea28

The `module` parameter carries semantic information about the forward ref. Forward refs are different if they refer to different module even if they have same name. This affects __eq__, __repr__ and __hash__ and methods.

aha79 requested review from Fidget-Spinner and gvanrossum as code owners January 11, 2022 15:28

the-knights-who-say-ni added the CLA not signed label Jan 11, 2022

This comment was marked as resolved.

Sign in to view

bedevere-bot added the awaiting review label Jan 11, 2022

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

Lib/test/test_typing.py Outdated Show resolved Hide resolved

Lib/typing.py Outdated Show resolved Hide resolved

blurb-it bot and others added 3 commits January 11, 2022 15:54

📜🤖 Added by blurb_it.

82fa061

Update Lib/test/test_typing.py

ac51d98

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Update Lib/typing.py

d643861

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst

ac16c5c

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Update ACKS

a2e1377

gvanrossum removed their request for review January 11, 2022 18:19

Fidget-Spinner reviewed Jan 12, 2022

View reviewed changes

Update Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst

c084fdb

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 1, 2022

JelleZijlstra self-requested a review February 9, 2022 15:06

aha79 and others added 2 commits February 11, 2022 09:01

Update Misc/NEWS.d/next/Library/2022-01-11-15-54-15.bpo-46333.B1faiF.rst

42fc26b

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

add tests for ForwardRef.hash

9dd2f8f

move ForwardRef.__hash__ test to correct function

94c9a69

AlexWaygood approved these changes Feb 11, 2022

View reviewed changes

AlexWaygood added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Feb 11, 2022

AlexWaygood requested a review from JelleZijlstra February 11, 2022 10:17

JelleZijlstra approved these changes Feb 11, 2022

View reviewed changes

aha79 mentioned this pull request Feb 11, 2022

bpo-46333: include module in ForwardRef.__repr__ #31283

Merged

JelleZijlstra merged commit 6e7b813 into python:main Feb 17, 2022

bedevere-bot removed the awaiting core review label Feb 17, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 17, 2022

bedevere-bot removed the needs backport to 3.9 only security fixes label Feb 17, 2022

Uh oh!

bpo-46333: Honor module parameter in ForwardRef #30536

bpo-46333: Honor module parameter in ForwardRef #30536

Uh oh!

Conversation

aha79 commented Jan 11, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood commented Jan 11, 2022

Uh oh!

gvanrossum commented Jan 11, 2022

Uh oh!

Fidget-Spinner commented Jan 12, 2022

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

aha79 Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aha79 Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Jan 12, 2022

Uh oh!

Fidget-Spinner commented Jan 12, 2022

Uh oh!

AlexWaygood commented Jan 12, 2022

Uh oh!

AlexWaygood commented Jan 14, 2022

Uh oh!

aha79 commented Jan 14, 2022

Uh oh!

AlexWaygood commented Jan 14, 2022

Uh oh!

aha79 commented Feb 9, 2022

Uh oh!

AlexWaygood commented Feb 9, 2022

Uh oh!

aha79 commented Feb 11, 2022

Uh oh!

AlexWaygood commented Feb 11, 2022

Uh oh!

AlexWaygood commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aha79 commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood commented Feb 11, 2022

Uh oh!

gvanrossum commented Feb 11, 2022

Uh oh!

AlexWaygood commented Feb 11, 2022

Uh oh!

JelleZijlstra commented Feb 11, 2022

Uh oh!

miss-islington commented Feb 17, 2022

Uh oh!

bpo-46333: Honor `module` parameter in ForwardRef #30536

bpo-46333: Honor `module` parameter in ForwardRef #30536

aha79 commented Jan 11, 2022 •

edited by bedevere-bot

Loading

Fidget-Spinner Jan 12, 2022 •

edited

Loading

aha79 Jan 12, 2022 •

edited

Loading

AlexWaygood commented Feb 11, 2022 •

edited

Loading

aha79 commented Feb 11, 2022 •

edited

Loading

AlexWaygood left a comment •

edited

Loading