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

go/types: go vet reporting incorrect "imported but not used" error #23914

Closed
somersf opened this issue Feb 18, 2018 · 6 comments
Closed

go/types: go vet reporting incorrect "imported but not used" error #23914

somersf opened this issue Feb 18, 2018 · 6 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@somersf
Copy link
Contributor

somersf commented Feb 18, 2018

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

This issue appeared in a canary build/test of part of our Go codebase, using go master HEAD recently.

Does this issue reproduce with the latest release?

This issue is not present in the released go1.10 or go1.9.4 versions.

This regression seems to have appeared in HEAD shortly after go1.11 development opened.

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

GOOS="linux" GOARCH="amd64"
GOOS="darwin" GOARCH="amd64"

What did you do?

Create a package issue containing two seperate files:

a.go:

package issue

import "regexp"

type Y interface {
	X

	Expr() *regexp.Regexp
}

b.go:

package issue

import "regexp"

type X interface {
	Exprs() []*regexp.Regexp
}

To reproduce, use a go HEAD version after approximately Feb 13th, and run:

$ cd issue

$ go vet .
# _/home/user/go-project/issue
./b.go:3:8: "regexp" imported but not used
vet: typecheck failures

Changing the order the files are presented to go vet changes the behavior:

$ cd issue

$ go vet a.go b.go
# command-line-arguments
./b.go:3:8: "regexp" imported but not used
vet: typecheck failures

$ go vet b.go a.go             # <--- This order succeeds
$ echo $?
0

Additional Information

From our testing logs:
go version devel +ce5fa6d5e9 Tue Feb 13 16:00:30 2018 +0000 linux/amd64 has this regression.
go version devel +c52e27e68d Sun Feb 11 20:41:48 2018 +0000 linux/amd64 succeeds.

A git bisect run, checking between HEAD (+7a1347a1a1 on 2018-02-18) and +c52e27e68d indicates that the regression may have been introduced or uncovered as part of commit dd44895 (merged to master Feb 12th).

What did you expect to see?

go vet should succeed on these input files, regardless of the file input order.

What did you see instead?

go vet is erroneously reporting "regexp" imported but not used when the package is being used for a return type.

The behavior also appears to be sensitive to the order in which go vet processes the input files.

@ianlancetaylor ianlancetaylor changed the title go vet reporting incorrect "imported but not used" error go/types: go vet reporting incorrect "imported but not used" error Feb 18, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Feb 18, 2018
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Feb 18, 2018
@rmmh
Copy link
Contributor

rmmh commented Feb 20, 2018

Here's a repro program that bisects to the same commit and demonstrates the order dependence, minimized from Prometheus.

package main

import (
        "fmt"
        "go/ast"
        "go/importer"
        "go/parser"
        "go/token"
        "go/types"
        "log"
)

// passes if analyzed this direction.
var srcs = []string{`
package a
import "io"
type Metric interface {
        Write(*io.Writer) error
}`, `
package a
type Untyped interface {
        Metric
}
`, `
package a
type value struct {
        selfCollector
}`, `
package a
type Collector interface {
}
type selfCollector struct {
        self Metric
}
func (c *selfCollector) init(self Metric) {
        c.self = self
}`}

func typecheck() {
        fset := token.NewFileSet()
        conf := types.Config{
                Importer: importer.For("source", nil),
                Error:    func(err error) { log.Println(err) },
        }
        files := []*ast.File{}
        for i, src := range srcs {
                parsed, err := parser.ParseFile(fset, fmt.Sprintf("f%d.go", i), src, 0)
                if err != nil {
                        log.Fatal(err)
                }
                files = append(files, parsed)
        }
        _, err := conf.Check(".", fset, files, nil)
        if err != nil {
                fmt.Println("bad typecheck!")
        }
}

func main() {
        typecheck()
        fmt.Println("first check succeeded")
        srcs[0], srcs[1], srcs[2] = srcs[1], srcs[2], srcs[0]
        typecheck()
}

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 20, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 20, 2018
@griesemer
Copy link
Contributor

Confirmed. I will look into this shortly; this is clearly a newly introduced issue. Thanks for reporting with concise reproducer.

@griesemer
Copy link
Contributor

The problem is worse. For a.go:

package bug
import "go/ast"
type A interface { B }

and b.go:

package bug
import "go/ast"
type B interface { b() ast.Node }

we get the error

b.go:2:8: "go/ast" imported but not used

in b.go which uses go/ast but not in a.go which doesn't use it.

The problem is that the method set of A is computed by including the methods from B which then all are type-checked while in the file scope of A; hence it appears that go/ast is used by A (while checking the b method). But since the b method is already type-checked, it doesn't need to be checked again in B, leading to the lack of use of go/ast in b.go`.

@griesemer
Copy link
Contributor

In fact, for b.go:

package bug
type B interface { b() ast.Node }

the code type-checks w/o errors...

@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Feb 21, 2018
@gopherbot
Copy link

Change https://golang.org/cl/96375 mentions this issue: go/types: add -panic flag to gotype command for debugging

@gopherbot
Copy link

Change https://golang.org/cl/96376 mentions this issue: go/types: type-check embedded methods in correct scope (regression)

gopherbot pushed a commit that referenced this issue Feb 22, 2018
Setting -panic will cause gotype to panic with the first reported
error, producing a stack trace for debugging.

For #23914.

Change-Id: I40c41cf10aa13d1dd9a099f727ef4201802de13a
Reviewed-on: https://go-review.googlesource.com/96375
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants