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/go/packages: panic when NeedImports is used without NeedDeps mode #33077

Closed
fatih opened this issue Jul 12, 2019 · 1 comment

Comments

@fatih
Copy link
Member

commented Jul 12, 2019

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

$ go version
go version go1.12.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes. This is from HEAD 9a621aea19f8341c01da59e0d42dd97700f677d0 of the tools repo (https://github.com/golang/tools)

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

go env Output
$ go env
GOARCH="amd64"
GOBIN="/Users/fatih/go/bin"
GOCACHE="/Users/fatih/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/fatih/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/k_/87syx3r50m93m72hvqj2qqlw0000gn/T/go-build799516690=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I run the following code:

package main

import (
	"flag"
	"fmt"
	"os"

	"golang.org/x/tools/go/packages"
)

func main() {
	flag.Parse()

	// Many tools pass their command-line arguments (after any flags)
	// uninterpreted to packages.Load so that it can interpret them
	// according to the conventions of the underlying build system.
	cfg := &packages.Config{
		Mode: packages.NeedFiles |
			packages.NeedImports |
			packages.NeedTypes |
			packages.NeedTypesInfo |
			packages.NeedSyntax,
	}
	pkgs, err := packages.Load(cfg, flag.Args()...)
	if err != nil {
		fmt.Fprintf(os.Stderr, "load: %v\n", err)
		os.Exit(1)
	}
	if packages.PrintErrors(pkgs) > 0 {
		os.Exit(1)
	}

	// Print the names of the source files
	// for each package listed on the command line.
	for _, pkg := range pkgs {
		fmt.Println(pkg.ID, pkg.GoFiles)
	}
}

with the following arg:

$ demo.go github.com/fatih/color
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1209c3e]

goroutine 7 [running]:
golang.org/x/tools/go/packages.(*loader).loadRecursive(0xc00010e0b0, 0x0)
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:661 +0x4e
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0xc00010e0b0, 0xc00001ca80, 0x0)
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:668 +0x35
created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:667 +0x13f
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1209c3e]

goroutine 12 [running]:
golang.org/x/tools/go/packages.(*loader).loadRecursive(0xc00010e0b0, 0x0)
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:661 +0x4e
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0xc00010e0b0, 0xc00001ca80, 0x0)
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:668 +0x35
created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1
        /Users/fatih/go/src/golang.org/x/tools/go/packages/packages.go:667 +0x13f
exit status 2

What did you expect to see?

The code should not panic when NeedDeps is not included the config.

What did you see instead?

If NeedDeps is included, it doesn't panic anymore:

cfg := &packages.Config{
	Mode: packages.NeedFiles |
		packages.NeedImports |
		packages.NeedDeps |
		packages.NeedTypes |
		packages.NeedTypesInfo |
		packages.NeedSyntax,
}
$demo.go github.com/fatih/color
github.com/fatih/color [/Users/fatih/go/src/github.com/fatih/color/color.go /Users/fatih/go/src/github.com/fatih/color/doc.go]

@gopherbot gopherbot added this to the Unreleased milestone Jul 12, 2019

@fatih fatih changed the title x/tools/go/packages: panic when NeedImports without NeedDeps mode x/tools/go/packages: panic when NeedImports is used without NeedDeps mode Jul 12, 2019

jirfag added a commit to golangci/tools that referenced this issue Jul 16, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes #golang/go#33077
jirfag added a commit to golangci/tools that referenced this issue Jul 16, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077
@gopherbot

This comment has been minimized.

Copy link

commented Jul 16, 2019

Change https://golang.org/cl/186337 mentions this issue: go/packages: allow types loading without NeedDeps

jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, golang/go#31930
jirfag added a commit to golangci/tools that referenced this issue Sep 9, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
jirfag added a commit to golangci/tools that referenced this issue Sep 10, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
      fixes golang/go#31699, fixes golang/go#31930
clintjedwards added a commit to clintjedwards/tools that referenced this issue Sep 19, 2019
go/packages: allow types loading without NeedDeps
Before separating Load* into Need* we could use LoadSyntax
to get types information by loading inital packages
from source code and loading it's direct dependencies from export data.
It was broken when separation was introduced and before this commit
everything was loading from source code what resulted into slow
loading times. This commit fixes it.

Also, do backwards-incompatible fix of definition of deprecated
LoadImports and LoadAllSyntax.

Improve an internal error message
"internal error: nil Pkg importing x from y": replace it with
"internal error: package x without types was imported from y".

Remove packages.NeedDeps request for loading in tests
TestLoadTypesBits and TestContainsOverlayXTest.

Fixes golang/go#31752, fixes golang/go#33077, fixes golang/go#32814,
          fixes golang/go#31699, fixes golang/go#31930

Change-Id: I416e3c1035d555d67039e45a4fdd1deb9b2184ef
GitHub-Last-Rev: 2e3a46e
GitHub-Pull-Request: golang#139
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186337
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.