-
Notifications
You must be signed in to change notification settings - Fork 570
compiler: fix incorrect emitted Javascript for type switch #683
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
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 looks reasonable based on me seeing things like this elsewhere:
if typesutil.IsJsObject(exprType) {
return c.formatExpr("%e.object", e.X)
}
But I'm not familiar with this to be confident.
compiler/statements.go
Outdated
@@ -126,6 +126,9 @@ func (c *funcContext) translateStmt(stmt ast.Stmt, label *types.Label) { | |||
if _, isInterface := implicit.Type().Underlying().(*types.Interface); !isInterface { | |||
value += ".$val" | |||
} | |||
if typesutil.IsJsObject(implicit.Type().Underlying()) { | |||
value += ".object" | |||
} |
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.
Would everything still work if the if statement is moved into the one above?
In other words, can ".object" be added without ".$val" in front of it?
Asking to learn.
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.
Would everything still work if the if statement is moved into the one above?
If you mean:
if _, isInterface := implicit.Type().Underlying().(*types.Interface); !isInterface {
value += ".$val"
if typesutil.IsJsObject(implicit.Type().Underlying()) {
value += ".object"
}
}
then yes, that's logically equivalent (because something cannot have an underlying type that is both an interface and *js.Object
).
In other words, can ".object" be added without ".$val" in front of it?
This is more to do with the structure of the value being type asserted (in Javascript world). Where a Javascript variable, say x
, holds a value of an interface type, x.$val
will give us the underlying value, regardless of type. Where the underlying value is a struct val or a pointer to a struct val, then it so happens that x.$val === x
, hence x.$val.object === x.object
.
So yes, we could elide the .$val
in the case where the type in the switch case is *js.Object
(because we know the structure of js.Object
) but to my mind it's clearer and more consistent (with the rest of the code base) to keep things accessing via .$val
.
Asking to learn.
I've still got lots to learn about this code base: I dip in and out of it as required 😄
... and it's quite possible some of my understanding is wrong let alone incomplete!
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.
If you mean:
if _, isInterface := implicit.Type().Underlying().(*types.Interface); !isInterface { value += ".$val" if typesutil.IsJsObject(implicit.Type().Underlying()) { value += ".object" } }
Yes, that's what I meant. I wanted to know if something would break/change if the code were written that way.
then yes, that's logically equivalent (because something cannot have an underlying type that is both an interface and
*js.Object
).
Okay, I see.
To confirm my understanding is correct, let me try to answer my question:
In other words, can ".object" be added without ".$val" in front of it?
No, ".object" cannot be added without ".$val" also being added.
Only these 3 scenarios are possible:
implicit.Type().Underlying()
type is an interface - thenvalue
stays as isimplicit.Type().Underlying()
type is not an interface, but it's not*js.Object
either - thenvalue
gets ".$val" appendedimplicit.Type().Underlying()
type is not an interface, it is*js.Object
- thenvalue
gets ".$val.object" appended
No other possibility exists.
If my understanding is indeed correct, then wouldn't the code become simpler (to understand and reason about) if you do move the if statement inside?
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.
Actually, I think the readability/clarity of can happen would be improved even more if you made it an if/else if statement with these 2 cases:
if typesutil.IsJsObject(implicit.Type().Underlying()) {
value += ".$val.object"
} else if _, ok := implicit.Type().Underlying().(*types.Interface); !ok {
value += ".$val"
}
(I also renamed isInterface
to just ok
because I think the long variable name doesn't actually improve readability here; but this is orthogonal and optional.)
What do you think? Is that better, worse, or the same in your opinion?
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 easy either way. I don't think it's particularly unclear one way or the other. I've made the change, rebased this branch and pushed.
The test you added isn't passing in CI. Was it passing for you locally? |
Hmm yes things were passing locally. Will take a look tomorrow and reply
properly then. Thanks for looking at this so promptly.
…On 22 Aug 2017 9:28 pm, "Dmitri Shuralyov" ***@***.***> wrote:
The test you added isn't passing in CI. Was it passing for you locally?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#683 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADN97gwG4FseGT6b5l2mqH_xrfXjkq_vks5sazoBgaJpZM4O_IIZ>
.
|
Worked out what I did wrong; added the test to the wrong package. Tests that import the |
Thanks for tackling this one! |
Got it. What was the package you added it to originally? (A question I wouldn't have to ask if GitHub preserved history of a PR when its branch is force-pushed to.) |
|
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.
Nice!
Sorry for the delay. I wanted to follow up to your comment, see #683 (comment). |
@myitcv If you're in agreement with my suggestion at #683 (comment), I can apply it, and merge this PR (unless you prefer to do it yourself). Let me know when you have a chance to look it over. |
This brings the code emitted for a type switch with a *js.Object case in line with the runtime $assertType function. In the case of a *js.Object value, we have to dereference via .$val.object. Fixes gopherjs#682
@shurcooL see #683 (comment) - pushed up a new rebased commit. Over to you! |
Use the more common got, want naming scheme. Use "JS" rather than "Js" in test name. Reference: https://golang.org/s/style#initialisms.
There's no reason for these to be fatal. We can continue running the test further if any one of them fails. That way, if there are multiple failures, it's easier to see them all at once. Separate var x from the switch by a blank line, since it's used by both switch and the if statement below. Having it without a blank line makes it seem that it's only needed for the switch.
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've pushed some minor improvements to the test.
LGTM.
Thanks a lot for contributing and fixing this bug, @myitcv!
This brings the code emitted for a type switch with a
*js.Object
case in line with the runtime
$assertType
function. In the case ofa
*js.Object
value, we have to dereference via.$val.object
.Fixes #682