-
-
Notifications
You must be signed in to change notification settings - Fork 667
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
fix: crash on assignment chain #2767
Conversation
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.
LGTM
Just fix lint error
let ret = module.block(null, [ | ||
this.makeCallDirect(setterInstance, [ | ||
module.local_tee(tempThis.index, thisExpr, returnType.isManaged), | ||
module.local_tee(tempThis.index, thisExpr, /* isManaged=*/false, thisType.toRef()), // thisType is managed but here it must be alive | ||
valueExpr | ||
], valueExpression), | ||
this.makeCallDirect(getterInstance, [ |
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.
Should we be calling the getter here? I tried this in the Node.js REPL:
> let x = { get y() { console.log("get"); return 123 }, set y(z) { console.log("set", z) } }
undefined
> let foo = x.y = 456
set 456
undefined
> foo
456
It doesn't seem to call the getter, unless I'm missing something...
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.
I do realize you didn't write this, but I wanted to point out what I noticed.
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.
Maybe we can use temp variable to store it?
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.
Yeah, valueExpr
should be tee'd to a temp local in the setter call, and the getter call should be replaced with a local.get to that local, if I'm correct about not calling that getter.
further issue see #2770 |
When creating tee assignment, compiler use incorrect this type (type of return value).
It causes crash when return type is not
usize
.Fixes: #2674