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

aha79

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/ :)

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

AlexWaygood

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

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

Fidget-Spinner

@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.

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

AlexWaygood

@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

@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

@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

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

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

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

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

@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

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

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

There were already tests for __eq__.

I didn't mean to imply otherwise!

aha79

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

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

gvanrossum

@AlexWaygood / @JelleZijlstra Is this ready to merge?

AlexWaygood

@AlexWaygood / @JelleZijlstra Is this ready to merge?

Looks that way to me :)

JelleZijlstra

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

miss-islington

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

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

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

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

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 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 label Feb 17, 2022

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!

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!

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!

bedevere-bot commented Feb 17, 2022

Uh oh!

bedevere-bot commented Feb 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

aha79 commented Jan 11, 2022 •

edited by bedevere-bot

Loading

AlexWaygood commented Feb 11, 2022 •

edited

Loading

aha79 commented Feb 11, 2022 •

edited

Loading

AlexWaygood left a comment •

edited

Loading