Skip to content

Fix reporting location in aliased types.#12745

Merged
JukkaL merged 1 commit into
python:masterfrom
oxidase:fix/12678
Aug 11, 2022
Merged

Fix reporting location in aliased types.#12745
JukkaL merged 1 commit into
python:masterfrom
oxidase:fix/12678

Conversation

@oxidase

@oxidase oxidase commented May 7, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread 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"

@oxidase oxidase May 7, 2022

Copy link
Copy Markdown
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.

@oxidase oxidase May 8, 2022

Copy link
Copy Markdown
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
Comment thread mypy/types.py Outdated
tp.line = newline
tp.column = newcolumn
for ta in getattr(tp, 'args', []):
replace_alias_location(ta, newline, newcolumn)

@JukkaL JukkaL May 9, 2022

Copy link
Copy Markdown
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.

@oxidase oxidase May 12, 2022

Copy link
Copy Markdown
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

oxidase commented May 31, 2022

Copy link
Copy Markdown
Contributor Author

@JukkaL any chance to get the fix merged?

JukkaL
JukkaL reviewed May 31, 2022
View reviewed changes

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

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.

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

@JukkaL JukkaL May 31, 2022

Copy link
Copy Markdown
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.

@oxidase oxidase Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment thread mypy/types.py


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

@JukkaL JukkaL May 31, 2022

Copy link
Copy Markdown
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?".

@oxidase oxidase Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added

@oxidase

oxidase commented Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

@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

oxidase commented Jun 21, 2022

Copy link
Copy Markdown
Contributor Author

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

@oxidase

oxidase commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

@JukkaL ping

@ilevkivskyi

ilevkivskyi commented Aug 10, 2022

Copy link
Copy Markdown
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

oxidase commented Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

@JukkaL rebased to the current master

@github-actions

github-actions Bot commented Aug 10, 2022

Copy link
Copy Markdown
Contributor

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

@oxidase

oxidase commented Aug 11, 2022

Copy link
Copy Markdown
Contributor Author

@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

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

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