Use go/types in Go 1.5 standard library. #278

dmitshur · 2015-08-25T06:10:07Z

Do not merge, this PR is a work in progress. It's created for initial review and to avoid potential duplicate work.

With the release of Go 1.5, go/types and some related packages have moved from golang.org/x/tools subrepo to standard library. We can start using them. This PR is about making that so.

There are currently 3 commits, first one is clean and ready, the other two are WIP and need some work.

We need to use golang.org/x/tools/go/importer and x/tools/go/types/typeutil, which continue to live in golang.org/x/tools but have not yet been updated to import the new go/types. Instead, they still import the old golang.org/x/tools/go/types. Despite being identical, those packages have different import paths, and as a result the types they define cannot be used interchangeably.

For now, vendor those packages and modify them to use the new go/types package. Hopefully that change will be done upstream soon (see golang/tools@12c48ce for a first similar change) and vendoring/modifying them will becoming unnecessary.

Update: This PR is now updated and needs to be merged soon in order to resolve #373.

Resolves #373.

dmitshur · 2015-08-25T06:36:03Z

This is fully functional and ready for review now. See commit messages for known issues/limitations.

I welcome suggestions for alternate better implementations of "Restore behavior of setting importError in Compile." commit. I was considering trying to return a custom error that implements an interface and checking for that (instead of assigning a local variable). See http://dave.cheney.net/2014/12/24/inspecting-errors for inspiration source (/cc @davecheney). However, I'm not sure it'll work great here.

dmitshur · 2015-08-25T07:40:48Z

CI test keps failing on testing/quick package. It worked the first time, but failed like 5 times after that. Pretty sure it's just a flaky test because my change dealt with handling of import errors.

--- FAIL: TestNilPointers (0.24s)
/tmp/test.265782680:4
return;}$goroutine.exit=true;}catch(err){$goroutine.exit=true;throw err;}final
                                                                    ^
RangeError: Maximum call stack size exceeded
    at $packages.testing/quick.M (/tmp/test.265782680)
    at $packages.testing/quick.M (/tmp/test.265782680:31:16386)
...
    at $packages.testing/quick.M (/tmp/test.265782680:31:19146)
    at $packages.testing/quick.U (/tmp/test.265782680:31:27955)
    at $packages.testing/quick.S (/tmp/test.265782680:31:24286)
    at $packages.testing/quick.CT [as F] (/tmp/test.265782680:31:51355)
    at $packages.testing.BN (/tmp/test.265782680:29:44958)
    at $goroutine (/tmp/test.265782680:4:28817)
    at $runScheduled [as _onTimeout] (/tmp/test.265782680:4:29579)
    at Timer.listOnTimeout (timers.js:119:15)
FAIL    testing/quick   2.424s

Can't reproduce it locally:

$ ./gopherjs test --short --minify testing/quick
PASS
ok      testing/quick   1.544s

Edit: Yep, it's the same test failure as what happened recently on master branch, see https://circleci.com/gh/gopherjs/gopherjs/517.

neelance · 2015-08-25T09:47:06Z

Hopefully I've fixed the stack size issue with 1f89545.

Alan Donovan said here golang/go#11861 (comment) that he wants to switch over the whole x/tools/go stuff in early September. Shall we wait for that?

dmitshur · 2015-08-25T10:11:06Z

Hopefully I've fixed the stack size issue with 1f89545.

Great! Feel free to rebase this branch and omit the last commit then. Otherwise I'll do it next day.

Alan Donovan said here golang/go#11861 (comment) that he wants to switch over the whole x/tools/go stuff in early September. Shall we wait for that?

Sounds good to me. This PR can stay open until then, so hopefully no one else spends time on this.

dmitshur · 2016-01-06T22:22:40Z

https://go-review.googlesource.com/#/c/18207/ has been merged, that means this PR can now be merged pending review. It'll need to be updated first.

dmitshur · 2016-01-07T00:33:20Z

In fact, this needs to be done quickly, because latest version (what you get if you go get -u right now) will not compile successfully on Go 1.5:

$ go get -u github.com/gopherjs/gopherjs
# github.com/gopherjs/gopherjs/compiler
github.com/gopherjs/gopherjs/compiler/utils.go:342: cannot use ty (type "golang.org/x/tools/go/types".Type) as type "go/types".Type in argument to c.p.anonTypeMap.At:
    "golang.org/x/tools/go/types".Type does not implement "go/types".Type (wrong type for Underlying method)
        have Underlying() "golang.org/x/tools/go/types".Type
        want Underlying() "go/types".Type
github.com/gopherjs/gopherjs/compiler/utils.go:348: cannot use ty (type "golang.org/x/tools/go/types".Type) as type "go/types".Type in argument to c.p.anonTypeMap.Set:
    "golang.org/x/tools/go/types".Type does not implement "go/types".Type (wrong type for Underlying method)
        have Underlying() "golang.org/x/tools/go/types".Type
        want Underlying() "go/types".Type

abrander · 2016-01-07T07:25:47Z

For others experiencing this problem a temporary workaround is something along the lines of:

$ cd ${GOPATH}/src/golang.org/x/tools/
$ git checkout 2477c0d5780ddec6439a9322a6862dfe20cc8b81
$ go get github.com/gopherjs/gopherjs

dmitshur · 2016-01-07T07:30:25Z

I'm working on making this PR merge-able right now.

Package golang.org/x/tools/go/types has moved into Go 1.5 standard library as go/types. Package golang.org/x/tools/go/exact is moved/renamed to go/constant.

dmitshur · 2016-01-07T08:06:05Z

Updated the PR.

I was working on removing the vendored/modified packages (e.g., see commit message of 5c4af73), however I learned that of the two packages that are kept in x/tools, only one was updated to use the new "go/types" import paths, but not the other.

So I ended up having to keep vendoring/modifying one of those two packages. See 76e265b. This is needed to be able to compile without errors, until that package is fixed upstream.

I've made a comment about it at https://go-review.googlesource.com/#/c/18207/ and awaiting response.

dmitshur · 2016-01-07T08:34:38Z

After looking into it more, I'm less sure about the status and future of x/tools/go/importer.

According to the current version of https://groups.google.com/forum/#!topic/golang-announce/qu_rAphYdxY, it says:

go/importer     (specific action: 1.5 version already moved to std repo under go/importer)

I'm not sure if something has changed since the last time I worked on it or not. But perhaps it's possible to use go/importer now, while it wasn't before. Or maybe it's still not possible because it doesn't offer complete functionality of x/tools/go/importer.

go/types defines some types just like golang.org/x/tools/go/types, but because they have different import paths, those types are not interchangeable. Not all packages have moved into Go standard library. Namely, golang.org/x/tools/go/importer and golang.org/x/tools/go/types/typeutil are still living in x/tools subrepo. While golang.org/x/tools/go/types/typeutil has been updated to import go/types in https://golang.org/cl/18207, it appears golang.org/x/tools/go/importer has not yet been updated in a similar way. So if we import it as is, we would have incompatible types defined in go/types and golang.org/x/tools/go/types. A temporary solution to have a working codebase is to vendor that package and modify it to import go/types. This can be undone as soon as the upstream package is modified to import go/types itself. I've asked about its status in the aforementioned CL, awaiting a reply in order to figure out the best plan forward.

The type of go/types.Importer has changed from a func to an interface. Make appropriate changes. Maintain behavior of setting importError in Compile. This is needed to maintain the fix for #119.

neelance · 2016-01-07T10:14:02Z

@shurcooL Thanks for digging into this topic and working on this contribution! Very much appreciated.

As far as I can see, go/importer is not yet a suitable replacement, since it has no function similar to https://godoc.org/golang.org/x/tools/go/importer#ExportData.

dmitshur · 2016-01-10T02:47:04Z

As far as I can see, go/importer is not yet a suitable replacement, since it has no function similar to https://godoc.org/golang.org/x/tools/go/importer#ExportData.

I've followed up about that by asking a question at https://go-review.googlesource.com/#/c/18382.

dmitshur · 2016-01-10T04:39:24Z

Robert Griesemer has answered the question, please see:

https://groups.google.com/d/msg/golang-codereviews/t0XvZyHIWz4/zkDrwuzHCgAJ

/cc @neelance I highly recommend you read it since it mentions the future plans for new binary export format and import/export, and prompts for feedback, in case you have some thoughts/suggestions/use cases in mind.

I replied here. Basically, as far as this PR is concerned, there is no follow up action that needs to be done. We will continue to vendor x/tools/go/importer for the time being because there are no plans to provide one upstream that is compatible with new go/types.

The copied importer is an old package from Go 1.5 times. It was a copy of x/tools/go/importer package. It does not support type aliases added in Go 1.9. Encountering a type alias declaration caused a panic. Use the gcimporter15 package instead. It supports a more modern binary export format and includes support for type aliases. The gcimporter15 package is marked as deprecated and said to be deleted in October 2017. However, the replacement package does not expose the functionality suitable for GopherJS current needs. That means we'll need to find a resolution by the time it's deleted. It will either be a feature request to add the needed functionality, a rewrite of GopherJS code to use available functionality, or vendoring/copying the code. Updates #278.

dmitshur force-pushed the go1.5gotypes branch from ee6b626 to 2f43f46 Compare August 25, 2015 06:27

dmitshur force-pushed the go1.5gotypes branch 4 times, most recently from dc8548f to abe91a0 Compare August 25, 2015 07:31

dmitshur force-pushed the go1.5gotypes branch from abe91a0 to cf0b29e Compare August 25, 2015 07:49

dmitshur force-pushed the go1.5gotypes branch from b876a40 to 3179f7e Compare August 27, 2015 04:32

dmitshur mentioned this pull request Sep 18, 2015

Interface implementation errors are not descriptive. #200

Closed

Use go/types in Go 1.5 standard library.

6eda189

Package golang.org/x/tools/go/types has moved into Go 1.5 standard library as go/types. Package golang.org/x/tools/go/exact is moved/renamed to go/constant.

dmitshur force-pushed the go1.5gotypes branch from 3179f7e to 4e13201 Compare January 7, 2016 08:03

dmitshur added 2 commits January 7, 2016 00:49

Updates for minor changes to the go/types API.

a240d35

The type of go/types.Importer has changed from a func to an interface. Make appropriate changes. Maintain behavior of setting importError in Compile. This is needed to maintain the fix for #119.

dmitshur force-pushed the go1.5gotypes branch from 4e13201 to a240d35 Compare January 7, 2016 08:51

neelance merged commit a240d35 into master Jan 7, 2016

neelance deleted the go1.5gotypes branch January 7, 2016 10:14

dmitshur mentioned this pull request Jan 15, 2016

Missing $copy function when building my playground. gopherjs/gopherjs.github.io#38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use go/types in Go 1.5 standard library. #278

Use go/types in Go 1.5 standard library. #278

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

neelance commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Jan 6, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

abrander commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

neelance commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 10, 2016

Uh oh!

dmitshur commented Jan 10, 2016

Uh oh!

Uh oh!

Use go/types in Go 1.5 standard library. #278

Use go/types in Go 1.5 standard library. #278

Uh oh!

Conversation

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

neelance commented Aug 25, 2015

Uh oh!

dmitshur commented Aug 25, 2015

Uh oh!

dmitshur commented Jan 6, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

abrander commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 7, 2016

Uh oh!

neelance commented Jan 7, 2016

Uh oh!

dmitshur commented Jan 10, 2016

Uh oh!

dmitshur commented Jan 10, 2016

Uh oh!

Uh oh!