Make TypeVisitor and SyntheticTypeVisitor (almost) fully abstract #7312
Conversation
This pull request resolves python#730 -- it makes all TypeVisitor and SyntheticTypeVisitor methods abstract. This thankfully required fairly minimal changes. I ended up needing to make only three non-trivial changes: 1. The visitor for PlaceholderType was moved out of TypeVisitor into SyntheticTypeVisitor. My understanding is that PlaceholderType is a semantic analysis only type, so this ought to be safe. 2. As a consequence of 1, I ended up removing the existing PlaceholderType visitor method from TypeTranslator. (It seems that method was added about 6 months ago when PlaceholderType was first introduced, and was untouched since then). 3. TypeIndirectionVisitor was turned from a SyntheticTypeVisitor into a TypeVisitor. I believe we call this visitor only in the final pass, after the type-checking phase.
There was a problem hiding this comment.
Thanks!
Sorry that I invalidated your PR title by just adding TypeAliasType :-) I will update it to reflect the status, anyway I am going to implement all the visitors for this type in a subsequent PR soon.
I have basically two questions:
- A bit suspicious
TypeAnalyzer.visit_overloaded(), but maybe just a comment would be sufficient. ErasedTypeshould behave the same way asPartialType, since it also exists only temporarily during type inference. Does this cause problems in tests/internal code?
mypy/fixup.py
Outdated
| @@ -185,6 +185,9 @@ def visit_overloaded(self, t: Overloaded) -> None: | |||
| for ct in t.items(): | |||
| ct.accept(self) | |||
|
|
|||
| def visit_erased_type(self, o: Any) -> None: | |||
| pass # Nothing to descend into. | |||
There was a problem hiding this comment.
I think ideally this should raise a RuntimeError as visit_partial_type() below, because this type only exists temporarily during type inference.
| @@ -373,6 +373,9 @@ def visit_overloaded(self, t: Overloaded) -> None: | |||
| if t.fallback is not None: | |||
| t.fallback.accept(self) | |||
|
|
|||
| def visit_erased_type(self, t: ErasedType) -> None: | |||
There was a problem hiding this comment.
Same here. Does it actually break some tests when you add a raise here?
| @@ -429,6 +432,10 @@ def visit_union_type(self, typ: UnionType) -> None: | |||
| for item in typ.items: | |||
| item.accept(self) | |||
|
|
|||
| def visit_placeholder_type(self, t: PlaceholderType) -> None: | |||
There was a problem hiding this comment.
I think this also should raise, since this should be called after semantic analysis is done. But maybe I am wrong because there are many visit methods for synthetic types. Or maybe it was mistakenly made SyntheticTypeVisitor instead of just TypeVisitor? Anyway, this is not important for this PR.
There was a problem hiding this comment.
I think it was intentional that this visitor is a SyntheticTypeVisitor -- we apparently try running this visitor on FuncBase.unanalyzed_type in the process_base_func method up above. Not entirely sure why, but I figured it'd be safest to just leave this alone.
| @@ -901,6 +901,9 @@ def visit_overloaded(self, typ: Overloaded) -> List[str]: | |||
| triggers.extend(self.get_type_triggers(item)) | |||
| return triggers | |||
|
|
|||
| def visit_erased_type(self, t: ErasedType) -> List[str]: | |||
| @@ -485,6 +488,9 @@ def visit_callable_type(self, t: CallableType, nested: bool = True) -> Type: | |||
| variables=self.anal_var_defs(variables)) | |||
| return ret | |||
|
|
|||
| def visit_overloaded(self, t: Overloaded) -> Type: | |||
| return t | |||
There was a problem hiding this comment.
Why this is so different from visit_callable_type() above?
| @@ -451,6 +451,9 @@ def visit_none_type(self, t: NoneType) -> Type: | |||
| def visit_uninhabited_type(self, t: UninhabitedType) -> Type: | |||
| return t | |||
|
|
|||
| def visit_erased_type(self, t: ErasedType) -> Type: | |||
There was a problem hiding this comment.
I think this too should raise, see visit_partial_type() below.
ilevkivskyi
changed the title
|
Thanks for the review! I added exceptions/asserts to the |
…thon#7312) This pull request almost resolves python#730 -- it makes all TypeVisitor and SyntheticTypeVisitor methods with the exception of TypeAliasType abstract. This thankfully required fairly minimal changes. I ended up needing to make only three non-trivial changes: 1. The visitor for PlaceholderType was moved out of TypeVisitor into SyntheticTypeVisitor. My understanding is that PlaceholderType is a semantic analysis only type, so this ought to be safe. 2. As a consequence of 1, I ended up removing the existing PlaceholderType visitor method from TypeTranslator. (It seems that method was added about 6 months ago when PlaceholderType was first introduced, and was untouched since then). 3. TypeIndirectionVisitor was turned from a SyntheticTypeVisitor into a TypeVisitor. I believe we call this visitor only in the final pass, after the type-checking phase.
This pull request almost resolves #730 -- it makes all TypeVisitor and SyntheticTypeVisitor methods with the exception of TypeAliasType abstract.
This thankfully required fairly minimal changes. I ended up needing to make only three non-trivial changes:
The visitor for PlaceholderType was moved out of TypeVisitor into SyntheticTypeVisitor. My understanding is that PlaceholderType is a semantic analysis only type, so this ought to be safe.
As a consequence of 1, I ended up removing the existing PlaceholderType visitor method from TypeTranslator. (It seems that method was added about 6 months ago when PlaceholderType was first introduced, and was untouched since then).
TypeIndirectionVisitor was turned from a SyntheticTypeVisitor into a TypeVisitor. I believe we call this visitor only in the final pass, after the type-checking phase.