Skip to content
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

x/tools/imports: doesn't add import for ast.Expr #34199

Closed
muirrn opened this issue Sep 10, 2019 · 6 comments

Comments

@muirrn
Copy link

commented Sep 10, 2019

What version of Go are you using (go version)?

$ go version
go version go1.13 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/muir/Library/Caches/go-build"
GOENV="/Users/muir/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/muir/projects//go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/muir/projects/tools/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/96/wknz1jg92yl623qgplk6qw2h0000gn/T/go-build439747941=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I ran organize imports on below file:

package main
var _ ast.Expr

What did you expect to see?

I expected it to add an import for "go/ast".

What did you see instead?

No import added. goimports works, and mysteriously organize imports works if you change it to ast.Node.

@gopherbot gopherbot added this to the Unreleased milestone Sep 10, 2019

@gopherbot gopherbot added the gopls label Sep 10, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Sep 10, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting section of the gopls Wiki page, and make sure that you have provided all of the relevant information here.

@heschik heschik changed the title x/tools/gopls: organize imports doesn't add import for ast.Expr x/tools/imports: doesn't add import for ast.Expr Sep 10, 2019

@heschik

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Not (just) a gopls bug, renamed.

@heschik heschik self-assigned this Sep 10, 2019

@heschik

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Huh.

$ grep 'bytes, type Buffer' go/api/go1.txt
pkg bytes, type Buffer struct
$ grep 'go/ast, type Expr' go/api/go1.txt
pkg go/ast, type Expr interface, End() token.Pos
pkg go/ast, type Expr interface, Pos() token.Pos
pkg go/ast, type Expr interface, unexported methods
...

We take the API files and turn them into zstdlib.go, and use that for the stdlib. So, two bugs: the built in exports list for go/ast is missing Expr, and goimports doesn't look at GOROOT for some reason.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 10, 2019

Change https://golang.org/cl/194568 mentions this issue: internal/imports: add all interfaces in mkstdlib

@dmitshur

This comment was marked as outdated.

Copy link
Member

commented Sep 10, 2019

Edit: Disregard this comment, I misread @heschik's comment above. I thought it was saying an entry for ast.Expr was missing from GOROOT/api/go1.txt, but it was talking about x/tools/internal/imports/zstdlib.go where it was in fact missing.

the built in exports list for go/ast is missing Expr

We've determined it's not missing, it just looks different. This line is the entry for the ast.Expr interface in GOROOT/api/go1.txt:

pkg go/ast, type Expr interface, unexported methods

The line looks a little unusual because of the ", unexported methods" suffix.

@dmitshur dmitshur added the NeedsFix label Sep 10, 2019

gopherbot pushed a commit to golang/tools that referenced this issue Sep 10, 2019
internal/imports: add all interfaces in mkstdlib
In api/*.txt, interface declarations are represented with lines like:
  pkg container/heap, type Interface interface { Len, Less, Pop, Push, Swap }
or, when they have no exported methods:
  pkg go/ast, type Expr interface, unexported methods

The latter form confuses mkstdlib into thinking that it's a method
because of the extra comma, and then it skips the interface entirely.
Running this program is a matter of seconds once per release, so rather
than trying to fix the optimization, just remove it. The parsing logic
doesn't care about the extra lines.

And the corresponding change to the copy in lsp/testdata/unimported.

Updates golang/go#34199

Change-Id: Ic34b8a47537608401e4ef6683617d797f9f50f8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194568
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Sep 10, 2019

Change https://golang.org/cl/194570 mentions this issue: internal/imports: fix scanning GOROOT in module mode

eduvim added a commit to eduvim/tools that referenced this issue Sep 11, 2019
internal/imports: add all interfaces in mkstdlib
In api/*.txt, interface declarations are represented with lines like:
  pkg container/heap, type Interface interface { Len, Less, Pop, Push, Swap }
or, when they have no exported methods:
  pkg go/ast, type Expr interface, unexported methods

The latter form confuses mkstdlib into thinking that it's a method
because of the extra comma, and then it skips the interface entirely.
Running this program is a matter of seconds once per release, so rather
than trying to fix the optimization, just remove it. The parsing logic
doesn't care about the extra lines.

And the corresponding change to the copy in lsp/testdata/unimported.

Updates golang/go#34199

Change-Id: Ic34b8a47537608401e4ef6683617d797f9f50f8a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194568
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
eduvim added a commit to eduvim/tools that referenced this issue Sep 11, 2019
internal/imports: fix scanning GOROOT in module mode
Go 1.13 introduced a module in GOROOT/src. That triggered goimports to
think that it was an invalid module, usable only as a replace target,
because its declared module path "std" didn't match its apparent path
"src". The stdlib is always in scope, so skip the needs-replace check
for GOROOT.

Fixes golang/go#34199

Change-Id: I1199378b940cfedc04e9a4e943c46b9ffdf18446
Reviewed-on: https://go-review.googlesource.com/c/tools/+/194570
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.