Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ForwardRef.__eq__ does not respect module parameter #90491

Closed
aha79 mannequin opened this issue Jan 10, 2022 · 10 comments
Closed

ForwardRef.__eq__ does not respect module parameter #90491

aha79 mannequin opened this issue Jan 10, 2022 · 10 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@aha79
Copy link
Mannequin

aha79 mannequin commented Jan 10, 2022

BPO 46333
Nosy @gvanrossum, @JelleZijlstra, @miss-islington, @sobolevn, @Fidget-Spinner, @kumaraditya303, @AlexWaygood, @aha79
PRs
  • bpo-46333: Honor module parameter in ForwardRef #30536
  • bpo-46333: include module in ForwardRef.__repr__ #31283
  • [3.10] bpo-46333: Honor module parameter in ForwardRef (GH-30536) #31379
  • [3.9] bpo-46333: Honor module parameter in ForwardRef (GH-30536) #31380
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2022-02-17.03:55:35.674>
    created_at = <Date 2022-01-10.15:20:29.114>
    labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
    title = 'ForwardRef.__eq__ does not respect module parameter'
    updated_at = <Date 2022-02-17.03:55:35.673>
    user = 'https://github.com/aha79'

    bugs.python.org fields:

    activity = <Date 2022-02-17.03:55:35.673>
    actor = 'JelleZijlstra'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-02-17.03:55:35.674>
    closer = 'JelleZijlstra'
    components = ['Library (Lib)']
    creation = <Date 2022-01-10.15:20:29.114>
    creator = 'andreash'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46333
    keywords = ['patch']
    message_count = 10.0
    messages = ['410221', '410237', '410257', '410397', '410407', '410410', '413134', '413380', '413381', '413382']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'python-dev', 'JelleZijlstra', 'miss-islington', 'sobolevn', 'kj', 'kumaraditya', 'AlexWaygood', 'andreash']
    pr_nums = ['30536', '31283', '31379', '31380']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue46333'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @aha79
    Copy link
    Mannequin Author

    aha79 mannequin commented Jan 10, 2022

    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 typing.py replace

    return self.__forward_arg__ == other.__forward_arg__

    with

    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:

    Change:

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

    to

        def __repr__(self):
            if self.__forward_module__ is None:
                return f'ForwardRef({self.__forward_arg__!r})'
            else:
                return f'ForwardRef({self.__forward_arg__!r}, module={self.__forward__module!r})'

    @aha79 aha79 mannequin added 3.10 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jan 10, 2022
    @gvanrossum
    Copy link
    Member

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

    @gvanrossum gvanrossum added 3.11 only security fixes labels Jan 10, 2022
    @aha79
    Copy link
    Mannequin Author

    aha79 mannequin commented Jan 10, 2022

    I will give it a try.

    @Fidget-Spinner
    Copy link
    Member

    @andreas
    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 #71204 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.

    @aha79
    Copy link
    Mannequin Author

    aha79 mannequin commented Jan 12, 2022

    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)

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

    @aha79
    Copy link
    Mannequin Author

    aha79 mannequin commented Jan 12, 2022

    Ah, let me add one point: PEP-563 (-> from __future__ import annotations) is also not helping.

    Even with PEP-563 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 PEP-563. 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.

    @gvanrossum
    Copy link
    Member

    New changeset b70690b by aha79 in branch 'main':
    bpo-46333: include module in ForwardRef.__repr__ (bpo-31283)
    b70690b

    @JelleZijlstra
    Copy link
    Member

    New changeset 6e7b813 by aha79 in branch 'main':
    bpo-46333: Honor module parameter in ForwardRef (GH-30536)
    6e7b813

    @miss-islington
    Copy link
    Contributor

    New changeset a17d59a by Miss Islington (bot) in branch '3.10':
    [3.10] bpo-46333: Honor module parameter in ForwardRef (GH-30536) (GH-31379)
    a17d59a

    @miss-islington
    Copy link
    Contributor

    New changeset 29ae7d3 by Miss Islington (bot) in branch '3.9':
    bpo-46333: Honor module parameter in ForwardRef (GH-30536)
    29ae7d3

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants