Skip to content

Wrong method set order when method names begin with non-ASCII characters. #763

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

Open
dmitshur opened this issue Feb 25, 2018 · 0 comments
Open

Comments

@dmitshur
Copy link
Member

dmitshur commented Feb 25, 2018

The TestIssue22073 test in reflect package is currently not passing without some modifications. This is because the GopherJS doesn't produce correct method set order when method names begin with non-ASCII characters.

Before Go 1.10, methods were sorted by name. That happened to guarantee that exported ASCII methods appear before non-exported ASCII methods, but it broke down when Unicode method names were considered. Go 1.10 changed that in golang/go@6f1724f. GopherJS needs to be updated to match behavior.

Currently, its method set sorting code, implemented in JavaScript in compiler/prelude/types.go, sorts by name:

typ.methodSetCache = [];
Object.keys(base).sort().forEach(function(name) {
  typ.methodSetCache.push(base[name]);
});
return typ.methodSetCache;

For the given type:

type NonExportedFirst int

func (i NonExportedFirst) ΦExported()       {}
func (i NonExportedFirst) nonexported() int { panic("wrong") }

That code puts nonexported before ΦExported (encoded as "\xCE\xA6Exported"). That's not correct.

The following hacky fix, temporarily tested out via c4db7b2 (but eventually reverted in 88dc1af), made the test pass:

typ.methodSetCache = [];
var x = Object.keys(base).sort();
if (x.length === 2 && x[0] === "nonexported" && x[1] === "\xCE\xA6Exported") {
  /* HACK: Hacky fix for TestIssue22073. Need to find a good general fix. */
  x[0] = "\xCE\xA6Exported";
  x[1] = "nonexported";
}
x.forEach(function(name) {
  typ.methodSetCache.push(base[name]);
});
return typ.methodSetCache;

It needs to be generalized and applied to resolve the issue.

dmitshur added a commit that referenced this issue Feb 25, 2018
This reverts commit c4db7b2.

That commit was a temporary hacky fix to help find out where the issue
was. It's not a general fix. Revert it, and file issue #763 to resolve
this issue fully and correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant