Don't special-case class instances in unary expression inference #15045
Conversation
We have a handy `to_meta_type` that does the right thing for class instances, and also works for all of the other types that are “instances of” something.
dcreager
requested review from
carljm,
MichaReiser,
AlexWaygood and
sharkdp
as code owners
|
| op @ (UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert), | ||
| Type::Instance(InstanceType { class }), | ||
| ) => { | ||
| (op @ (UnaryOp::UAdd | UnaryOp::USub | UnaryOp::Invert), _) => { |
There was a problem hiding this comment.
nit: I know there's a lot of variants in the Type enum, but I'd prefer it if we could explicitly list all the remaining ones in this match arm rather than using _ for the second tuple element. The advantage is that if we add another Type variant in the future that we want to special-case in unary expressions (similar to the special-casing we already do for Type::IntLiteral and Type::BooleanLiteral above), there's no chance of us forgetting to add that special-casing. We'll be forced to explicitly consider whether the new variant should be handled in its own match arm or be handled as part of the catch-all fallback case here.
There was a problem hiding this comment.
I'm not so sure explicitly listing all variants is always the best answer. In this case, the handling in this arm really is a generic implementation of the semantics in terms of other type semantics. It should always be correct for any type (even the ones we special case above here), assuming a correct implementation of calling dunder methods (to_meta_type, member, etc -- all of which do use exhaustive matches and we'll be forced to consider for any new Type variant.) Thus, a special case for a new Type variant here would not be required for correctness, it would just potentially offer more precision. I kind of think it's fine, possibly even good, for that "more precision" to be driven only by actual user needs, and not something we are forced to consider.
But I'm really fine with it either way.
There was a problem hiding this comment.
Yes, you're right that there isn't a correctness issue here. But I would expect more precision here to be both trivial to accomplish for any new variants and simpler for us to compute. The IntLiteral() branches above really involve much less work for us than doing the full lookup for an instance type's dunder method in typeshed or user code (which could involve materialising an MRO!). I don't really see a reason not for us to infer the more precise type whenever possible if it's trivial to do so, and I'd at least like us to be forced to consider whether it would be better to do so
There was a problem hiding this comment.
Sounds like there's a -0 vote from Carl and a +1 from Alex, so I went ahead and expanded the clauses out. We can always put it back to _ in the future if we find this more unwieldy than the benefit.
…5161) Just like in #15045 for unary expressions: In binary expressions, we were only looking for dunder expressions for `Type::Instance` types. We had some special cases for coercing the various `Literal` types into their corresponding `Instance` types before doing the lookup. But we can side-step all of that by using the existing `Type::to_meta_type` and `Type::to_instance` methods.
We have a handy
to_meta_typethat does the right thing for class instances, and also works for all of the other types that are “instances of” something. Unless I'm missing something, this should let us get rid of the catch-all clause in one fell swoop.cf #14548