Skip to content

Fix disappearing errors when re-running dmypy check #14835

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

Merged
merged 8 commits into from
Apr 9, 2023
Merged

Fix disappearing errors when re-running dmypy check #14835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b4569fa
Add test to demonstrate issue #9655
meshy Mar 4, 2023
1831cbd
Prove ignores without error codes also disappear
meshy Mar 12, 2023
6094fc4
Prove possibly-undefined errors disappear
meshy Mar 12, 2023
eac851b
Ensure errors persist after dmypy update
meshy Mar 13, 2023
5bfb6b5
Improve test names
meshy Mar 31, 2023
3d8d2b7
Add new failing tests
meshy Mar 31, 2023
520f8e8
Ensure errors persist after file change
meshy Apr 8, 2023
6c14e7d
Fix no-op file modification on Windows
meshy Apr 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mypy/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,8 @@ def generate_unused_ignore_errors(self, file: str) -> None:
False,
False,
False,
origin=(self.file, [line]),
target=self.target_module,
)
self._add_error_info(file, info)

Expand Down Expand Up @@ -720,6 +722,8 @@ def generate_ignore_without_code_errors(
False,
False,
False,
origin=(self.file, [line]),
target=self.target_module,
)
self._add_error_info(file, info)

Expand Down
6 changes: 6 additions & 0 deletions mypy/server/update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,8 @@ def restore(ids: list[str]) -> None:
state.type_check_first_pass()
state.type_check_second_pass()
state.detect_possibly_undefined_vars()
state.generate_unused_ignore_notes()
state.generate_ignore_without_code_notes()
t2 = time.time()
state.finish_passes()
t3 = time.time()
Expand Down Expand Up @@ -1027,6 +1029,10 @@ def key(node: FineGrainedDeferredNode) -> int:
if graph[module_id].type_checker().check_second_pass():
more = True

graph[module_id].detect_possibly_undefined_vars()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: I haven't added a call to finish_passes() here. I'm not sure what it does... should it be here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so, since finish_passes seems to only perform things related to non-fine-grained builds.

I think that there may be another issue here: detect_possible_undefined_vars() is somewhat expensive, as it runs over the entire file. In this function it would be better to only analyze the nodes that are being processed. For example, we could pass the set of targets to detect_possibly_undefined_vars, and it would only catch errors in these targets. I'm not sure if processing the entire file causes correctness issues, but plausibly it could.

graph[module_id].generate_unused_ignore_notes()
graph[module_id].generate_ignore_without_code_notes()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it may be more correct to only perform these after the end of a build increment, after we've propagated all changes. After a call of reprocess_nodes we might still be in the middle of processing some files, and some errors could still be not generated yet. So maybe we'd need to keep track of all the files that have processed nodes, and call these functions at the end?

@ilevkivskyi What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know TBH, I thought we clear those "intermediate" errors. So I guess we may clear them for some reason? If yes, this should be fixed. But if we fixing that is hard, then yes, we can delay generating these errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi both -- thank you for your comments on this.

Apologies for not addressing your points before.

I've re-opened this PR (with some minor changes to address a regression) in #15043, and I'd appreciate your feedback there. In particular, I'd really welcome your thoughts on what should be done to address the comments you left here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I have removed these three lines from the new PR.


if manager.options.export_types:
manager.all_types.update(graph[module_id].type_map())

Expand Down
89 changes: 89 additions & 0 deletions test-data/unit/daemon.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,3 +522,92 @@ class A:
x: int
class B:
x: int

[case testUnusedTypeIgnorePreservedOnRerun]
-- Regression test for https://github.com/python/mypy/issues/9655
$ dmypy start -- --warn-unused-ignores --no-error-summary
Daemon started
$ dmypy check -- bar.py
bar.py:2: error: Unused "type: ignore" comment
== Return code: 1
$ dmypy check -- bar.py
bar.py:2: error: Unused "type: ignore" comment
== Return code: 1

[file foo/__init__.py]
[file foo/empty.py]
[file bar.py]
from foo.empty import *
a = 1 # type: ignore

[case testTypeIgnoreWithoutCodePreservedOnRerun]
-- Regression test for https://github.com/python/mypy/issues/9655
$ dmypy start -- --enable-error-code ignore-without-code --no-error-summary
Daemon started
$ dmypy check -- bar.py
bar.py:2: error: "type: ignore" comment without error code [ignore-without-code]
== Return code: 1
$ dmypy check -- bar.py
bar.py:2: error: "type: ignore" comment without error code [ignore-without-code]
== Return code: 1

[file foo/__init__.py]
[file foo/empty.py]
[file bar.py]
from foo.empty import *
a = 1 # type: ignore

[case testUnusedTypeIgnorePreservedAfterChange]
-- Regression test for https://github.com/python/mypy/issues/9655
$ dmypy start -- --warn-unused-ignores --no-error-summary
Daemon started
$ dmypy check -- bar.py
bar.py:2: error: Unused "type: ignore" comment
== Return code: 1
$ {python} -c "print('\n')" >> bar.py
$ dmypy check -- bar.py
bar.py:2: error: Unused "type: ignore" comment
== Return code: 1

[file foo/__init__.py]
[file foo/empty.py]
[file bar.py]
from foo.empty import *
a = 1 # type: ignore

[case testTypeIgnoreWithoutCodePreservedAfterChange]
-- Regression test for https://github.com/python/mypy/issues/9655
$ dmypy start -- --enable-error-code ignore-without-code --no-error-summary
Daemon started
$ dmypy check -- bar.py
bar.py:2: error: "type: ignore" comment without error code [ignore-without-code]
== Return code: 1
$ {python} -c "print('\n')" >> bar.py
$ dmypy check -- bar.py
bar.py:2: error: "type: ignore" comment without error code [ignore-without-code]
== Return code: 1

[file foo/__init__.py]
[file foo/empty.py]
[file bar.py]
from foo.empty import *
a = 1 # type: ignore

[case testPossiblyUndefinedVarsPreservedAfterUpdate]
-- Regression test for https://github.com/python/mypy/issues/9655
$ dmypy start -- --enable-error-code possibly-undefined --no-error-summary
Daemon started
$ dmypy check -- bar.py
bar.py:4: error: Name "a" may be undefined [possibly-undefined]
== Return code: 1
$ dmypy check -- bar.py
bar.py:4: error: Name "a" may be undefined [possibly-undefined]
== Return code: 1

[file foo/__init__.py]
[file foo/empty.py]
[file bar.py]
from foo.empty import *
if False:
a = 1
a