Make nil == null, js.Undefined == undefined a bijection. #240

dmitshur · 2015-06-07T21:10:59Z

This PR is a work in progress.

Fixes #237.

Add failing test for nil, undefined equality.
Run gopherjs tests with both GopherJS and Go.

dmitshur · 2015-06-07T21:24:01Z

Ok, good, the tests I've added are indeed failing now, which is expected:

https://circleci.com/gh/gopherjs/gopherjs/453

dmitshur · 2015-06-07T22:49:02Z

Woohoo, everything works! Tests pass, including old tests, and newly added tests.

Now that's the case, next step would be to see if the code can be refactored to be simpler/more direct/better. @neelance, please take a look and review.

dmitshur · 2015-06-07T22:53:42Z

compiler/prelude/prelude.go

@@ -372,7 +372,7 @@ var $internalAppend = function(slice, array, offset, length) {

 var $equal = function(a, b, type) {
  if (type === $jsObjectPtr) {
-    return a === b;
+    return a === b || (a.hasOwnProperty("object") && a.object === undefined && b.hasOwnProperty("object") && b.object === undefined);


It might make sense to factor out this "if both a and b are undefined, return true" check out into $interfaceIsEqual, since that's where "if both a and b are nil interfaces, return true" happens. I wasn't sure which is better.

Also, I am unsure about the way "if both a and b are undefined" is being checked. It seems to work, but maybe there's a much better JavaScript way?

As I've written in the other comments, the *js.Object type holds the JS value itself. So when comparing two undefined, the JS value in a would be undefined and b would also be undefined. So a === b holds.

Unless I change something else elsewhere, a === b did not hold. That's why I needed to add the "or" block. If you remove it, tests will not pass.

Why was this necessary? Which test was failing?

This change is really not the correct way. If something fails, then we'll have to look for the bug somewhere else. ;-)

Have you reviewed #237? This PR is to fix the issue described there, and it uses the solution I proposed. I should make sure you agree with that proposal first, before we discuss its implementation details.

Why was this necessary? Which test was failing?

The test for #237 that I added.

Namely, calling js.Global.Call("eval", "(foo(null))")) would print expected output, but calling js.Global.Call("eval", "(foo(undefined))") would print:

x: undefined x == nil: false x == js.Undefined: false

Instead of:

x: undefined x == nil: false x == js.Undefined: true

In the x == js.Undefined case, a and b on this line should end up being the JS value undefined. But this does not happen. The issue is in $interfaceIsEqual, which does not correctly unwrap the JS value. It already shows with this simple test:

var i interface{} = js.Undefined println(i == js.Undefined)

So $interfaceIsEqual needs to be fixed. I think it does not even need to call $equal in this case, because this just boils down to a === b which can be inlined.

That sounds like the way to go. How should I unwrap the undefined value in $interfaceIsEqual?

Just get the object field. Check for the type first, then you don't need hasOwnProperty.

neelance · 2015-06-07T23:43:02Z

The tests in github.com/gopherjs/gopherjs/tests only use Go features, but not the js package. That's why they can be run by both, normal Go and GopherJS. Tests for the js package are at https://github.com/gopherjs/gopherjs/blob/master/js/js_test.go and they are guarded by // +build js because the js package is not intended to work with pure Go (it could provide features to create mocks for testing, but that functionality is not implemented yet).

I am grateful that you wanted to tackle this bug, but it seems to me like you are not aware about how *js.Object works internally (and it is not really documented, sorry). The basic idea is that whenever something has the type *js.Object (with pointer), then it will directly contain a native JS object at runtime. This is why a === b works if the type which is being checked for equality is *js.Object. No need to change that. But this only applies to *js.Object. For js.Object (without pointer) and interfaces, the normal js.Object Go struct is used, which is recursive: It has a *js.Object field which, according to the rule above, can hold the JS object. Voila! For example take a look at https://github.com/gopherjs/gopherjs/blob/master/compiler/expressions.go#L1092

dmitshur · 2015-06-08T00:16:22Z

I am grateful that you wanted to tackle this bug, but it seems to me like you are not aware about how *js.Object works internally (and it is not really documented, sorry).

No problem, it's true that I am poorly familiar with internal details of GopherJS compiler, in fact this was my first time trying to make a PR for it, so I expected that even if I got it to work and pass tests, there's still a high chance my entire approach is wrong or there's a simpler, better, more direct way to accomplish it.

In addition to try to help fix this issue, it was meant to be a learning experience so I could make better contributions in the future, which is why I appreciate the explanations.

the js package is not intended to work with pure Go

Is this documented somewhere? I thought the js package did not provide functionality beyond the current mocks, but I didn't realize it was completely not intended to be used by normal Go compiler. If that's the case, what are mocks for? Are they just for docs?

Note that I initially wrote the if guard like this:

if runtime.GOARCH == "js" { ... }

But then changed it. Anyway, I thought importing js package was okay to do without hiding it behind js build tag.

neelance · 2015-06-08T00:43:13Z

The mocks are there for type checking, go/types needs something to work on. It is also quite good that you can compile it with pure Go without any errors. Just when using it at runtime, don't expect any meaningful thing to happen. That's why I'd say that printing js.Undefined right in pure Go is not a requirement. All other stuff of the js package doesn't work either with pure Go.

neelance · 2015-06-08T00:44:31Z

In short: The behavior of the js package with pure Go is currently not defined at all.

dmitshur · 2015-06-08T00:48:55Z

Ok. How would you like to proceed? Do you want to give me actionable feedback to change this PR so it can be accepted, or do you want to do this yourself?

neelance · 2015-06-08T00:54:01Z

I would love to help you. I'll post some comments on the code.

neelance · 2015-06-08T00:54:56Z

circle.yml

@@ -4,7 +4,7 @@ machine:

 dependencies:
  pre:
-    - cd /usr/local && sudo rm -rf go && curl https://storage.googleapis.com/golang/go1.4.1.linux-amd64.tar.gz | sudo tar -xz && sudo chmod a+w go/src/path/filepath
+    - cd /usr/local && sudo rm -rf go && curl https://storage.googleapis.com/golang/go1.4.2.linux-amd64.tar.gz | sudo tar -xz && sudo chmod a+w go/src/path/filepath


Separate commit pls.

Done in a55786e.

dmitshur · 2015-06-08T02:28:22Z

I am grateful that you wanted to tackle this bug, but it seems to me like you are not aware about how *js.Object works internally (and it is not really documented, sorry).

Just to prove how little I knew when I began working on this PR, you can see my WIP commit here from when I just got it to work, before I cleaned up all the debugging/test code. :P

Catch and handle the case of both values being js.Undefined in $interfaceIsEqual. Place this check underneath the check for (a.constructor !== b.constructor) as an optimization. Related to #237.

Fixes #237.

dmitshur · 2015-06-08T03:51:12Z

I've addressed all your feedback, PTAL.

I've moved the Go 1.4.1 -> 1.4.2 update into a separate commit.

I've removed the github.com/gohperjs/gopherjs/js tests from github.com/gohperjs/gopherjs/tests package, so it's no longer testing it via Go compiler. Instead, it's only in github.com/gohperjs/gopherjs/js package tests (in js_test.go).

I've broken down the verbose tests into minimal previously-failing tests.

There are now two compiler commits:

One fixing interface equality for undefined (js.Undefined), see cb553f3. It comes with a test for it.
Another fixing missing internalize case for undefined value, see bf81f38. It comes with a test for it. Together with above, it closes Cannot read property 'constructor' of undefined #237.

All tests are passing. Please review again and let me know how this looks.

Commit cb553f3 fixes the issue for when both *js.Object were undefined. This change makes that fix general and apply when the value of the two *js.Object is anything.

neelance · 2015-06-09T20:22:44Z

LGTM

Make nil == null, js.Undefined == undefined a bijection.

dmitshur mentioned this pull request Jun 7, 2015

Cannot read property 'constructor' of undefined #237

Closed

dmitshur reviewed Jun 7, 2015
View reviewed changes

neelance reviewed Jun 8, 2015
View reviewed changes

dmitshur added 4 commits June 7, 2015 20:41

Circle CI: Update Go to latest stable version 1.4.2.

a55786e

Circle CI: Run gopherjs tests with Go as well.

8839012

Fix interface equality for undefined (js.Undefined).

cb553f3

Catch and handle the case of both values being js.Undefined in $interfaceIsEqual. Place this check underneath the check for (a.constructor !== b.constructor) as an optimization. Related to #237.

Fix internalize case for undefined value.

bf81f38

Fixes #237.

Fix $interfaceIsEqual general handling of two *js.Object.

0bfcd10

Commit cb553f3 fixes the issue for when both *js.Object were undefined. This change makes that fix general and apply when the value of the two *js.Object is anything.

neelance added a commit that referenced this pull request Jun 9, 2015

Merge pull request #240 from shurcooL/nil-undefined-equality

2ce233f

Make nil == null, js.Undefined == undefined a bijection.

neelance merged commit 2ce233f into gopherjs:master Jun 9, 2015

dmitshur deleted the nil-undefined-equality branch June 9, 2015 20:23

dmitshur mentioned this pull request Aug 22, 2015

*js.Error and *js.Object treating null objects differently #275

Open

Make nil == null, js.Undefined == undefined a bijection. #240

Make nil == null, js.Undefined == undefined a bijection. #240

Uh oh!

Conversation

dmitshur commented Jun 7, 2015

Uh oh!

dmitshur commented Jun 7, 2015

Uh oh!

dmitshur commented Jun 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neelance commented Jun 7, 2015

Uh oh!

dmitshur commented Jun 8, 2015

Uh oh!

neelance commented Jun 8, 2015

Uh oh!

neelance commented Jun 8, 2015

Uh oh!

dmitshur commented Jun 8, 2015

Uh oh!

neelance commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Jun 8, 2015

Uh oh!

dmitshur commented Jun 8, 2015

Uh oh!

neelance commented Jun 9, 2015

Uh oh!

Uh oh!