Skip to content

Fix reporting location in aliased types. #12745

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 11, 2022
Merged

Fix reporting location in aliased types. #12745

merged 1 commit into from
Aug 11, 2022

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented May 7, 2022

Description

Fixes #12678 by updating line and column not only in an alias type variable but also recursively in arguments.

The current develop output

python3 -m pip install numpy mypy
echo -e "import numpy.typing as npt;\na: npt.NDArray[float]" > test_ntp.py
python3 -m mypy test_ntp.py --cache-dir=/dev/null
test_ntp.py:212: error: Type argument "float" of "dtype" must be a subtype of "generic"
Found 1 error in 1 file (checked 1 source file)
python3 -m mypy test_ntp.py --cache-dir=/dev/null --pretty
Traceback (most recent call last):
  File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/miha/.local/lib/python3.8/site-packages/mypy/__main__.py", line 34, in <module>
    console_entry()
  File "/home/miha/.local/lib/python3.8/site-packages/mypy/__main__.py", line 12, in console_entry
    main(None, sys.stdout, sys.stderr)
  File "mypy/main.py", line 96, in main
  File "mypy/main.py", line 173, in run_build
  File "mypy/build.py", line 180, in build
  File "mypy/build.py", line 256, in _build
  File "mypy/build.py", line 2727, in dispatch
  File "mypy/build.py", line 3075, in process_graph
  File "mypy/build.py", line 3192, in process_stale_scc
  File "mypy/errors.py", line 649, in file_messages
  File "mypy/errors.py", line 622, in format_messages
IndexError: list index out of range

The branch output

python3 -m mypy test_ntp.py --cache-dir=/dev/null
test_ntp.py:2: error: Type argument "float" of "dtype" must be a subtype of "generic"
Found 1 error in 1 file (checked 1 source file)
python3 -m mypy test_ntp.py --cache-dir=/dev/null --pretty
test_ntp.py:2: error: Type argument "float" of "dtype" must be a subtype of "generic"
    a: npt.NDArray[float]
       ^
Found 1 error in 1 file (checked 1 source file)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

oxidase
oxidase commented May 7, 2022
View reviewed changes
test-data/unit/check-generics.test Outdated
reveal_type(a) # N: Revealed type is "other.array[Any, other.dtype[builtins.float]]"

[out]
main:2: error: Type argument "float" of "dtype" must be a subtype of "generic"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

output with the current master is

  main:12: error: Type argument "float" of "dtype" must be a subtype of "generic" (diff)

where 12 is the line number in other.py but is used as a reference with main.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with --pretty flag the test fails on master as

[gw0] linux -- Python 3.8.10 /home/miha/foss/mypy/venv/bin/python3
data: /home/miha/foss/mypy/test-data/unit/check-generics.test:651:
/home/miha/foss/mypy/mypy/test/testcheck.py:141: in run_case
    self.run_case_once(testcase)
/home/miha/foss/mypy/mypy/test/testcheck.py:199: in run_case_once
    res = build.build(sources=sources,
/home/miha/foss/mypy/mypy/build.py:181: in build
    result = _build(
/home/miha/foss/mypy/mypy/build.py:257: in _build
    graph = dispatch(sources, manager, stdout)
/home/miha/foss/mypy/mypy/build.py:2752: in dispatch
    process_graph(graph, manager)
/home/miha/foss/mypy/mypy/build.py:3110: in process_graph
    process_stale_scc(graph, scc, manager)
/home/miha/foss/mypy/mypy/build.py:3227: in process_stale_scc
    manager.flush_errors(manager.errors.file_messages(graph[id].xpath), False)
/home/miha/foss/mypy/mypy/errors.py:707: in file_messages
    return self.format_messages(self.error_info_map[path], source_lines)
/home/miha/foss/mypy/mypy/errors.py:680: in format_messages
    source_line = source_lines[line - 1]
E   IndexError: list index out of range

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
JukkaL
JukkaL reviewed May 9, 2022
View reviewed changes
mypy/types.py Outdated
tp.line = newline
tp.column = newcolumn
for ta in getattr(tp, 'args', []):
replace_alias_location(ta, newline, newcolumn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might not find all component types (e.g. within callables). Also, we prefer to avoid getattr since it can't be type checked by mypy and mypyc generates slow code for it.

Can you instead define a subclass of TypeTraverserVisitor, and override at least visit_instance to update line/column info. You can use it to update all component type objects (of the desired types), and in a type-safe manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JukkaL I've updated the PR, please could you check again?

@github-actions

This comment has been minimized.

Sign in to view
@oxidase oxidase requested a review from JukkaL May 23, 2022 06:26
@oxidase
Copy link
Contributor Author

oxidase commented May 31, 2022

@JukkaL any chance to get the fix merged?

JukkaL
JukkaL reviewed May 31, 2022
View reviewed changes
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.

A few additional minor things, otherwise looks good.

mypy/types.py Outdated
new_tp.column = newcolumn
return new_tp
newtp = tp.accept(replacer)
newtp.accept(LocationSetter(newline, newcolumn))
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make sure that we don't regress any existing use case, can you also set the line and column attributes of the return value of the visitor, in case line/column of non-Instance types is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

mypy/types.py
@@ -2925,16 +2926,26 @@ def visit_type_var(self, typ: TypeVarType) -> Type:
return typ


class LocationSetter(TypeTraverserVisitor):
def __init__(self, line: int, column: int) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a TODO comment along the lines of "Should we update locations of other Type subclasses?".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@oxidase
Copy link
Contributor Author

oxidase commented Jun 1, 2022

@JukkaL Thanks for your comments! I've updated PR as suggested.

@github-actions

This comment has been minimized.

Sign in to view
@oxidase oxidase requested a review from JukkaL June 10, 2022 16:08
@oxidase
Copy link
Contributor Author

oxidase commented Jun 21, 2022

@JukkaL any chance to get the fix merged into the next release?
The PR also should fix #11009

@oxidase
Copy link
Contributor Author

oxidase commented Jul 19, 2022

@JukkaL ping

@ilevkivskyi
Copy link
Member

@oxidase There is now merge conflict, sorry, can't merge. Can you please rebase/merge?

@github-actions

This comment has been minimized.

Sign in to view
@oxidase
Copy link
Contributor Author

oxidase commented Aug 10, 2022

@JukkaL rebased to the current master

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@oxidase
Copy link
Contributor Author

oxidase commented Aug 11, 2022

@JukkaL sorry, I can not merge in the repository due to

Only those with write access to this repository can merge pull requests.

Please could you merge ^ PR?

JukkaL
JukkaL approved these changes Aug 11, 2022
View reviewed changes
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.

Thanks for the updates!

@JukkaL JukkaL merged commit 601802c into python:master Aug 11, 2022
@oxidase oxidase deleted the fix/12678 branch August 11, 2022 13:53
@matthewhughes934 matthewhughes934 mentioned this pull request Aug 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JukkaL JukkaL JukkaL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can not find "numpy.core._multiarray_umath" if numpy is installed via pip with --target argument.
3 participants
@oxidase @ilevkivskyi @JukkaL