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

Closed
sobolevn opened this issue Sep 1, 2022 · 6 comments
Closed

@overload decorator on compiled functions is not tested #96478

sobolevn opened this issue Sep 1, 2022 · 6 comments
Labels
tests Tests in the Lib/test dir topic-typing 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 topic-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

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

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
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 5, 2022
Co-authored-by: Alex Waygood <[email protected]>
(cherry picked from commit f177f6f)

Co-authored-by: Nikita Sobolev <[email protected]>
sobolevn added a commit to sobolevn/typing_extensions that referenced this issue Sep 6, 2022
JelleZijlstra pushed a commit to python/typing_extensions that referenced this issue Sep 6, 2022
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this issue Sep 6, 2022
./python.exe -m test -R : test.test_typing would fail, apparently
because the dictionary used in the @patch decorator was modified.
JelleZijlstra added a commit that referenced this issue Sep 6, 2022
./python.exe -m test -R : test.test_typing would fail, apparently
because the dictionary used in the @patch decorator was modified.
miss-islington added a commit that referenced this issue Sep 21, 2022
Co-authored-by: Alex Waygood <[email protected]>
(cherry picked from commit f177f6f)

Co-authored-by: Nikita Sobolev <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 27, 2022
./python.exe -m test -R : test.test_typing would fail, apparently
because the dictionary used in the @patch decorator was modified.
(cherry picked from commit f0d9136)

Co-authored-by: Jelle Zijlstra <[email protected]>
miss-islington added a commit that referenced this issue Sep 27, 2022
./python.exe -m test -R : test.test_typing would fail, apparently
because the dictionary used in the @patch decorator was modified.
(cherry picked from commit f0d9136)

Co-authored-by: Jelle Zijlstra <[email protected]>
@kumaraditya303
Copy link
Contributor

Fixed by #96479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants