Show error locations in other modules on warm runs #20635
Conversation
This comment has been minimized.
This comment has been minimized.
|
Oh, hm, the primer shows an actual problem in my logic. I need to recurse into compound statements when searching for defs. Probably not hard to do. I will update the PR today/tomorrow. |
|
OK, hopefully this should be fixed now. Also opened #20640 as a follow-up for |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Nice, this fixes one of the biggest remaining deviations between warm and cold runs. Left a few comments, overall looks good.
| tmp/a.py:2: error: Unexpected keyword argument "uhhhh" for "lol" | ||
| tmp/b.py:2: note: "lol" defined here | ||
|
|
||
| [case testCachedUnexpectedKeywordArgumentNested] |
There was a problem hiding this comment.
Test also when the target is a method, and also when multiple classes in the target file have a method of the same name?
| return visitor.found | ||
|
|
||
|
|
||
| class DefinitionSeeker(StatementVisitor[None]): |
There was a problem hiding this comment.
Can you add a docstring that explains that this finds the topmost functions/classes with the given name, and it has to be used multiple times to find methods or nested classes. I was a bit confused by this initially. (Or add docstring to find_definition above).
| else: | ||
| path = state.manager.maybe_swap_for_shadow_path(state.path) | ||
| source = decode_python_encoding(state.manager.fscache.read(path)) | ||
| tree = parse(source, state.path, state.id, state.manager.errors, state.options) |
There was a problem hiding this comment.
I guess we could only run semantic analysis pass 1 to get reachability information, and then find_definitions would have to skip unreachable blocks? But this is not a big deal.
2 participants
Fixes #4772
This also allows to remove all but one
_no_paralleltest suffixes. I decided to keep the last one (caused by an existing inconsistent behaviour inpartialplugin) for now.I am still fascinated by how non-trivial this seemingly simple fix is. The key idea is to split the error tuple generation in two phases: first phase can produce relative error locations (as symbol full names) and those are serialized to cache. We then resolve relative locations in the second phase (which may potentially require re-parsing some ASTs that were already discarded, I cache these globally per-run). Couple missing things in the PR:
SymbolNodeare supported (so e.g. assignments are not yet supported).Both are fine for now, since neither of two cases are actually used, we can support those later when/if needed.