make magic methods work with casts too #20654
Conversation
| v = cast_v; | ||
| } | ||
| Value* out; | ||
| try { |
There was a problem hiding this comment.
Why is this try-catch necessary? Shouldn't bool_cast be a CastValue and throw the right error?
There was a problem hiding this comment.
It's to report a better error message. Otherwise it would just show prim::Bool schema matching errors. I guess i could move this whole logic to CastValue. EDIT: that wouldn't work with UDT
| def __bool__(self): | ||
| return (1, 2) | ||
|
|
||
| with self.assertRaisesRegex(RuntimeError, "expected a bool expression for condition"): |
There was a problem hiding this comment.
The error here is up a level too high, it should be thrown at the cast level to match Python:
class X(object):
def __bool__(self):
return (1, 2)
x = X()
if x:
print("hi")prints
Traceback (most recent call last):
File "../test.py", line 142, in <module>
if x:
TypeError: __bool__ should return bool, returned tuple
There was a problem hiding this comment.
We don't currently have the infrastructure for that. This could be possible with Interfaces, but is not currently possible. I don't think that should be a blocker for landing this.
There was a problem hiding this comment.
In CastValue you know the type it should be and what the type actually is, can't you compare there?
There was a problem hiding this comment.
That doesn't fit in with MagicMethod sugared val, which abstracts over the input sugared val. Where would that check go?
There was a problem hiding this comment.
MagicMethod calls CastValue, so it'd be fine there, then you could also get rid of the special erroring for emitCond
There was a problem hiding this comment.
MagicMethod also calls BuiltinFunctions - it doesn't just call CastValue. it's an abstraction
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@eellison is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
5 participants
Previous implementation of magic methods extended from BuiltinOperators, but it should be able to work with other sugared values, such as casts.
I was also considering making CastValue's and BuiltinOperators's extend from a MagicMethod super class, and having them try to call into the super's before their own call. However, not all Builtin Operators have corresponding magic methods so i did it this way instead (although there are workarounds for that).