bpo-37953: Fix ForwardRef equality checks

[plokmijnuhby]

Ideally if we stick a ForwardRef in a dictionary we would like to reliably be able to get it out again.

https://bugs.python.org/issue37953

[gvanrossum]

This looks reasonable, but let's make sure @ilevkivskyi agrees.

Can you add a test (preferably one that shows what's not working without the fix)?

It's fairly unorthodox to have a PR without a bug in the tracker -- can you create a bug describing the problem and link this to it? Also please add a news item.

[ilevkivskyi]

TBH I don't understand what problem is being solved. This definitely requires tests.

[gvanrossum]

The problem seems clear enough to me: The __hash__() and __eq__() operators compute a value that depends on __forward_value__, but it's possible to use a ForwardRef as a dict key before the latter field is assigned (it is initialized to None). If then (through an alias) its _evaluate() is called, which assigns a non-None value to __forward_value__, the hash and equality change, which violates the rules for hashable objects and confuses the dict lookup operation so the lookup will fail.

I'm not clear however on why someone would use a ForwardRef as a dict key. Maybe @plokmijnuhby can explain their use case?

[ilevkivskyi]

OK, I see how the current status is wrong. On the other hand, just removing the value for evaluated forward references will make two references referring to different classes with the same name equal.

I think that the first one is more important, so I am fine with just removing the __forward_value__ from __hash__() and __eq__().

[ilevkivskyi]

Mostly looks good, just one question.

Please also add a NEWS item.

[hongweipeng]

I try a test in this PR:

from typing import ForwardRef, get_type_hints

def namespace1():
    a = ForwardRef('A')

    class A: pass
    def fun(x: a): pass

    get_type_hints(fun, globals(), locals())
    return hash(a)

def namespace2():
    a = ForwardRef('A')

    class A: pass
    def fun(x: a): pass

    get_type_hints(fun, globals(), locals())
    return hash(a)

print(namespace1() == namespace2())

Output True in this PR and output False in master branch.

I'm afraid of removing the __forward_value__ can break a legitimate code.

[brandtbucher]

[edited]

@hongweipeng, I see what you mean now. This is definitely tricky.

I think combining the __eq__ from #15628 with the __hash__ from this PR makes the most sense.

[hongweipeng]

@brandtbucher Yeah, I want to use __eq__ to remove duplicate item instead of using __hash__.

Now I think that __eq__ is only used for ForwardRef, while others keep the original __hash__ way.

[ilevkivskyi]

Now I think that __eq__ is only used for ForwardRef, while others keep the original __hash__ way.

TBH I don't really like making __hash__() and __eq__() inconsistent. There are however precedents, for example hash('abc') == hash(b'abc')is True while 'abc' == b'abc' is False. So a possible thing I could imagine is keeping only __forward_arg__, but also take into account __forward_value__ in __eq__() if it is not None for self and other.

IOW I am fine with

I think combining the __eq__ from #15628 with the __hash__ from this PR makes the most sense.

[hongweipeng]

Removing __forward_value__ can solve the recursion problem incidentally issue37806.

[ilevkivskyi]

Removing __forward_value__ can solve the recursion problem incidentally.

Do you mean removing it from __hash__ or from both places?

Generally, I would propose to keep only one PR open, because currently the discussion is scattered between two PRs. @plokmijnuhby Are you interested in continuing to work on this PR?

[hongweipeng]

@ilevkivskyi Yes, we can remove __forward_value__ in __hash__ and keep it in __eq__. In this case, an actually recursive object will also throw an RecursionError normally.

[plokmijnuhby]

@ilevkivskyi I'm happy to keep working on it, but I'm not quite sure how where we want to take this. It seems like there are still a few ideas bouncing around here. I mean, in theory we could remove the caching thing entirely, and just perform a lookup of the class name in a function's __globals__ whenever we need to evaluate a forward reference on it.

[aeros]

I'm not entirely certain if this is something we'd consider to be a behavioral changing enhancement or if the current behavior is an actual bug. For now, I'll categorize it as enhancement but feel free to change it.

[plokmijnuhby]

Okay, here's my thoughts on forward references. @hongweipeng's code example doesn't make much sense to me because users are unlikely to evaluate references right after creating them, i.e. in the same calling context, but much more likely to have someone else's code evaluate them a little later, in a completely different calling context. The trouble is that we have no good way of determining the correct context after we've left it.

There are several possible solutions to this:

• WONTFIX the issue and ignore it.
• Detect the calling context the moment a forward reference is created, through sys._getframe hacks. Not a brilliant idea, particularly since using the form def foo(a: 'Bar') to use strings as forward references means we'd also have to do some some sort of monkey patch to str, so let's avoid that one.
• Changes to the parser itself. This would probably slow down the whole python system.
• A change to how we think about forward references. Forward references could be considered to have no meaning unless they have an obvious context available, eg if globals and locals were passed to get_type_hints. This would mean that a forward reference is basically just a string that can be executed in a fancy way, so it makes sense to mark it as immutable. This is the approach I was leaning towards.

[ilevkivskyi]

So we now have two competing almost identical PRs: this one and #15650. This one was first, but the other has more tests. I think the best way is to polish this one, so I am going to close the alternative one.

[ilevkivskyi]

Use `__hash__` and `__eq__` instead of just hash and eq. Also this news item is too long, couple short sentences (or even one) should be enough.

[plokmijnuhby]

I changed it, is that better?

[aeros]

Also, thanks for the PR and welcome @plokmijnuhby. (:

[ilevkivskyi]

Thanks for updates! LGTM now.

[ilevkivskyi]

Thanks everyone for help!

@plokmijnuhby To make things consistent, you might also want to add this fix to the backport at https://github.com/python/typing.

[miss-islington]

Thanks @plokmijnuhby for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-16128 is a backport of this pull request to the 3.8 branch.