Fine-grained: Don't infer partial types from multiple targets#4553
Fine-grained: Don't infer partial types from multiple targets#4553
Conversation
Definitions of local partial types can't span multiple fine-grained incremental targets.
|
This is ready for review but don't merge yet since I want to fix various issues in internal Dropbox codebases first. |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks, looks good, I have few suggestions here.
| def is_defined_in_base_class(self, var: Var) -> bool: | ||
| if var.info is not None: | ||
| for base in var.info.mro[1:]: | ||
| if base.get(var.name()) is not None: |
There was a problem hiding this comment.
Shouldn't we also check that it is not None in base class?
There was a problem hiding this comment.
Sorry, I'm not sure what you mean by this. Can you give an example where this would make a difference?
There was a problem hiding this comment.
I mean add ... and not isinstance(base.get(var.name()), NoneTyp)
There was a problem hiding this comment.
base.get() returns Optional[SymbolTableNode] so that exact check doesn't make sense. If you meant guarding agains a definition like x = None # type: None in a base class, I don't agree -- it should be treated as a valid definition. If you still don't agree, can you give me a test case where this would make a difference?
There was a problem hiding this comment.
Yes, sorry, you are right, this is perfectly valid.
mypy/checker.py
Outdated
| ]) | ||
|
|
||
| # Keeps track of partial types in a single scope | ||
| PartialTypeScope = NamedTuple('PartialTypeMap', [('map', Dict[Var, Context]), |
There was a problem hiding this comment.
Shouldn't NAME in NAME = NamedTuple('NAME', ...) always be the same in l.h.s. and r.h.s.?
mypy/checker.py
Outdated
|
|
||
| # Keeps track of partial types in a single scope | ||
| PartialTypeScope = NamedTuple('PartialTypeMap', [('map', Dict[Var, Context]), | ||
| ('is_function', bool)]) |
There was a problem hiding this comment.
I think a short comment about why we need to keep track about is_function will help here. (My understanding this is because fine-grained targets are always top level functions.)
| # flags: --local-partial-types --strict-optional | ||
| from typing import Any | ||
|
|
||
| A: Any |
There was a problem hiding this comment.
I would add the same test where A is not Any, but a normal class. (And None in two subclasses like here.)
| y = '' | ||
| [out] | ||
| == | ||
| a.py:1: error: Need type annotation for 'y' |
There was a problem hiding this comment.
Just for completes I would add a tests where the error appears in the first run, but not in the second one (like if a user listened and actually added an annotation).
|
The travis failure seems like a known flake. |
* master: (27 commits) Don't call --strict-optional and --incremental experimental (python#4642) Sync typeshed (python#4641) Fix callable types with inconsistent argument counts (python#4611) Fix example (add 'class A:') Make psutil an optional dependency (python#4634) mypy and mypy_extensions aren't posix only (python#3765) Documentation for attr support (python#4632) Use read_with_python_encoding in stubgen to handle file encoding (python#3790) Sync typeshed (python#4631) Add remaining core team emails to CREDITS (python#4629) Fix issues with attr code. (python#4628) Better support for converter in attrs plugin. (python#4607) Clean up credits (python#4626) Support type aliases in fine-grained incremental mode (python#4525) Fine-grained: Fix crash caused by unreachable class (python#4613) Treat divmod like a binary operator (python#4585) Sync typeshed (python#4605) Fine-grained: Don't infer partial types from multiple targets (python#4553) Fine-grained: Compare symbol table snapshots when following dependencies (python#4598) Fix type of forward reference to a decorated class method (python#4486) ...
…#4553) Previously if partial types were inferred from assignments in multiple scopes, like in the example below, fine-grained incremental mode could display bogus messages, since only one of the scopes could be reprocessed in one incremental update: ``` x = None def f() -> None: global x x = '' ``` This PR fixes the issue by always inferring partial types within a single fine-grained incremental target. This is always enabled in fine-grained incremental mode and can also be turned on in other modes through a new (but hidden) `--local-partial-types` option. This was complicated by various edge cases that are covered by the new test cases and/or documented in comments. If the new option is not enabled, the old behavior should (mostly) be preserved, module some fixes that may affect also the default mode. Work towards python#4492.
2 participants
Previously if partial types were inferred from assignments in multiple
scopes, like in the example below, fine-grained incremental mode could
display bogus messages, since only one of the scopes could be
reprocessed in one incremental update:
This PR fixes the issue by always inferring partial types within a single
fine-grained incremental target. This is always enabled in fine-grained
incremental mode and can also be turned on in other modes through
a new (but hidden)
--local-partial-typesoption.This was complicated by various edge cases that covered by the new
test cases and/or documented in comments.
If the new option is not enabled, the old behavior should (mostly) be
preserved, module some fixes that may affect also the default mode.
Work towards #4492.