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

Give arguments a more reasonable location #14562

Conversation

koogoro
Copy link
Collaborator

@koogoro koogoro commented Jan 30, 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.

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.
@github-actions

This comment has been minimized.

- Only fetch python AST's end column and line when available
- Move type-ignore to the line with an argument
- Update python-slow test case to use location of argument
@koogoro
Copy link
Collaborator Author

koogoro commented Jan 30, 2023

This might have to include some flag to use the old behavior, or to try to detect type: ignore at the old location, since it'll break existing users otherwise.

@github-actions

This comment has been minimized.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 31, 2023

This might have to include some flag to use the old behavior, or to try to detect type: ignore at the old location, since it'll break existing users otherwise.

I like the idea of detecting a type: ignore at the old location, if it's not hard to implement. Otherwise we'll need two feature releases before we can enable this by default (as it breaks backward compatibility), and it will cause some friction for users.

Add another context to this message reporting. This means that we'll now check both contexts when looking at what errors to ignore.
@koogoro koogoro force-pushed the maxmurin/better-location-for-incompatibility-error branch from 4d0993a to 6e325ee Compare February 13, 2023 21:56
@github-actions
Copy link
Contributor

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

pip (https://github.com/pypa/pip)
- src/pip/_internal/network/xmlrpc.py:32: error: Argument 3 of "request" is incompatible with supertype "Transport"; supertype defines the argument type as "Union[bytes, Union[bytearray, memoryview, array[Any], mmap, _CData, PickleBuffer]]"  [override]
+ src/pip/_internal/network/xmlrpc.py:36: error: Argument 3 of "request" is incompatible with supertype "Transport"; supertype defines the argument type as "Union[bytes, Union[bytearray, memoryview, array[Any], mmap, _CData, PickleBuffer]]"  [override]
- src/pip/_internal/network/xmlrpc.py:32: note: This violates the Liskov substitution principle
+ src/pip/_internal/network/xmlrpc.py:36: note: This violates the Liskov substitution principle
- src/pip/_internal/network/xmlrpc.py:32: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ src/pip/_internal/network/xmlrpc.py:36: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

operator (https://github.com/canonical/operator)
- ops/framework.py:688: error: Name "kind" already defined on line 679  [no-redef]
+ ops/framework.py:688: error: Name "kind" already defined on line 680  [no-redef]
- ops/charm.py:1086: error: Name "raw" already defined on line 1082  [no-redef]
+ ops/charm.py:1086: error: Name "raw" already defined on line 1083  [no-redef]
- ops/charm.py:1087: error: Name "actions_raw" already defined on line 1082  [no-redef]
+ ops/charm.py:1087: error: Name "actions_raw" already defined on line 1084  [no-redef]

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs)
- rolemanagement/utils.py:57: error: Argument 2 of "update_roles_atomically" is incompatible with supertype "MixinMeta"; supertype defines the argument type as "Optional[List[Any]]"  [override]
+ rolemanagement/utils.py:61: error: Argument 2 of "update_roles_atomically" is incompatible with supertype "MixinMeta"; supertype defines the argument type as "Optional[List[Any]]"  [override]
- rolemanagement/utils.py:57: note: This violates the Liskov substitution principle
+ rolemanagement/utils.py:61: note: This violates the Liskov substitution principle
- rolemanagement/utils.py:57: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ rolemanagement/utils.py:61: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
- rolemanagement/utils.py:57: error: Argument 3 of "update_roles_atomically" is incompatible with supertype "MixinMeta"; supertype defines the argument type as "Optional[List[Any]]"  [override]
+ rolemanagement/utils.py:62: error: Argument 3 of "update_roles_atomically" is incompatible with supertype "MixinMeta"; supertype defines the argument type as "Optional[List[Any]]"  [override]
+ rolemanagement/utils.py:62: note: This violates the Liskov substitution principle
+ rolemanagement/utils.py:62: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

ibis (https://github.com/ibis-project/ibis)
- ibis/backends/duckdb/__init__.py:276: error: Argument 1 of "read_csv" is incompatible with supertype "_FileIOHandler"; supertype defines the argument type as "Union[str, Path]"  [override]
+ ibis/backends/duckdb/__init__.py:278: error: Argument 1 of "read_csv" is incompatible with supertype "_FileIOHandler"; supertype defines the argument type as "Union[str, Path]"  [override]
- ibis/backends/duckdb/__init__.py:276: note: This violates the Liskov substitution principle
+ ibis/backends/duckdb/__init__.py:278: note: This violates the Liskov substitution principle
- ibis/backends/duckdb/__init__.py:276: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ ibis/backends/duckdb/__init__.py:278: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
- ibis/backends/duckdb/__init__.py:320: error: Argument 1 of "read_parquet" is incompatible with supertype "_FileIOHandler"; supertype defines the argument type as "Union[str, Path]"  [override]
+ ibis/backends/duckdb/__init__.py:322: error: Argument 1 of "read_parquet" is incompatible with supertype "_FileIOHandler"; supertype defines the argument type as "Union[str, Path]"  [override]
- ibis/backends/duckdb/__init__.py:320: note: This violates the Liskov substitution principle
+ ibis/backends/duckdb/__init__.py:322: note: This violates the Liskov substitution principle
- ibis/backends/duckdb/__init__.py:320: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ ibis/backends/duckdb/__init__.py:322: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

@koogoro
Copy link
Collaborator Author

koogoro commented Feb 13, 2023

Looks like the primer is only detecting changed line numbers now, which is desirable.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Looks good, and thanks for preserving backward compatibility! This makes the error messages easier to understand, as they point to a more relevant location. Being able to place type ignore comments more precisely is nice.

@JukkaL JukkaL merged commit d586070 into python:master Feb 16, 2023
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.

Better error location for incompatible override due to argument incompatibility
2 participants