Skip to content

Commit

Permalink
compiler/natives/src/reflect: Don't skip string internalization for t…
Browse files Browse the repository at this point in the history
…ypes.

I don't see a good reason to skip string internalization in reflect
package. It was introduced as part of other changes for Go 1.7 support
in commit f1514b4, but no rationale
was provided.

Skipping internalization is inconsistent and causes issues because
string comparison no longer works as expected.

Specifically, the test TestMethodByNameUnExportedFirst was failing with:

	got , expected ΦExported

The reason it was failing is because the test makes a
typ.MethodByName("ΦExported") call. Once that string is internalized,
it becomes "\xCE\xA6Exported".

As a result, the code in rtype.MethodByName was failing to find the
method by name, because the internalized and non-internalized strings
did not match:

	have: unexported want: ΦExported match: false
	have: ΦExported want: ΦExported match: false

Removing this special behavior seems to not have any adverse effects,
fixes the issue and makes things simpler:

	have: unexported want: ΦExported match: false
	have: ΦExported want: ΦExported match: true
  • Loading branch information
dmitshur committed Dec 31, 2017
1 parent 645ccd9 commit 2f2d5b3
Showing 1 changed file with 7 additions and 13 deletions.
20 changes: 7 additions & 13 deletions compiler/natives/src/reflect/reflect.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func reflectType(typ *js.Object) *rtype {
rt := &rtype{
size: uintptr(typ.Get("size").Int()),
kind: uint8(typ.Get("kind").Int()),
str: newNameOff(newName(internalStr(typ.Get("string")), "", typ.Get("exported").Bool())),
str: newNameOff(newName(typ.Get("string").String(), "", typ.Get("exported").Bool())),
}
js.InternalObject(rt).Set("jsType", typ)
typ.Set("reflectType", js.InternalObject(rt))
Expand All @@ -57,12 +57,12 @@ func reflectType(typ *js.Object) *rtype {
for i := range reflectMethods {
m := methodSet.Index(i)
reflectMethods[i] = method{
name: newNameOff(newName(internalStr(m.Get("name")), "", internalStr(m.Get("pkg")) == "")),
name: newNameOff(newName(m.Get("name").String(), "", m.Get("pkg").String() == "")),
mtyp: newTypeOff(reflectType(m.Get("typ"))),
}
}
ut := &uncommonType{
pkgPath: newNameOff(newName(internalStr(typ.Get("pkg")), "", false)),
pkgPath: newNameOff(newName(typ.Get("pkg").String(), "", false)),
mcount: uint16(methodSet.Length()),
_methods: reflectMethods,
}
Expand Down Expand Up @@ -116,13 +116,13 @@ func reflectType(typ *js.Object) *rtype {
for i := range imethods {
m := methods.Index(i)
imethods[i] = imethod{
name: newNameOff(newName(internalStr(m.Get("name")), "", internalStr(m.Get("pkg")) == "")),
name: newNameOff(newName(m.Get("name").String(), "", m.Get("pkg").String() == "")),
typ: newTypeOff(reflectType(m.Get("typ"))),
}
}
setKindType(rt, &interfaceType{
rtype: *rt,
pkgPath: newName(internalStr(typ.Get("pkg")), "", false),
pkgPath: newName(typ.Get("pkg").String(), "", false),
methods: imethods,
})
case Map:
Expand All @@ -148,14 +148,14 @@ func reflectType(typ *js.Object) *rtype {
offsetAnon |= 1
}
reflectFields[i] = structField{
name: newName(internalStr(f.Get("name")), internalStr(f.Get("tag")), f.Get("exported").Bool()),
name: newName(f.Get("name").String(), f.Get("tag").String(), f.Get("exported").Bool()),
typ: reflectType(f.Get("typ")),
offsetAnon: offsetAnon,
}
}
setKindType(rt, &structType{
rtype: *rt,
pkgPath: newName(internalStr(typ.Get("pkgPath")), "", false),
pkgPath: newName(typ.Get("pkgPath").String(), "", false),
fields: reflectFields,
})
}
Expand Down Expand Up @@ -259,12 +259,6 @@ func newTypeOff(t *rtype) typeOff {
return typeOff(i)
}

func internalStr(strObj *js.Object) string {
var c struct{ str string }
js.InternalObject(c).Set("str", strObj) // get string without internalizing
return c.str
}

func isWrapped(typ Type) bool {
return jsType(typ).Get("wrapped").Bool()
}
Expand Down

0 comments on commit 2f2d5b3

Please sign in to comment.