GeezerVison

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

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

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

@gvanrossum Sure. Leave it to me.

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

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 🙂

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)

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

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 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?

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)

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:

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

There were already tests for __eq__.

I didn't mean to imply otherwise!

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

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

@AlexWaygood / @JelleZijlstra Is this ready to merge?

@AlexWaygood / @JelleZijlstra Is this ready to merge?

Looks that way to me :)

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

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

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

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