-
Notifications
You must be signed in to change notification settings - Fork 40
Fix boxing of Nothing / Add explicit type annotation to Val, Let #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
IIUC, both 'A and BoxedInt are correct here ('A is bound) but we
do not compare modulo this, apparently.
| List( | ||
| Definition.Let(orig, origTpe, transform(binding)), | ||
| Definition.Let(id, transform(tpe), coerce(ValueVar(orig, origTpe)))) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to translate one let into two here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the coerce expects a Pure [1] expression but the binding is not.
So we introduce two bindings:
- One for the "original" value
- One for the boxed/unboxed value
AFAICT there is no binding construct for expressions, is there?
Alterantively, we could use something like Run(Let(orig, origTpe, transform(binding), coerce(...))).
[1]: This is mostly to cut down on cases, and since we will use the value as a parameter to Make anyway (so an intermediate value is needed).
| case v @ source.ValDef(id, tpe, binding) if pureOrIO(binding) => | ||
| val transformed = Run(transform(binding)) | ||
| val transformedTpe = v.symbol.tpe match { | ||
| case Some(tpe) => transform(tpe) | ||
| case None => transformed.tpe | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern shows up quite often, how can we simplify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like foo(v.symbol.tpe, transformed.tpe) might be possible, equivalent to a
v.symbol.tpe.map(transform).getOrElse(transformed.tpe).
Though I'm not sure it's worth it.
I am unsure if we can abstract over more easily since the type of binding/transformed is different in most cases (Stmt for val, Expr for let). - This also means that the call to transform (on the binding) is a different one.
|
Sadly, this still doesn't solve the LLVM issue for me on #449 Even when merging this PR here I still get on |
|
@phischu @b-studios |
|
I think the problem is here:
Where now, we also need to check the paramaters and potentially box/unbox those. |
This will: - Potentially incur double coercions for arguments - Probably not work in some cases, as we do not track the return type But this does fix the bugs we are currently running into. Note: In the future, a more complete rewrite is needed, as it was written with parametric polymorphism in mind and is now used for subtyping. This means the assumption that casts *only* need to happen at instantiation sites (i.e. call sites) is broken.
|
@b-studios Now, the |
|
Can you add the new minified example to the hotfix? |
|
This suggests to me that using PolymorphismBoxing to also deal with Nothing and Any was maybe not a good idea. |
|
I merged it into #449 and it indeed resolves the problems with the LLVM backend there. Let's keep boxing on our list and try to clean this up in the future. |
No description provided.