Make TypeQuery more general, handling nonboolean queries. #3084
Conversation
Instead of TypeQuery always returning a boolean and having the strategy be an enum, the strategy is now a Callable describing how to combine partial results, and the two default strategies are plain old funcitons. To preserve the short-circuiting behavior of the previous code, this PR uses an exception. This is a pure refactor that I am using in my experimentation regarding fixing python#1551. It should result in exactly no change to current behavior. It's separable from the other things I'm experimenting with, so I'm filing it as a separate pull request now. It enables me to rewrite the code that pulls type variables out of types as a TypeQuery. Consider waiting to merge this PR until I have some code that uses it ready for review. Or merge it now, if you think it's a pleasant cleanup instead of an ugly complication. I'm of two minds on that particular question.
mypy/types.py
Outdated
| multiple results. The strategy must be either | ||
| ANY_TYPE_STRATEGY or ALL_TYPES_STRATEGY. | ||
| """ | ||
| class TypeQuery(Generic[T], TypeVisitor[T]): |
There was a problem hiding this comment.
I think you can remove Generic[T] from bases.
mypy/types.py
Outdated
| """True if any type's result is True""" | ||
| if accumulated: | ||
| raise ShortCircuitQuery() | ||
| return current |
There was a problem hiding this comment.
Maybe replace raising exception with returning a tuple of (current, short_circuit), and break in query_types loop when should_break is true? I don't think using exceptions to return a value is bad if it's a relatively less frequent condition, but in this case it will happen very often. Of course, in this case, all strategies would need to support this interface, so if you think it's too much, it's fine.
There was a problem hiding this comment.
Compare StopIteration for the paradigm I am trying to use. I thought about using raise StopIteration literally, but since this is not exactly an iterator I made my own specialized exception.
(In other words, I like it this way, but I would be open to the idea of using StopIteration instead of my custom thing if that seems cleaner)
mypy/types.py
Outdated
|
|
||
| Common use cases involve a boolean query using ANY_TYPE_STRATEGY and a | ||
| default of False or ALL_TYPES_STRATEGY and a default of True. | ||
| """ |
There was a problem hiding this comment.
Maybe get rid of TypeQuery.default, and instead store it inside the strategy? In that case, all methods that currently return self.default would call self.query_types([]) instead. This would make things slightly more streamlined, as everything will be routed through strategy (in particular, a strategy would even be able to change the default for query result in case the list is empty).
|
Ok, @pkch inspired me to make the strategy function just take an iterable instead of two items, simplifying things greatly. No more default; no more defined strategies (use the builtins |
|
(And it would now be useful to me to get this merged, because I'm going to start circling #3113 for a landing, and it depends on this) |
|
Will try to review tomorrow. Or if @ilevkivskyi likes it enough he can merge! |
|
Yee-haw! |
Instead of TypeQuery always returning a boolean and having the strategy be a (fake) enum, the strategy is now a
Callabledescribing how to combine partial results. The strategy takes in an Iterable of the type it's going to return, and returns one of them. ForANY_TYPE_STRATEGY, use the builtinany, forALL_TYPE_STRATEGY, use the builtinall.This is a pure refactor that I am using in my experimentation regarding fixing #1551. It should result in exactly no change to current behavior. It's separable from the other things I'm experimenting with, so I'm filing it as a separate pull request now. It enables me to rewrite the code that pulls type variables out of types as a TypeQuery.
I now think this PR is a pleasant cleanup, and we should merge it 😄