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
Jump to file or symbol
Failed to load files and symbols.
+28 −1
Diff settings

Always

Just for now

Next

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
  • Loading branch information...
myitcv committed Aug 22, 2017
commit 5fc26c036a81bd4a44b42b68fd6ed27a0dadc71c
View
@@ -123,7 +123,9 @@ func (c *funcContext) translateStmt(stmt ast.Stmt, label *types.Label) {
var bodyPrefix []ast.Stmt
if implicit := c.p.Implicits[clause]; implicit != nil {
value := refVar
if _, isInterface := implicit.Type().Underlying().(*types.Interface); !isInterface {
if typesutil.IsJsObject(implicit.Type().Underlying()) {
value += ".$val.object"
} else if _, ok := implicit.Type().Underlying().(*types.Interface); !ok {
value += ".$val"
}

This comment has been minimized.

@dmitshur

dmitshur Aug 22, 2017

Member

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.

@dmitshur

dmitshur Aug 22, 2017

Member

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.

This comment has been minimized.

@myitcv

myitcv Aug 23, 2017

Contributor

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!

@myitcv

myitcv Aug 23, 2017

Contributor

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!

This comment has been minimized.

@dmitshur

dmitshur Aug 31, 2017

Member

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:

  1. implicit.Type().Underlying() type is an interface - then value stays as is
  2. implicit.Type().Underlying() type is not an interface, but it's not *js.Object either - then value gets ".$val" appended
  3. implicit.Type().Underlying() type is not an interface, it is *js.Object - then value 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?

@dmitshur

dmitshur Aug 31, 2017

Member

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:

  1. implicit.Type().Underlying() type is an interface - then value stays as is
  2. implicit.Type().Underlying() type is not an interface, but it's not *js.Object either - then value gets ".$val" appended
  3. implicit.Type().Underlying() type is not an interface, it is *js.Object - then value 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?

This comment has been minimized.

@dmitshur

dmitshur Aug 31, 2017

Member

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?

@dmitshur

dmitshur Aug 31, 2017

Member

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?

This comment has been minimized.

@myitcv

myitcv Sep 6, 2017

Contributor

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.

@myitcv

myitcv Sep 6, 2017

Contributor

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.

bodyPrefix = []ast.Stmt{&ast.AssignStmt{
View
@@ -571,3 +571,28 @@ func TestUint8Array(t *testing.T) {
t.Errorf("Non-empty byte array is not externalized as a Uint8Array")
}
}
func TestTypeSwitchJsObject(t *testing.T) {
obj := js.Global.Get("Object").New()
obj.Set("foo", "bar")
exp := "bar"
if act := obj.Get("foo").String(); act != exp {
t.Fatalf("Direct access to *js.Object field gave %q; expected %q", act, exp)
}
var x interface{} = obj
switch x := x.(type) {
case *js.Object:
if act := x.Get("foo").String(); act != exp {
t.Fatalf("Value passed through interface and type switch gave %q; expected %q", act, exp)
}
}
if y, ok := x.(*js.Object); ok {
if act := y.Get("foo").String(); act != exp {
t.Fatalf("Value passed through interface and type assert gave %q; expected %q", act, exp)
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.