Skip to content

Go 1.10 support (version bump to GopherJS 1.10-1). #755

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

Merged
merged 38 commits into from
Feb 26, 2018
Merged

Go 1.10 support (version bump to GopherJS 1.10-1). #755

merged 38 commits into from
Feb 26, 2018

Conversation

dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 17, 2018

This PR adds support for Go 1.10, which has been released today.

It is passing CI ✅ on Linux (see here), and all tests are passing on macOS for me locally (aside from an unrelated upstream issue golang/go#23881). It's fully ready for testing and review!

(I have not tested it on Windows and can't guarantee support there.)

I've been testing the go1.10 branch on all of my GopherJS packages during the last few weeks, and there are no known show-stoppers at this time.

The aspirational plan is to merge this sometime next week, after it has a chance to go through review and additional testing, unless major show-stopping issues are discovered and take longer to fix. Minor non-critical issues don't need to be resolved as part of this PR, they can be worked on post-release. But any viable opportunities to improve code can be applied.

Please see issue #725 for context. Each individual commit has a commit message that describes what it's doing.

Resolves #725.

dmitshur and others added 30 commits February 16, 2018 12:51
…ncname.

These are used by the new panicwrap implementation. panicwrap wasn't
implemented previously either, so this is not a regression.

Related to golang/go@354fa9a.

Fixes:

	runtime/error.go:134:8: undeclared name: getcallerpc
	runtime/error.go:135:10: undeclared name: funcname
	runtime/error.go:135:19: undeclared name: findfunc
We can now use the upstream one from package math.

In Go 1.10, it's now implemented directly. Previously, it relied on max
and had assembly implementations, which is why a custom implementation
for GopherJS was needed.

Related to golang/go@b97688d.

Fixes:

	math/math.go:67:9: undeclared name: dim
In Go 1.10, package time has a new LoadLocationFromTZData API addition.
There were some internal code changes, and this change makes appropriate
updates to GopherJS.

Fixes:

	/src/time/time.go:83:9: undeclared name: loadZoneFile
	/usr/local/go/src/time/zoneinfo.go:302:28: undeclared name: zoneSources
It always had the zero value and was unused. It was removed upstream.

Keep name.pkgPath method in order to override the upstream method that
remains with non-trivial implementation (one that would fail).
See https://github.com/golang/go/blob/bfb8f2a765d6097c82b30ae2c19c99fa1082add3/src/reflect/type.go#L515-L529
and https://go-review.googlesource.com/c/go/+/54331/3/src/reflect/type.go#b515.

Related to golang/go@9c9df65 (/cc @mvdan).

Fixes:

	reflect/type.go:1451:45: cannot convert false (untyped bool constant) to string
	reflect/type.go:1451:50: too few arguments in call to newName
	reflect/type.go:1854:45: cannot convert false (untyped bool constant) to string
	reflect/type.go:1854:50: too few arguments in call to newName
	reflect/type.go:1897:45: cannot convert false (untyped bool constant) to string
	reflect/type.go:1897:50: too few arguments in call to newName
	reflect/type.go:2065:47: cannot convert false (untyped bool constant) to string
	reflect/type.go:2065:52: too few arguments in call to newName
	reflect/type.go:2260:44: cannot convert false (untyped bool constant) to string
	reflect/type.go:2260:49: too few arguments in call to newName
	reflect/type.go:2260:49: too many errors
…m uint64 to int.

It has changed upstream.

Related to golang/go@8a6e51a.

Fixes:

	reflect/value.go:2119:31: cannot use buffer (variable of type int) as uint64 value in argument to makechan
syscall.Exit implementation has been removed upstream. In Go 1.10, it's
provided via runtime.exit. We can't do that, so just implement
syscall.Exit ourselves as it was previously implemented.

Related to golang/go@438c8f6.

Fixes:

	Error: runtime error: native function not implemented: syscall.Exit
Copy semaphore implementation from sync package. In Go 1.10,
these are used in FD.destroy and FD.Close methods.

Related to golang/go@382d492.

Fixes:

	Error: runtime error: native function not implemented: internal/poll.runtime_Semrelease
The behavior or reflect.Copy has changed upstream to somewhat mirror
the special case behavior of the copy built-in:

	// As a special case, src can have kind String
	// if the element type of dst is kind Uint8.

Related to golang/go@245e386.

Fixes:

	Error: reflect: call of reflect.Copy on string Value
…ded fields.

Update for upstream changes.

Related to golang/go@6471ace.
Related to golang/go@d349fa2.

Fixes:

	=== RUN   TestCallPanic
	--- FAIL: TestCallPanic (0.01s)

	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/github.com/gopherjs/gopherjs/
	test.043844921:1412
	          throw new Error(msg);
	                ^
	Error: did not panic
…ported methods.

Update overridden method to keep up to date with an upstream change.

Related to golang/go@6f1724f.
At least this helps narrow down where the issue was. Need to come up
with a general, correct fix.

Related to golang/go@6f1724f.
…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
It uses runtime.SetFinalizer, which is not supported by GopherJS.
…verted nil interface.

Not yet sure if this is the right/best fix, needs investigation. But it
allows the TestCallConvert test to pass without breaking any others.

Related to golang/go@eafa29b.

Fixes:

	$ gopherjs test -v --run=TestCallConvert reflect
	=== RUN   TestCallConvert
	--- FAIL: TestCallConvert (0.10s)
		all_test.go:1639: expected [nil], got [<io.Reader Value>]
	FAIL
	FAIL	reflect	0.808s
…lers.

frameSkip relies on runtime.Callers when testing.T.Helper was called.

Fixes:

	$ gopherjs test --run=TestBuilder strings
	--- FAIL: TestBuilder (0.07s)

	github.com/gopherjs/gopherjs/test.481734121:1412
	          throw new Error(msg);
	                ^
	Error: testing: zero callers found
…chitecture.

Can't use package unsafe, so use a simple type conversion from []byte
to string in Builder.String method.

Further optimizations are deferred for later (most likely not needed
because Go->wasm will be ready by the time they're needed).

Related to golang/go#18990.
It's being used in a few places, and previously had no implementation
in GopherJS. (In upstream Go, it's being provided by runtime via
go:linkname directive.)

This restores more helpful error messages as in GopherJS 1.9.

Before:

	runtime error: native function not implemented: sync.throw

After:

	runtime error: sync: inconsistent mutex state
…on for types."

This reverts commit 2f2d5b3.

That commit (and its commit message) was wrong/incomplete. Now I
understand there was a reason for skipping string internalization
for types. The following commit is a better alternative fix.
…gesize.

Fixes:

	--- FAIL: TestIndexByteNearPageBoundary (0.00s)
		Error: runtime error: native function not implemented: syscall.Getpagesize
	FAIL	bytes	19.341s
Since the package files are excluded too, the tests can't compile.

Fixes:

	../../../usr/local/go/src/crypto/rand/rand_linux_test.go:13:17: undeclared name: batched
	../../../usr/local/go/src/crypto/rand/rand_linux_test.go:31:7: undeclared name: batched
	../../../usr/local/go/src/crypto/rand/rand_linux_test.go:38:7: undeclared name: batched
This helps know which parts of the package can be relied on, or not.
It's being done upstream to trick the escape analysis and prevent
allocations. We can't rely on it here.

Fixes:

	=== RUN   TestBuilder
	--- FAIL: TestBuilder (0.01s)

	TypeError: Cannot create property '$proxies' on number '0'
TestMultiWriterSingleChainFlatten was added in Go 1.10, and it needs to
be skipped (just like TestMultiReaderFlatten) because it relies on
unsupported runtime features.

Related to golang/go@060d1a5.

Fixes:

	=== RUN   TestMultiWriterSingleChainFlatten
	--- FAIL: TestMultiWriterSingleChainFlatten (0.01s)
	    test.558082205:38: multiWriter did not flatten chained multiWriters: expected writeDepth 12, got 4
	FAIL    io  0.888s
TestTypeRace was added in Go 1.10, and it needs to be skipped because
it relies on sync.WaitGroup to behave correctly under concurrency.
However, encoding/gob is currently configured to use nosync, which
cannot succeed.

Since this is a new test, opt to skip it for now rather than making it
succeed by replacing nosync with sync.

Related to golang/go@1a9f27d.

Fixes:

	=== RUN   TestTypeRace
	--- FAIL: TestTypeRace (0.00s)
		Error: sync: WaitGroup counter not zero
	FAIL    encoding/gob    3.030s
…itly converted nil interface."

This reverts commit 1fb1aaa.

The change in that commit was not a complete and correct solution.
Instead of resolving the underlying problem, it masked a symptom,
allowing the TestCallConvert test to pass.

A better fix will follow next.
…Kind() == Interface && v.IsNil().

This change adds a copy of Value.assignTo method to reflect package,
identical to upstream, except with golang/go@eafa29b
not applied.

That commit adds a short-circuit check to that skips ifaceE2I, in order
to avoid a panic that occurs in ifaceE2I under that condition in gc and
gccgo compilers.

GopherJS implements a custom version of ifaceE2I which needs to run and
does not panic in that condition. So, there's no need skip it.

Fixes:

	$ gopherjs test --short reflect html/template
	--- FAIL: TestCallConvert (0.01s)
		all_test.go:1639: expected [nil], got [<io.Reader Value>]
	FAIL
	FAIL	reflect	3.084s
	--- FAIL: TestEscapingNilNonemptyInterfaces (0.01s)

	go/src/github.com/shurcooL/play/241/test.243863553:9608
				if (!(($parseInt(methodSet.length) === 0)) || !!(typ.named)) {
	                              ^
	TypeError: Cannot read property 'length' of undefined
	    at reflectType (go/src/github.com/shurcooL/play/241/test.243863553:9608:31)
	    at Object.TypeOf (go/src/github.com/shurcooL/play/241/reflect.go:312:3)
	    at indirect (/html/template/content.go:119:6)
	    at stringify (/html/template/content.go:153:4)
	    at htmlEscaper (/html/template/html.go:43:3)
	    at $call (go/src/github.com/shurcooL/play/241/test.243863553:30:50)
	    at go/src/github.com/shurcooL/play/241/test.243863553:1948:22
	    at Object.$packages.reflect.Value.ptr.call (go/src/github.com/shurcooL/play/241/reflect.go:880:3)
	    at Object.$packages.reflect.Value.ptr.Call (/reflect/value.go:308:3)
	    at Object.$packages.text/template.state.ptr.evalCall (/text/template/exec.go:667:3)
	    at Object.$packages.text/template.state.ptr.evalFunction (/text/template/exec.go:535:3)
	    at Object.$packages.text/template.state.ptr.evalCommand (/text/template/exec.go:432:4)
	    at Object.$packages.text/template.state.ptr.evalPipeline (/text/template/exec.go:405:4)
	    at Object.$packages.text/template.state.ptr.walk (/text/template/exec.go:231:4)
	    at Object.$packages.text/template.state.ptr.walk (/text/template/exec.go:239:5)
	    at Object.$packages.text/template.Template.ptr.execute (/text/template/exec.go:194:3)
	    at Object.$packages.text/template.Template.ptr.Execute (/text/template/exec.go:177:3)
	    at Object.$packages.html/template.Template.ptr.Execute (/html/template/template.go:122:3)
	    at Object.TestEscapingNilNonemptyInterfaces [as $blk] (/html/template/content_test.go:448:3)
	    at Object.tRunner [as $blk] (/testing/testing.go:777:3)
	    at fun (go/src/github.com/shurcooL/play/241/test.243863553:1479:37)
	    at $goroutine (go/src/github.com/shurcooL/play/241/test.243863553:1477:19)
	    at $runScheduled (go/src/github.com/shurcooL/play/241/test.243863553:1517:7)
	    at $schedule (go/src/github.com/shurcooL/play/241/test.243863553:1533:5)
	    at $go (go/src/github.com/shurcooL/play/241/test.243863553:1509:3)
	    at Object.<anonymous> (go/src/github.com/shurcooL/play/241/test.243863553:55363:1)
	    at Object.<anonymous> (go/src/github.com/shurcooL/play/241/test.243863553:55366:4)
	    at Module._compile (module.js:660:30)
	    at Object.Module._extensions..js (module.js:671:10)
	    at Module.load (module.js:573:32)
	    at tryModuleLoad (module.js:513:12)
	    at Function.Module._load (module.js:505:3)
	    at Function.Module.runMain (module.js:701:10)
	    at startup (bootstrap_node.js:190:16)
	    at bootstrap_node.js:662:3
	FAIL	html/template	1.079s
@dmitshur dmitshur mentioned this pull request Feb 17, 2018
@johanbrandhorst
Copy link

Amazing work, as usual @shurcooL. I'll try this out on my projects today and let you know if I run into any problems.

@johanbrandhorst
Copy link

All good here, excited to have this merged when you're happy to do so.

dmitshur and others added 2 commits February 25, 2018 17:52
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.
@dmitshur
Copy link
Member Author

dmitshur commented Feb 25, 2018

This PR has been open for a week, and there've been no code review comments, nor issues reported.

I'm reverting a temporary hacky fix in commit c4db7b2 that was applied for debugging purposes (reverted in commit 88dc1af), and opening #763 to track progress on resolving that issue. It's not a blocker for release: it only affects reflect code that touches methods sets that include exported methods starting with non-ASCII letters, which is rare.

Tomorrow (Monday), I'm going to bump the version from "GopherJS 1.10-wip" to "GopherJS 1.10-1" and merge this. There'll be an announcement blog post and a tweet.

The GopherJS Playground will be updated at the same time.

@myitcv
Copy link
Member

myitcv commented Feb 26, 2018

@shurcooL - apologies, not had much time recently, hence not had a chance to look at the detail of this PR.

For what it's worth I've just tried this out against my React work (links just above this comment) and all looks ok, both for regular files and minified output.

dmitshur added a commit to gopherjs/gopherjs.github.io that referenced this pull request Feb 26, 2018
…d64).

Regenerate with:

	go generate github.com/gopherjs/gopherjs.github.io/playground

Follows gopherjs/gopherjs#755.
@dmitshur dmitshur merged commit 4e9f423 into master Feb 26, 2018
@dmitshur dmitshur deleted the go1.10 branch February 26, 2018 19:28
@dmitshur
Copy link
Member Author

GopherJS 1.10-1 is now released and available on master branch.

Blog post: https://medium.com/gopherjs/gopherjs-1-10-1-is-released-2ff9002a6712.

Tweet: https://twitter.com/GopherJS/status/968207925477965824.

GopherJS Playground has been updated: https://gopherjs.github.io/playground/#/UdyEyAtguS.

Thanks everyone! 🎉 🏄‍♂️

@dmitshur dmitshur mentioned this pull request Mar 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go 1.10 support.
4 participants