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

compiler: fix incorrect emitted Javascript for type switch #683

Merged
merged 3 commits into from Sep 6, 2017

Conversation

Projects
None yet
4 participants
@myitcv
Contributor

myitcv commented Aug 22, 2017

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 #682

@dmitshur

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.

Show outdated Hide outdated compiler/statements.go
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 22, 2017

Member

The test you added isn't passing in CI. Was it passing for you locally?

Member

dmitshur commented Aug 22, 2017

The test you added isn't passing in CI. Was it passing for you locally?

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 22, 2017

Contributor
Contributor

myitcv commented Aug 22, 2017

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 23, 2017

Contributor

The test you added isn't passing in CI. Was it passing for you locally?

Worked out what I did wrong; added the test to the wrong package. Tests that import the js package can only be run via gopherjs.

Contributor

myitcv commented Aug 23, 2017

The test you added isn't passing in CI. Was it passing for you locally?

Worked out what I did wrong; added the test to the wrong package. Tests that import the js package can only be run via gopherjs.

@flimzy

This comment has been minimized.

Show comment
Hide comment
@flimzy

flimzy Aug 23, 2017

Contributor

Thanks for tackling this one!

Contributor

flimzy commented Aug 23, 2017

Thanks for tackling this one!

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 23, 2017

Member

Worked out what I did wrong; added the test to the wrong package. Tests that import the js package can only be run via gopherjs.

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.)

Member

dmitshur commented Aug 23, 2017

Worked out what I did wrong; added the test to the wrong package. Tests that import the js package can only be run via gopherjs.

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.)

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 23, 2017

Contributor

What was the package you added it to originally?

github.com/gopherjs/gopherjs/tests

Contributor

myitcv commented Aug 23, 2017

What was the package you added it to originally?

github.com/gopherjs/gopherjs/tests

@neelance

Nice!

@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Aug 27, 2017

Contributor

Rebased against master now the Go1.9 changes have been merged.

@shurcooL - anything else you want to look at in this? @neelance has given it 👍

Contributor

myitcv commented Aug 27, 2017

Rebased against master now the Go1.9 changes have been merged.

@shurcooL - anything else you want to look at in this? @neelance has given it 👍

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 31, 2017

Member

Sorry for the delay. I wanted to follow up to your comment, see #683 (comment).

Member

dmitshur commented Aug 31, 2017

Sorry for the delay. I wanted to follow up to your comment, see #683 (comment).

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 6, 2017

Member

@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.

Member

dmitshur commented Sep 6, 2017

@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.

compiler: fix incorrect emitted Javascript for type switch
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 #682
@myitcv

This comment has been minimized.

Show comment
Hide comment
@myitcv

myitcv Sep 6, 2017

Contributor

@shurcooL see #683 (comment) - pushed up a new rebased commit. Over to you!

Contributor

myitcv commented Sep 6, 2017

@shurcooL see #683 (comment) - pushed up a new rebased commit. Over to you!

dmitshur added some commits Sep 6, 2017

js: Use more idiomatic style in test.
Use the more common got, want naming scheme.

Use "JS" rather than "Js" in test name.

Reference: https://golang.org/s/style#initialisms.
js: Make test cases non-fatal.
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.
@dmitshur

I've pushed some minor improvements to the test.

LGTM.

Thanks a lot for contributing and fixing this bug, @myitcv!

@dmitshur dmitshur merged commit b40cd48 into gopherjs:master Sep 6, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment