Fix disappearing errors when re-running dmypy check by meshy · Pull Request #14835 · python/mypy · GitHub

This adds a commit which fixes issue #9655 wherein some types of error would be lost when a file was re-processed by dmypy. Regression tests are also included.

This also fixes another error where sometimes files would not be re-processed by dmypy if the only error in the file was either "unused type ignore" or "ignore without code".

This test currently fails with this error: AssertionError: Command 3 (dmypy check -- bar.py) did not give expected output --- Captured stderr call --- Expected: bar.py:2: error: Unused "type: ignore" comment (diff) == Return code: 1 (diff) Actual: (empty) It demonstrates a bug that when an module is removed using `FineGrainedBuildManager.update` because it is not "seen" by `fine_grained_increment_follow_imports`, then "unused type: ignore" warnings disappear from subsequent checks. Ref: python#9655

Findings: the error is removed from the fine-grained manager on this line (manager.errors.reset()), and isn't restored thereafter. I don't yet understand what mechanism should restore it, or why it is not restored.

I have noticed that the "example" module is not listed in manager.errors.targets(). Adding the target to the ErrorInfo class generated when this error is made was not sufficient to fix the issue.

Apologies if this is a little rambly. Please ask if there is anything I can clarify.

TLDR: There are a couple of things going on here, and I don't have a solution that solves both yet have added a commit which should fix both.

Issue 1: We miss some important steps when modules are re-checked by reprocess_nodes in update.py.
Issue 2: We do not call reprocess_nodes when unused type ignore errors are the only errors in a file.

Issue 1 details:

On the first pass (in build.py), unused ignore errors are added to the manager.errors just after calling type_check_second_pass. Notice that we call generate_unused_ignore_notes:

mypy/mypy/build.py

Lines 3435 to 3445 in 4365dad

    
           while unfinished_modules: 
        
               for id in stale: 
        
                   if id not in unfinished_modules: 
        
                       continue 
        
                   if not graph[id].type_check_second_pass(): 
        
                       unfinished_modules.discard(id) 
        
                       graph[id].detect_possibly_undefined_vars() 
        
                       graph[id].finish_passes() 
        
           for id in stale: 
        
               graph[id].generate_unused_ignore_notes() 
        
               graph[id].generate_ignore_without_code_notes()

On the second pass (in update.py), while we do a similar "second pass" check, we do not then look for unused type ignores.

mypy/mypy/server/update.py

Lines 1024 to 1028 in 4365dad

    
           more = checker.check_second_pass(nodes) 
        
           while more: 
        
               more = False 
        
               if graph[module_id].type_checker().check_second_pass(): 
        
                   more = True

Notice that the above does not call generate_unused_ignore_notes, and is also missing:

generate_ignore_without_code_notes
detect_possibly_undefined_vars
finish_passes

Adding this line the code-block above will cause the error to be shown...

diff --git mypy/server/update.py mypy/server/update.py
index 00b823c99..7c499afb8 100644
--- mypy/server/update.py
+++ mypy/server/update.py
@@ -1027,6 +1027,9 @@ def reprocess_nodes(
         if graph[module_id].type_checker().check_second_pass():
             more = True
 
+    graph[module_id].generate_unused_ignore_notes()
+
     if manager.options.export_types:
         manager.all_types.update(graph[module_id].type_map())

... but only if there is already another error in the file (see Issue 2). I assume that it would make sense to call the other missing methods too, but I don't have a failing tests for them yet (and I don't know what finish_passes does).

Issue 2 details:

The ErrorInfo instance used to represent an unused type ignore error does not have a target assigned to it. This means that when a file has no other errors, it will not be reprocessed because it is not included in FineGrainedBuildManager.previous_targets_with_errors, which is used to work out which files to re-process.

I can work around that by adding this...

diff --git mypy/errors.py mypy/errors.py
index 2c2c1e5ca..552756646 100644
--- mypy/errors.py
+++ mypy/errors.py
@@ -668,6 +668,8 @@ class Errors:
                 False,
                 False,
                 False,
+                origin=(self.file, None),
+                target=self.current_target(),
             )
             self._add_error_info(file, info)

(Something similar will be required in generate_ignore_without_code_notes().)

Unfortunately, that is incompatible with the solution to Issue 1, because on the second pass there is some missing state, and I get this error:

Daemon crashed!
Traceback (most recent call last):
  File ".../mypy/dmypy_server.py", line 231, in serve
    resp = self.run_command(command, data)
  File ".../mypy/mypy/dmypy_server.py", line 278, in run_command
    ret = method(self, **data)
  File ".../mypy/mypy/dmypy_server.py", line 356, in cmd_check
    return self.check(sources, export_types, is_tty, terminal_width)
  File ".../mypy/mypy/dmypy_server.py", line 415, in check
    messages = self.fine_grained_increment_follow_imports(sources)
  File ".../mypy/mypy/dmypy_server.py", line 664, in fine_grained_increment_follow_imports
    messages = fine_grained_manager.update([], to_delete)
  File ".../mypy/mypy/server/update.py", line 288, in update
    changed_modules = propagate_changes_using_dependencies(
  File ".../mypy/mypy/server/update.py", line 881, in propagate_changes_using_dependencies
    triggered |= reprocess_nodes(manager, graph, id, nodes, deps, processed_targets)
  File ".../mypy/mypy/server/update.py", line 1030, in reprocess_nodes
    graph[module_id].generate_unused_ignore_notes()
  File ".../mypy/mypy/build.py", line 2587, in generate_unused_ignore_notes
    self.manager.errors.generate_unused_ignore_errors(self.xpath)
  File ".../mypy/mypy/errors.py", line 672, in generate_unused_ignore_errors
    target=self.current_target(),
  File ".../mypy/mypy/errors.py", line 344, in current_target
    return self.scope.current_target()
  File ".../mypy/mypy/scope.py", line 33, in current_target
    assert self.module
AssertionError

I'm now investigating how to ensure that manager.errors.scope.module is appropriately set, but if anyone can suggest a course of action there, I'd appreciate it.

EDIT: I have now added more tests, and proposed a solution.

This test fails with the error: AssertionError: Command 3 (dmypy check -- bar.py) did not give expected output --- Captured stderr call --- Expected: bar.py:2: error: "type: ignore" comment without error code [ignore-without-code] (diff) == Return code: 1 (diff) Actual: (empty) This test illustrates that '"type: ignore" comment without error code' errors currently disappear in the same way that 'Unused "type: ignore"' errors do as described in python#9655. Ref: python#9655

This test fails with the error: AssertionError: Command 3 (dmypy check -- bar.py) did not give expected output --- Captured stderr call --- Expected: bar.py:4: error: Name "a" may be undefined [possibly-undefined] (diff) == Return code: 1 (diff) Actual: (empty) This test illustrates that possibly-undefined errors currently disappear in the same way that 'Unused "type: ignore"' errors do as described in python#9655. Ref: python#9655

I'm happy that this works in my small examples now, but I have some minor questions about my approach.

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

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.

Question 1: I'm not sure if origin is required here. I noticed that the other places that passed in target= passed this too, so I assumed it couldn't hurt. Should I remove it?
UPDATE: I've changed origin= to origin=(self.file, [line]) now.

Question 2: I would have liked to use target=self.current_target() here, similar to the other uses in the file, but as I detailed in another comment that causes an error. Is this good enough? Are we losing important context by taking this approach?

Before this change, if a module was removed with `update` in the `FineGrainedBuildManager`, then some errors would not re-appear. This change ensures that three types of error are maintained when running `reprocess_nodes`: - possibly undefined vars - unused ignores - ignores without codes By adding targets to ErrorInfos, this also fixes an issue where unused ignore and ignores without codes errors didn't get re-processed when they were the only issue in the file. Fixes python#9655

What is the status of this pull request? I keep running into this issue and it means I have to keep restarting the daemon when I'm type checking my code.

What is the status of this pull request?

I'm happy with the changes in this PR now, and am awaiting feedback. I don't know what the next steps are to get this merged.

I keep running into this issue and it means I have to keep restarting the daemon when I'm type checking my code.

Have you tried out this branch locally? Does this solve the issue for you?

I don't know much about dmypy or incremental mode, so I may not be the best reviewer here (though I'm still hoping to take a look soon anyway, when I have the time!). I'm also only a triager, not a maintainer, so I don't have merge privileges.

Hopefully @ilevkivskyi will be able to take a look, if he has the time :)

Have you tried out this branch locally? Does this solve the issue for you?

That's unfortunate, just tried installing this locally and it halfway works, subsequent runs without changing the file keep getting the same list of comments like expected, but then if the file changes still no comments even though there are errors.

If it's helpful, here is my list of flags for the daemon:

Details check_untyped_defs = true disallow_any_generics = true disallow_untyped_calls = true disallow_untyped_defs = true ignore_missing_imports = true no_implicit_optional = true no_implicit_reexport = true show_column_numbers = true show_error_codes = true strict = true strict_equality = true warn_redundant_casts = true warn_return_any = true warn_unreachable = true warn_unused_configs = true warn_unused_ignores = true

just tried installing this locally and it halfway works, subsequent runs without changing the file keep getting the same list of comments like expected, but then if the file changes still no comments even though there are errors.

Thanks for checking this out, and finding this new angle on the issue. I've confirmed that making changes to the file does cause the unused-ignore and untyped-ignore warnings to disappear. Interestingly, the possibly-undefined error appears to remain.

I haven't yet looked into how this is happening. Hopefully I will get some more time for this soon.

There aren't any updates in these tests, we just re-run dmypy.

These tests show how some errors disappear on a re-run of dmypy after a file is altered.

@meshy I think to fix the issue reported by @CoolCat467 you will also need to change update_module_isolated(), currently it only calls detect_possibly_undefined_vars(), but not the other two.

Unfortunately it looks like it is not possible to avoid the code duplication without a bigger refactoring, so I think if it fixes the issue it should be fine for now.

I don't have Windows, so I can't reproduce this locally. Can someone please help me with this?

I can reproduce this locally on Windows, with Python 3.11, so it does seem to be something Windows-specific rather than py37-specific.

@meshy, applying this diff to your PR branch fixes the issue for me locally on Windows:

diff --git a/test-data/unit/daemon.test b/test-data/unit/daemon.test
index 34d1b2b92..869f60b4e 100644
--- a/test-data/unit/daemon.test
+++ b/test-data/unit/daemon.test
@@ -564,7 +564,7 @@ Daemon started
 $ dmypy check -- bar.py
 bar.py:2: error: Unused "type: ignore" comment
 == Return code: 1
-$ echo '' >> bar.py
+$ {python} -c "print('\n')" >> bar.py
 $ dmypy check -- bar.py
 bar.py:2: error: Unused "type: ignore" comment
 == Return code: 1
@@ -582,7 +582,7 @@ Daemon started
 $ dmypy check -- bar.py
 bar.py:2: error: "type: ignore" comment without error code  [ignore-without-code]
 == Return code: 1
-$ echo '' >> bar.py
+$ {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

Before this change, tests failed because the way we modified a file with echoing didn't work the same way on windows. This change uses Python to print a newline to the end of the file, which doesn't run afoul of the same issues with newlines. Co-authored-by: AlexWaygood <alex.waygood@gmail.com>

Closing and reopening to retrigger CI

Thank you both for your help, that seems right.

@AlexWaygood I have applied your patch, and listed you as a co-author on that commit.

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

@meshy congrats on your first mypy contribution! This was a tough first PR 😀

Thank you very much! ❤️

This reverts commit a9ee618. The original fix doesn't work in all cases. Reverting it since fixing remaining issues is non-trivial, and it's better not to include regressions or partially implemented features in mypy 1.3. See #9655 for context.

Unfortunately I'm planning to revert this (#15179), due to remaining issues that may be non-trivial to fix (see #15043). I'm happy to receive an updated PR that also includes fixes to the issues. By reverting the change, there's no pressure to get the fixes included in mypy 1.3, which should be out soon.

TBH I don't understand the reason. Does this PR causes any troubles (new errors)? Before this PR the logic in update.py was clearly wrong, this PR makes it at least generally consistent.

@ilevkivskyi unfortunately yes, it did cause a regression: #15043

OK, I see. I thought that was one of remaining unfixed errors.

…#15179) This reverts commit a9ee618. The original fix doesn't work in all cases. Reverting it since fixing remaining issues is non-trivial, and it's better not to include regressions or partially implemented features in mypy 1.3. See #9655 for context.

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?

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.

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.

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

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.

…#15179) This reverts commit a9ee618. The original fix doesn't work in all cases. Reverting it since fixing remaining issues is non-trivial, and it's better not to include regressions or partially implemented features in mypy 1.3. See #9655 for context.

This catches a regression caused by the previous attempt to fix python#9655 where "type: ignore" comments are erroneously marked as unused in re-runs of dmypy. Ref: python#14835

I think I've now addressed the issues which lead to this being reverted, and opened the new PR for review.

This catches a regression caused by the previous attempt to fix python#9655 where "type: ignore" comments are erroneously marked as unused in re-runs of dmypy. Ref: python#14835

meshy mentioned this pull request Mar 4, 2023

cached runs of dmypy stop reporting "unused type: ignore" warnings #9655

Open

AlexWaygood added topic-daemon dmypy topic-type-ignore # type: ignore comments labels Mar 5, 2023

meshy commented Mar 12, 2023

View reviewed changes

Comment thread test-data/unit/daemon.test

meshy force-pushed the issue-9655 branch from 496b33e to 1831cbd Compare March 12, 2023 22:30

meshy commented Mar 13, 2023

View reviewed changes

meshy changed the title Demonstrate disappearing "unused type: ignore" warnings Fix disappearing errors when re-running dmypy check Mar 13, 2023

meshy requested a review from AlexWaygood March 13, 2023 00:28

meshy marked this pull request as ready for review March 13, 2023 00:30

This comment has been minimized.

Sign in to view

meshy force-pushed the issue-9655 branch from fa65250 to eac851b Compare March 13, 2023 08:33

This comment has been minimized.

Sign in to view

AlexWaygood requested a review from ilevkivskyi March 25, 2023 13:08

meshy added 2 commits March 31, 2023 23:28

Improve test names

5bfb6b5

There aren't any updates in these tests, we just re-run dmypy.

Add new failing tests

3d8d2b7

These tests show how some errors disappear on a re-run of dmypy after a file is altered.

meshy marked this pull request as draft March 31, 2023 23:34

This comment has been minimized.

Sign in to view

AlexWaygood closed this Apr 9, 2023

AlexWaygood reopened this Apr 9, 2023

ilevkivskyi closed this Apr 9, 2023

ilevkivskyi reopened this Apr 9, 2023

ilevkivskyi merged commit a9ee618 into python:master Apr 9, 2023

meshy deleted the issue-9655 branch April 9, 2023 19:08

meshy mentioned this pull request May 1, 2023

Type ignore comments erroneously marked as unused by dmypy #15043

Merged

JukkaL reviewed May 4, 2023

View reviewed changes

+                  graph[module_id].detect_possibly_undefined_vars()
+                  graph[module_id].generate_unused_ignore_notes()
+                  graph[module_id].generate_ignore_without_code_notes()

Fix disappearing errors when re-running dmypy check by meshy · Pull Request #14835 · python/mypy · GitHub

Repository navigation

Fix disappearing errors when re-running dmypy check - #14835

Fix disappearing errors when re-running dmypy check#14835ilevkivskyi merged 8 commits intopython:masterpython/mypy:masterfrom meshy:issue-9655meshy/mypy:issue-9655Copy head branch name to clipboard

Conversation

meshy commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshy commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

meshy commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshy left a comment

Choose a reason for hiding this comment

Uh oh!

meshy Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

JukkaL May 4, 2023

Choose a reason for hiding this comment

Uh oh!

meshy Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

CoolCat467 commented Mar 25, 2023

Uh oh!

meshy commented Mar 25, 2023

Uh oh!

meshy commented Mar 25, 2023

Uh oh!

AlexWaygood commented Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CoolCat467 commented Mar 25, 2023

Uh oh!

meshy commented Mar 27, 2023

Uh oh!

ilevkivskyi commented Apr 3, 2023

Uh oh!

This comment has been minimized.

AlexWaygood commented Apr 9, 2023

Uh oh!

AlexWaygood commented Apr 9, 2023

Uh oh!

AlexWaygood commented Apr 9, 2023

Uh oh!

meshy commented Apr 9, 2023

Uh oh!

github-actions Bot commented Apr 9, 2023

Uh oh!

AlexWaygood commented Apr 9, 2023

Uh oh!

meshy commented Apr 9, 2023

Uh oh!

JukkaL commented May 4, 2023

Uh oh!

ilevkivskyi commented May 4, 2023

Uh oh!

meshy commented May 4, 2023

Uh oh!

ilevkivskyi commented May 4, 2023

Uh oh!

JukkaL May 4, 2023

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi May 6, 2023

Choose a reason for hiding this comment

Uh oh!

meshy Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

meshy Jan 11, 2024

Choose a reason for hiding this comment

Uh oh!

Fix disappearing errors when re-running dmypy check#14835
ilevkivskyi merged 8 commits into
python:masterfrom
meshy:issue-9655

meshy commented Mar 4, 2023 •
edited
Loading

meshy commented Mar 7, 2023 •
edited
Loading

meshy commented Mar 12, 2023 •
edited
Loading

meshy Mar 13, 2023 •
edited
Loading

AlexWaygood commented Mar 25, 2023 •
edited
Loading