Skip to content
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

syscall/js: comparing two number objects with == fails #25802

Closed
hajimehoshi opened this issue Jun 9, 2018 · 17 comments
Closed

syscall/js: comparing two number objects with == fails #25802

hajimehoshi opened this issue Jun 9, 2018 · 17 comments
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@hajimehoshi
Copy link
Member

(This issue is copied from https://github.com/neelance/go/issues/27)

I'm using 8d6062b on neelance's wasm-wip branch

package main

import (
        "syscall/js"
)

func main() {
        n := js.Global.Get("Number")
        println(n.New(1) == n.New(1))
}

Shows false, I expected true though. I suggest == return true since it'd be more intuitive.

The current result might be intended, but was really confusing to me. For example, getting WebGL's constant like FRAMEBUFFER_COMPLETE via Get creates a number object if I understand correctly, and comparing this constant value and another value always fails.

// gl is a WebGL context of a canvas.
if s := gl.Call("checkFramebufferStatus", gl.Get("FRAMEBUFFER")); s != gl.Get("FRAMEBUFFER_COMPLETE") {
        // This always comes here, whatever s is!
        return js.Null, errors.New(fmt.Sprintf("opengl: creating framebuffer failed: %d", s.Int()))
}

String object might be in the same situation .

@agnivade agnivade added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-wasm WebAssembly issues labels Jun 9, 2018
@agnivade
Copy link
Contributor

agnivade commented Jun 9, 2018

/cc @neelance

@dmitshur
Copy link
Contributor

dmitshur commented Jun 9, 2018

I’m not completely sure if two distinct js.Value values should report positively for equality, even if their underlying values are equal.

A similar situation I can think of is reflect.Value, where distinct reflect.Value values that represent the same underlying value are not equal:

To compare two Values, compare the results of the Interface method. Using == on two Values does not compare the underlying values they represent.

https://play.golang.org/p/cX0RhxB_800

The original code would work correctly if you compared the underlying values, right? Something like this:

// gl is a WebGL context of a canvas.
if s := gl.Call("checkFramebufferStatus", gl.Get("FRAMEBUFFER")); s.Int() != gl.Get("FRAMEBUFFER_COMPLETE").Int() {
        return js.Null, errors.New(fmt.Sprintf("opengl: creating framebuffer failed: %d", s.Int()))
}

These are some initial thoughts, I’m open to further discussion.

@myitcv
Copy link
Member

myitcv commented Jun 9, 2018

@hajimehoshi

Shows false, I expected true though

Why is this your expectation? Is there some example from the Javascript world where this equates to true (that I'm missing)?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness

@hajimehoshi
Copy link
Member Author

hajimehoshi commented Jun 9, 2018

The original code would work correctly if you compared the underlying values, right? Something like this:

Yes, Int() worked fine. However, I think it'd be the best if I do this without Int() since we cannot expect which API returns numbers as Number objects or just numbers. == sometimes works and sometimes not. I think this is different from reflect.Value situation. EDIT: Apparently Number objects are always created via js functions. This is deterministic :-)

Of course always adding Int() would be a solution. I'm not sure but I'd be fine if there is an explicit rule like what reflect.Value's comment says.

@hajimehoshi
Copy link
Member Author

@myitcv

Oh I was misunderstanding:

$ new Number(0) === new Number(0)
false

Looks like 'false' would be fine in my example...

@hajimehoshi
Copy link
Member Author

hajimehoshi commented Jun 9, 2018

OK another example would be

package main

import (
        "syscall/js"
)

func main() {
        js.Global.Set("a", 1)
        js.Global.Set("b", 1)
        println(js.Global.Get("a") == js.Global.Get("b")) // false
}

Looks like Number objects are always created whenever syscall/js function is used?

@neelance
Copy link
Member

We need to add func Equal(a, b js.Value) bool to the syscall/js package. It is not possible to compare two js.Value with the == operator.

@myitcv
Copy link
Member

myitcv commented Jun 20, 2018

@neelance Would Equal be equivalent to ==? What about ===?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness

@hajimehoshi
Copy link
Member Author

I thought Equal implements ===, not ==. Who wants JavaScript's ==?

@neelance
Copy link
Member

Yes, Equal would be JS' ===, since this is what fits Go's semantics of equality the best. My preference would be to not offer ==, because it is considered bad practice by many.

@myitcv
Copy link
Member

myitcv commented Jun 21, 2018

My preference would be to not offer ==, because it is considered bad practice by many.

Indeed, my point was that it immediately raises the question of which one it refers to. There is also Object.is which is distinct from both == and ===.

Is there any reason this needs to be part of the API at all? Can we not make it incumbent on the developer to call the appropriate JS function?

@neelance
Copy link
Member

Is there any reason this needs to be part of the API at all? Can we not make it incumbent on the developer to call the appropriate JS function?

What exactly do you mean?

@myitcv
Copy link
Member

myitcv commented Jun 21, 2018

I should start by clarifying one thing: it's my understanding that currently a == comparison of two js.Value in the Go world effectively give us the JS semantics of === (in the same way as == in GopherJS transpiles to ===) . It's possible that I misunderstood what @hajimehoshi observed above if that's not the case...

What exactly do you mean?

By doing things like:

js.Global.Get("Object").Call("is", v1, v2).Bool()

(although that said I'm not sure if there are JS function equivalents of == and ===, so maybe this breaks down...unless we resort to using eval).

@neelance
Copy link
Member

Afaik there are no JS function equivalents of == and ===. Also, checking two values for equality is a common case, so it should be part of the core API.

@neelance
Copy link
Member

I just got an initial idea on how to make Go's == work for js.Value. This solution might also give performance benefits in other situations.

@myitcv
Copy link
Member

myitcv commented Jun 22, 2018

Chatted to @neelance about this offline so just adding some points here for the record.

I think what we need to do here is make Go's comparison == work for js.Value's following the semantics of === in Javascript. Per #25802 (comment) @neelance thinks he has a solution for that.

func TestEquals(t *testing.T) {
	a := js.Global.Call("eval", "5")
	b := js.Global.Call("eval", "5")
	cmp := js.Global.Call("eval", "(function(a, b) { return a === b; })")
	if !cmp.Invoke(a, b).Bool() {
		t.Fatalf("Javascript === should be true") // ok
	}
	if a != b {
		t.Fatalf("should compare equal in Go") // currently fails
	}
}

This will, I think, then mean we don't need to add Equal to the API (which is a derivative "win", the main point here being that we "need" Go's == to work for js.Value).

If users want Javascript's == or Object.is semantics they can then use the eval "trick" to do just that.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/120561 mentions this issue: syscall/js: use stable references to JavaScript values

@golang golang locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly issues FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants