Skip to content

Inconsistent type conversion for *js.Object #682

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.

Sign up for GitHub

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

Jump to bottom
Closed
flimzy opened this issue Aug 19, 2017 · 4 comments
Closed

Inconsistent type conversion for *js.Object #682

flimzy opened this issue Aug 19, 2017 · 4 comments
Labels
bug NeedsFix

Comments

@flimzy
Copy link
Member

flimzy commented Aug 19, 2017 •
edited by dmitshur
Loading

I've discovered that doing a type switch on a *js.Object behaves differently than an explicit type assert. Playground link

package main

import (
	"fmt"

	"github.com/gopherjs/gopherjs/js"
)

func main() {
	obj := js.Global.Get("Object").New()
	obj.Set("foo","bar")
	
	fmt.Printf("obj.foo = %s\n", obj.Get("foo").String())
	
	var x interface{} = obj
	switch t := x.(type) {
	case *js.Object:
	    fmt.Printf("t.foo = %s\n", t.Get("foo").String())
	}
	
	if y, ok := x.(*js.Object); ok {
	    fmt.Printf("y.foo = %s\n", y.Get("foo").String())
	}
}

Produces the following output:

obj.foo = bar 
t.foo = undefined 
y.foo = bar

where I would have expected:

obj.foo = bar 
t.foo = bar
y.foo = bar
@flimzy flimzy mentioned this issue Aug 19, 2017
Merged
@dmitshur
Copy link
Member

Thanks for reporting. This looks like a clear bug.

@myitcv
Copy link
Member

myitcv commented Aug 21, 2017

I'll look to try and pick this up as part of #617

@myitcv myitcv mentioned this issue Aug 21, 2017
Open
@dmitshur
Copy link
Member

@myitcv In general, I would make the suggestion that we (all) aim to make PRs as small and self-contained as possible. That approach makes it easiest and fastest to review, merge, and revert if neccessary, spot issues, etc. On the other hand, large PRs are hard to review, slow to merge, hard to revert, etc.

Of course, you should use your judgement each time. If this change is significantly easier to fix as part of a bigger change, then it's fine. I just wanted to mention this point for awareness.

@myitcv
Copy link
Member

myitcv commented Aug 22, 2017

Thanks @shurcooL - completely agree with all your points.

I should have been clearer in my previous comment. I dug into the bug briefly and it looked like there was more overlap with #617 than not, hence my comment to pick it up as part of that issue and PR.

But having just looked further the fix is actually unrelated to #617. Pushing up a proposed fix in a separate PR shortly.

myitcv added a commit to myitcv/gopherjs that referenced this issue Aug 22, 2017
@myitcv
compiler: fix incorrect emitted Javascript for type switch
eb84138 
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
@myitcv myitcv mentioned this issue Aug 22, 2017
Merged
myitcv added a commit to myitcv/gopherjs that referenced this issue Aug 23, 2017
@myitcv
compiler: fix incorrect emitted Javascript for type switch
43d3c42 
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
myitcv added a commit to myitcv/gopherjs that referenced this issue Aug 27, 2017
@myitcv
compiler: fix incorrect emitted Javascript for type switch
32ff48b 
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
myitcv added a commit to myitcv/gopherjs that referenced this issue Sep 6, 2017
@myitcv
compiler: fix incorrect emitted Javascript for type switch
5fc26c0 
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
dmitshur pushed a commit that referenced this issue Sep 6, 2017
@myitcv @dmitshur
compiler: Fix generated code for type switch with *js.Object case. (#683
b40cd48 
)

This brings generated JS code for a type switch with a *js.Object
case in line with the runtime $assertType function. For *js.Object
type, we have to dereference via .$val.object. See
https://github.com/gopherjs/gopherjs/blob/4b94c02883c52ae7f268e602db1dacf7bd0fbd78/compiler/prelude/types.go#L735-L740.

Fixes #682.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug NeedsFix
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dmitshur @myitcv @flimzy