Skip to content

Add no-overload-impl error code #11944

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

Merged
merged 3 commits into from Jan 15, 2022
Merged

Add no-overload-impl error code #11944

merged 3 commits into from Jan 15, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 9, 2022

Description

Adds a new no-overload-impl error code that is raised when overloaded functions outside of stub files are not followed by an implementation.

Test Plan

I validated the doc changes and all tests are passing locally. I did not find anywhere in the tests that the error code of specific failures is checked, but please let me know if I missed something.

@ghost
Copy link
Author

ghost commented Jan 9, 2022

@hauntsaninja following up on your message on gitter, I've opened a PR that adds the relevant error code. Please take a look when you have a chance, thanks!

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghost
Copy link
Author

ghost commented Jan 9, 2022

Do we need to update tests in https://github.com/python/mypy/blob/master/test-data/unit/check-errorcodes.test ?

Added, thanks! I missed that the test cases were mostly defined under test-data

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
- steam/ext/commands/bot.py:671: error: An overloaded function outside a stub file must have an implementation  [misc]
+ steam/ext/commands/bot.py:671: error: An overloaded function outside a stub file must have an implementation  [no-overload-impl]

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/mark/structures.py:405: error: An overloaded function outside a stub file must have an implementation  [no-overload-impl]
+ src/_pytest/mark/structures.py:423: error: An overloaded function outside a stub file must have an implementation  [no-overload-impl]

@sobolevn
Copy link
Member

sobolevn commented Jan 9, 2022

@bphillips-exos can you please check why this new entry in mypy_primer is shown?

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/mark/structures.py:405: error: An overloaded function outside a stub file must have an implementation  [no-overload-impl]
+ src/_pytest/mark/structures.py:423: error: An overloaded function outside a stub file must have an implementation  [no-overload-impl]

This might be a newly added code, but it is better to double check 🙂

@ghost
Copy link
Author

ghost commented Jan 9, 2022

Here is the relevant snippet from the pytest source code. Pytest is overriding the annotations for some decorator definitions with overloads. The ignore[misc] decorator is required because this is a python file, not pyi. If this PR is merged, pytest would simply need to modify the ignore comment to be # type: ignore[override,no-overload-impl]. I'm not familiar with mypy_primer, but it's possible it isn't handling this edge case correctly.

L405
L423

# Typing for builtin pytest marks. This is cheating; it gives builtin marks
# special privilege, and breaks modularity. But practicality beats purity...
if TYPE_CHECKING:
    from _pytest.fixtures import _Scope

    class _SkipMarkDecorator(MarkDecorator):
        @overload  # type: ignore[override,misc]   (L405)
        def __call__(self, arg: _Markable) -> _Markable:
            raise NotImplementedError()

        @overload  # noqa: F811
        def __call__(self, reason: str = ...) -> "MarkDecorator":  # noqa: F811
            raise NotImplementedError()

@sobolevn
Copy link
Member

sobolevn commented Jan 9, 2022

mypy_primer runs your branch of mypy on multiple open-source projects.

Looks like the reason for the new issue is exact type: ignore error code. Thank you!

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It seems to me like it might make sense for this error code to be generalised to something like "overload-def". It could cover a couple other lints that mypy has about overload definitions, for instance "Single overload definition, multiple required" and "@Final should be applied only to overload implementation". Thoughts?

@ghost
Copy link
Author

ghost commented Jan 11, 2022

Thanks! It seems to me like it might make sense for this error code to be generalised to something like "overload-def". It could cover a couple other lints that mypy has about overload definitions, for instance "Single overload definition, multiple required" and "@Final should be applied only to overload implementation". Thoughts?

Hi hauntsaninja, I actually don't like the idea of generalizing error codes. In my opinion, much of the value of error codes comes from being able to be very specific about which errors to ignore. I find pylint does this very well and I can turn off specific checks when they don't make sense for a particular project.

For the use case I am trying to address with this PR, I have a codebase which supports dynamic dispatch based on overloaded function signatures. I would like to disable no-overload-impl globally so that I don't have to # type: ignore this feature everywhere it is used. If the error codes were generalized, I would be forced to also disable all the other related overload checks that I would like to keep.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thank you again :-)

@hauntsaninja hauntsaninja merged commit 9943444 into python:master Jan 15, 2022
@ghost ghost deleted the add-overload-impl-error-code branch January 16, 2022 12:52
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants