This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: ForwardRef.__eq__ does not respect module parameter
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.11, Python 3.10, Python 3.9
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: AlexWaygood, JelleZijlstra, andreash, gvanrossum, kj, kumaraditya, miss-islington, python-dev, sobolevn
Priority: normal Keywords: patch

Created on 2022-01-10 15:20 by andreash, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 30536 merged python-dev, 2022-01-11 15:28
PR 31283 merged andreash, 2022-02-11 19:59
PR 31379 merged miss-islington, 2022-02-17 03:28
PR 31380 merged miss-islington, 2022-02-17 03:28
Messages (10)
msg410221 - (view) Author: Andreas H. (andreash) * Date: 2022-01-10 15:20
The __eq__ method of ForwardRef does not take into account the module parameter. 

However, ForwardRefs with dissimilar module parameters are referring to different types even if they have different name. Thus also the ForwardRef's with same name but different module, should not be considered equal.

Consider the following code

from typing import *

ZZ = Optional['YY'] 
YY = int

YY = Tuple[Optional[ForwardRef("YY", module=__name__)], int]
print( YY.__args__[0].__args__[0].__forward_module__ )
# this prints None, but should print __main__ (or whatever __name__ contains)

When the first ForwardRef is not created, the program behaves correctly

#ZZ = Optional['YY'] 
YY = int

YY = Tuple[Optional[ForwardRef("YY", module=__name__)], int]
print( YY.__args__[0].__args__[0].__forward_module__ )
# this prints __main__ (or whatever __name__ contains)

The issue is that the line `ZZ = Optional['YY']` creates a cache entry, which is re-used instead of the second definition `Optional[ForwardRef("YY", module=__name__)]` and thus shadows the different argument of ForwardRef.

This problem could be fixed if the __eq__ method of FowardRef also checks for module equality.

i.e. in ForwardRef.__eq__ in replace 

   return self.__forward_arg__ == other.__forward_arg__


   return self.__forward_arg__ == other.__forward_arg__  and  self.__forward__module__ == other.__forward__module__ 

Ideally, (and for consistency reasons) the `__repr__` method of `ForwardRef` would also include the module arg if it present:


    def __repr__(self):
        return f'ForwardRef({self.__forward_arg__!r})'


    def __repr__(self):
        if self.__forward_module__ is None:
            return f'ForwardRef({self.__forward_arg__!r})'
            return f'ForwardRef({self.__forward_arg__!r}, module={self.__forward__module!r})'
msg410237 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-01-10 18:10
Good catch. Do you want to submit a PR? I agree that the repr() could also be better (but please only show other values if they differ from the default).
msg410257 - (view) Author: Andreas H. (andreash) * Date: 2022-01-10 21:47
I will give it a try.
msg410397 - (view) Author: Ken Jin (kj) * (Python committer) Date: 2022-01-12 14:04
First of all, thanks for addressing my questions on the PR. I have one more question which I'll post here since it's not entirely related to your PR.

ForwardRef isn't meant to be explicitly instantiated by a user [1] (it's a typing internal class), so do you mind sharing what your current use case is please? My concern here is that exposing things in __repr__ would force us to keep `module` forever (I'm 100% fine with __eq__ and __hash__). After digging up the commit history, I now recall that the module parameter was used solely as a fix for ForwardRefs in different-module TypedDict (see GH-27017 or bpo-41249). It's not used anywhere else, so I don't know if your current use case was ever intended when the module parameter was added.

msg410407 - (view) Author: Andreas H. (andreash) * Date: 2022-01-12 16:01
Yeah, sure. The use-case is (de)serialization. Right now I use the library cattr, but there are many others. 

If you are interested there is related discussion in the cattr board [1].

The original problem is how to define the types for serialization.

1. If everything is a class, things work well, but not if type aliases are used: 

2. Type aliases sometimes have to be used - they cannot be avoided in all cases, 
   especially with recursive types. The famous example is 

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

   (Note: even though mypy does not support this construct, pylance meanwhile does [2])

3. `typing.Annotated` seems to be made for specifying additional information such as value ranges, right to be used
   in (de)serialization + validation contexts. Often these will just be type aliases (not used as class members). 
   Combination is possible with typing.NewType.

The problem is that the implicit `ForwardRef('Json')` cannot be automatically resolved (as it only a name with no context). 
There is really no way this could be handle inside a library such as cattr.  

When one wants to separate interface from implementation this issue is even more complicated. The module where the serialization function is called is typically different from the module with the type definition (This is probably more the norm than the exception)

The option I expored is to explicitly create the ForwardRef and specify the module parameter (even though I have to admit that I also did read that the ForwardRef is only for internal use). 
    Json = Union[ List[ForwardRef(Json',module=__name__)], Dict[str, ForwardRef(Json',module=__name__)], int, float, bool, None ]

Ugly, but this is better than nothing.

A (worse) alternative is to do

    Json = Union[ List['Json'], Dict[str, 'Json'], int, float, bool, None ]
    typing._eval_type(Json, globals(), locals())

That works since it puts the ForwardRefs into "evaluated" state (self.__forward_value__ is then set)

A third, but future, alternative could be to automatically decompose the argument of ForwardRef into module and type. Then one could write

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

and all above problems would be solved.

Then ForwardRef could stay internal and this is more readable. Even though, the static type checkers would need to understand `__name__+'TYPE'` constructs to be useful.

Anyhow, it would be nice to have a solution here.

msg410410 - (view) Author: Andreas H. (andreash) * Date: 2022-01-12 16:26
Ah, let me add one point: PEP563  (-> `from __future__ import annotations`) is also not helping. 

Even with PEP563 enabled, the JSON example  

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

needs to be written in exact the same way as without PEP563. In other words there are cases where `ForwardRef` cannot be avoided. And unforntunately these are the cases where we have the ForwardRef missing context issue.
msg413134 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-02-12 15:36
New changeset b70690bb37cc4bac695051484734eede0c1f9ada by aha79 in branch 'main':
bpo-46333: include `module` in `ForwardRef.__repr__` (#31283)
msg413380 - (view) Author: Jelle Zijlstra (JelleZijlstra) * (Python committer) Date: 2022-02-17 03:28
New changeset 6e7b813195f9bd6a2a15c1f00ef2c0180f6c751a by aha79 in branch 'main':
bpo-46333: Honor `module` parameter in ForwardRef (GH-30536)
msg413381 - (view) Author: miss-islington (miss-islington) Date: 2022-02-17 03:53
New changeset a17d59a6df146077b5420b36c35a1b82ab3f071a by Miss Islington (bot) in branch '3.10':
[3.10] bpo-46333: Honor `module` parameter in ForwardRef (GH-30536) (GH-31379)
msg413382 - (view) Author: miss-islington (miss-islington) Date: 2022-02-17 03:53
New changeset 29ae7d3996e073ec3f15c4e7486b17c1d0e3c8f5 by Miss Islington (bot) in branch '3.9':
bpo-46333: Honor `module` parameter in ForwardRef (GH-30536)
Date User Action Args
2022-04-11 14:59:54adminsetgithub: 90491
2022-02-17 03:55:35JelleZijlstrasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2022-02-17 03:53:34miss-islingtonsetmessages: + msg413382
2022-02-17 03:53:11miss-islingtonsetmessages: + msg413381
2022-02-17 03:28:47miss-islingtonsetpull_requests: + pull_request29530
2022-02-17 03:28:42miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request29529
2022-02-17 03:28:29JelleZijlstrasetmessages: + msg413380
2022-02-12 15:36:01gvanrossumsetmessages: + msg413134
2022-02-11 19:59:43andreashsetpull_requests: + pull_request29443
2022-02-09 08:46:18AlexWaygoodsetnosy: + JelleZijlstra, sobolevn
2022-01-12 16:26:23andreashsetmessages: + msg410410
2022-01-12 16:01:20andreashsetmessages: + msg410407
2022-01-12 14:04:25kjsetmessages: + msg410397
2022-01-11 15:28:20python-devsetkeywords: + patch
nosy: + python-dev

pull_requests: + pull_request28737
stage: needs patch -> patch review
2022-01-11 04:52:04kumaradityasetnosy: + kumaraditya
2022-01-10 22:03:58AlexWaygoodsetnosy: + AlexWaygood
2022-01-10 21:47:11andreashsetmessages: + msg410257
2022-01-10 18:10:27gvanrossumsetstage: needs patch
2022-01-10 18:10:18gvanrossumsetmessages: + msg410237
versions: + Python 3.11
2022-01-10 15:20:29andreashcreate