Refactor import diagnostics, fix diagnostics in some fine-grained cases #4840
Conversation
mypy/build.py
Outdated
| root_source: bool = False) -> Tuple[str, str]: | ||
| """Find a module by name, respecting follow_imports and producing diagnostics. | ||
|
|
||
| """ |
There was a problem hiding this comment.
Document the return value and arguments.
mypy/build.py
Outdated
|
|
||
| def skipping_module(manager: BuildManager, line: int, caller_state: Optional[State], | ||
| id: str, path: str) -> None: | ||
| assert caller_state, (id, path) |
There was a problem hiding this comment.
Add short docstring (no need to explain arguments in detail)?
mypy/build.py
Outdated
|
|
||
|
|
||
| def skipping_ancestor(manager: BuildManager, id: str, path: str, ancestor_for: 'State') -> None: | ||
| # TODO: Read the path (the __init__.py file) and return |
There was a problem hiding this comment.
Add short docstring (no need to explain arguments in detail)?
mypy/build.py
Outdated
| manager, self.options, dep, | ||
| caller_state=state, caller_line=line, | ||
| ancestor_for=ancestor) | ||
| except Exception: |
There was a problem hiding this comment.
A catch-all except looks a bit suspicious. Would it be possible to explicitly enumerate the exceptions that we are expecting here? Some exceptions such as TypeErrors we probably don't want to silently ignore.
| [file a.py] | ||
| import b | ||
| from c import x | ||
| [file b.py] |
There was a problem hiding this comment.
Maybe include something here that would trigger an error to verify that this actually gets skipped? Maybe also update b.py?
| [file a.py] | ||
| from c import x | ||
| import b | ||
| [file b.py] |
There was a problem hiding this comment.
Again maybe introduce an error here that shouldn't be reported?
| a.py:2: note: Import of 'b' ignored | ||
| a.py:2: note: (Using --follow-imports=error, module not passed on command line) | ||
|
|
||
| -- This test fails because p.b doesn't seem to trigger p properly... |
There was a problem hiding this comment.
Create an issue about this (if one doesn't already exist) and mention it here.
mypy/build.py
Outdated
| @@ -2008,6 +1909,34 @@ def write_cache(self) -> None: | |||
| self.mark_interface_stale() | |||
| self.interface_hash = new_interface_hash | |||
|
|
|||
| def verify_dependencies(self) -> None: | |||
| """Report errors for import targets in module that don't exist.""" | |||
mypy/build.py
Outdated
| @@ -2018,6 +1947,132 @@ def generate_unused_ignore_notes(self) -> None: | |||
| if self.options.warn_unused_ignores: | |||
| self.manager.errors.generate_unused_ignore_notes(self.xpath) | |||
|
|
|||
| # Module import and diagnostic glue | |||
mypy/build.py
Outdated
| root_source: bool = False) -> Tuple[str, str]: | ||
| """Find a module by name, respecting follow_imports and producing diagnostics. | ||
|
|
||
| """ |
mypy/build.py
Outdated
| if self.priorities.get(dep) != PRI_INDIRECT] | ||
| assert self.ancestors is not None | ||
| for dep in dependencies + self.suppressed + self.ancestors: | ||
| if dep not in manager.modules and not manager.options.ignore_missing_imports: |
There was a problem hiding this comment.
Hm, ignore_missing_imports is a per-module option. Can you use graph[dep].options.ignore_missing_imports? (Though you may have to figure out how to pass the graph here.) I don't think that self.options is quite right (since it indicates whether self.id should be ignored if not found, not modules it depends on) but I'm not 100% sure.
There was a problem hiding this comment.
Good catch.
graph[dep].options won't be the right thing, since the whole problem here is that the module isn't in graph, which is why we are considering reporting an error. I think the right thing is manager.options.clone_for_module(dep).
mypy/build.py
Outdated
| "(Using --follow-imports=error, submodule passed on command line)", | ||
| severity='note', only_once=True) | ||
|
|
||
| # The driver |
mypy/build.py
Outdated
| # - skip -> don't analyze, make the type Any | ||
| follow_imports = options.follow_imports | ||
| if (root_source # Honor top-level modules | ||
| or not (path.endswith('.py') # Stubs are always normal |
There was a problem hiding this comment.
Indent this and the next line 4 more. Also, maybe "or not (X or Y)" is more clearly written as "or (not X and not Y)"? Seeing 'not' twice feels less offensive than having to decipher "or not or".
Refactor all of the import diagnostics so that they can be used
outside of
State's__init__method.Then use that to fix some problems with import diagnostics in fine-grained mode.
Fixes #4798.
This refactor also lays the ground work for fixing (coarse) incremental mode's interaction
with
--warn-unused-ignores