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

@overload decorator on compiled functions is not tested #96478

Open
sobolevn opened this issue Sep 1, 2022 · 5 comments
Open

@overload decorator on compiled functions is not tested #96478

sobolevn opened this issue Sep 1, 2022 · 5 comments
Labels
expert-typing tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@sobolevn
Copy link
Member

sobolevn commented Sep 1, 2022

If I change this line

pass

to be

    try:
        _overload_registry[f.__module__][f.__qualname__][f.__code__.co_firstlineno] = func
    except AttributeError:
        # Not a normal function; ignore.
        raise # pass

test_typing continues to pass as expected. I think it is dangerous, because we can accidentally break something and not notice.

I will send a simple test case to catch this: we should not fail.

@sobolevn sobolevn added the type-bug An unexpected behavior, bug, or error label Sep 1, 2022
@AlexWaygood AlexWaygood added tests Tests in the Lib/test dir expert-typing labels Sep 1, 2022
sobolevn added a commit to sobolevn/cpython that referenced this issue Sep 1, 2022
sobolevn added a commit to sobolevn/cpython that referenced this issue Sep 1, 2022
sobolevn added a commit to sobolevn/cpython that referenced this issue Sep 1, 2022
@sobolevn
Copy link
Member Author

sobolevn commented Sep 1, 2022

Moreover, this has a side-effect:

overload(sum)

creates a following entry in _overload_registry:

defaultdict(<function OverloadTests.<lambda> at 0x10185cec0>, {'builtins': defaultdict(<class 'dict'>, {'print': {}})})

@sobolevn
Copy link
Member Author

sobolevn commented Sep 1, 2022

This happens because only the last [f.__code__.co_firstlineno] = func part of

    try:
        _overload_registry[f.__module__][f.__qualname__][f.__code__.co_firstlineno] = func
    except AttributeError:
        # Not a normal function; ignore.
        pass

fails with AttrtibuteError. Others are keys are recorded already by a defaultdict. I don't think it is a big deal, but it is surely interesting.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 1, 2022

The current code for overload() is:

f = getattr(func, "__func__", func)
try:
    _overload_registry[f.__module__][f.__qualname__][f.__code__.co_firstlineno] = func
except AttributeError:
    # Not a normal function; ignore.
    pass
return _overload_dummy

If we wanted to get rid of the side effect, we could maybe change the code to something like this:

f = getattr(func, "__func__", func)
try:
    mod, qualname, lineno = f.__module__, f.__qualname__, f.__code__.co_firstlineno
except AttributeError:
    # Not a normal function; ignore.
    pass
else:
    _overload_registry[mod][qualname][lineno] = func
return _overload_dummy

But we worked quite hard to make sure that the performance regression introduced by making overloads retrievable at runtime was as small as possible. So I'd want to make sure that this didn't make things any slower.

@sobolevn
Copy link
Member Author

sobolevn commented Sep 1, 2022

I don't think that we want to remove this side-effect. get_overloads still works correctly for end users 🤔

Moreover, annotating C functions is not that common. It is more like an exceptional case.
Slowing down regular use-cases is not worth it in my opinion.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 1, 2022

I don't think that we want to remove this side-effect. get_overloads still works correctly for end users 🤔

Moreover, annotating C functions is not that common. It is more like an exceptional case. Slowing down regular use-cases is not worth it in my opinion.

Yeah, the possibility of this side effect causing a memory leak is pretty minimal :)

JelleZijlstra pushed a commit that referenced this issue Sep 5, 2022
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 5, 2022
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
(cherry picked from commit f177f6f)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit to sobolevn/typing_extensions that referenced this issue Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expert-typing tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants