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

Better error location for incompatible override due to argument incompatibility #8298

Closed
JukkaL opened this issue Jan 16, 2020 · 6 comments · Fixed by #14562
Closed

Better error location for incompatible override due to argument incompatibility #8298

JukkaL opened this issue Jan 16, 2020 · 6 comments · Fixed by #14562

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 16, 2020

An incompatible override gets reported on the first line of a method header, even though it would be better to point it at the line with an argument that is incompatible, when possible. Example:

class A:
    def f(self, x: int, y: str, z: float) -> None: pass

class B(A):
    def f(  # Argument 3 incompatible with supertype "A" <-- reported here
            self,
            x: int,
            y: str,
            z: bool,  # <-- would be better to report it here
    ) -> None:
        pass

This would improve usability, especially for methods with many arguments on many lines. This would also make it cleaner to ignore these errors, as the location of the # type: ignore comment would document which argument is incompatible (if there is one argument per line, which is a common convention).

@dhalenok
Copy link
Contributor

@JukkaL I'd like to help with this issue but not quite sure about where to start, any suggestions?

@JukkaL
Copy link
Collaborator Author

JukkaL commented Feb 4, 2020

@DenisOH Look for calls to argument_incompatible_with_supertype in mypy/checker.py to find the relevant code.

@Arshaan-256
Copy link

@DenisOH Can I work on this?

@ethanhs
Copy link
Collaborator

ethanhs commented Feb 26, 2020

@Arshaan-256 it looks like Denis has already made a PR for this!

@Arshaan-256
Copy link

Oh! I am so sorry. I thought that the PR didn't work since the issue was still open. Sorry!

@dhalenok
Copy link
Contributor

@Arshaan-256 I'm still waiting on that PR to be reviewed. However, if it's not good or needs improvement, feel free to take over the issue.

@AlexWaygood AlexWaygood added topic-inheritance Inheritance and incompatible overrides topic-error-reporting How we report errors labels Mar 29, 2022
JukkaL pushed a commit that referenced this issue Feb 16, 2023
Modifies arguments parsed from the AST to use the AST's row and column
information. Modifies function definitions to not overwrite their
arguments' locations. Modifies incorrect override messages to use the
locations of arguments instead of the method itself. Modifies tests to
expect the new locations.

I'm not sure whether this handles bound arguments correctly; it passes
tests but I don't know whether there's some edge case I'm missing.

Fixes #8298.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants