Go: x/tools/cmd/stringer: can't find packages

[randall77]

I'm having a problem using stringer with go generate. It reports errors because it can't find packages that I think it should be able to find. go build has no trouble finding them.

a.go, in directory $GOPATH/src/a:

package a

import "a/b"

type A int

const (
Aone A = 1
Atwo = 2
Athree = 3
)

//go:generate stringer -type=A

func a() {
b.B()
}

b.go, in directory $GOPATH/src/a/b:

package b

func B() {
}

In $GOPATH/src/a, this happens:

go generate
stringer: checking package: a.go:3:8: could not import a/b (can't find import: a/b)
a.go:13: running "stringer": exit status 1

Running go build instead of go generate works fine.

[minux]

Have you tried "go install a/b" before running stringer?

[randall77]

Ok, that works. Why?

[robpike]

Stringer packages to be installed so it can parse and type check the files it's reading.

[josharian]

Ok, that works. Why?

stringer uses go/types, which uses go/loader to find packages. There are multiple strategies for finding packages available. Stringer uses gcimporter, which parses gc's object files. If the package has not been installed, the object files are not available to parse.

[randall77]

So this should be documented somewhere, perhaps at https://godoc.org/golang.org/x/tools/cmd/stringer
Confused the hell out of me.

[robpike]

It's not a property of stringer _per_ _se_. You can't build or parse (actually type check) a package without building (or parsing) its dependencies first. Any tool that looks at types has the same property.

It's the nature of the work.

[randall77]

So shouldn't stringer build its dependencies if they haven't been built yet? Or at least parse them if it can't find a built object file? Or give me a better error message than "could not import a/b (can't find import: a/b)", like "could not import a/b (it must be built first)"?

[josharian]

@randall77 I agree entirely, but this is a broader problem than just stringer. Filed #10276 for this.

[alandonovan]

For the record:

This CL https://go-review.googlesource.com/#/c/8561/ changes stringer to use go/loader, which loads the entire program from source code, so it gets the correct result no matter what compiler you use or how fresh your .a cache is. The change has been mothballed pending a clean-up of the go/loader API (some of which has already happened).

[szabba]

What's the status on this? Will this not be implemented, or will this be addressed somewhere else (the Go gerrit seems to suggest this was abandoned altogether)?

[oryband]

for the record, i'm still getting can't find import for installed pacakges, even after go installing it. Can we please revive the fix for this issue?

As a side note, I think forcing a go install of the package in order for it to get picked up by gotype is a bad choice. Most imported packages have no executable thus no reason to go install them.

[alandonovan]

Don't hold your breath: it will likely be several months before go/loader is stable enough to have its API frozen, and this is why CL 8561 was rejected.

I agree that using compiler export data is rarely the right solution for analysis tools; source is the ground truth (and provides better diagnostic information such as locations for types and functions). The set of installed packages is usually very stale.

[pdf]

@alandonovan where can I find the blocking go/loader issue(s)? I tried searching this tracker but couldn't find anything open that appears relevant.

[alandonovan]

@pdf: https://github.com/golang/go/issues/14120

[SamWhited]

I know this problem isn't specific to stringer, but I just spent ages trying to track it down before realizing that it's using go/types and things have to be installed; I came here to file an issue about documentation and found this issue. Even though the problem isn't specific to stringer, lots of people will be using stringer that might be confused by this issue, so I still think that it's worth documenting in the mean time, or at least mentioning it in the error message ("Can't find package <whatever>, did you go install it?")

[myitcv]

@alandonovan

Don't hold your breath: it will likely be several months before go/loader is stable enough to have its API frozen, and this is why CL 8561 was rejected.

Is there a reason the API needs to be frozen before any other package under godoc.org/golang.org/x/tools depends on it? Or is this a preference, based on the quite understandable desire to avoid having to refactor N other packages with every changes to loader?

I understand that for go vet loader needs to have moved to the Go repo, but for stringer we could probably make the change now?

[alandonovan]

Since they live in the same repo (x/tools), having stringer depend on go/loader would add little to the maintenance burden, but in https://go-review.googlesource.com/#/c/8561/ Rob said he'd prefer to wait until go/loader is tidied up, by which he likely means the changes described here:
https://docs.google.com/document/d/1IIjEhq54T5KZHTdSyquMhaQ58htYclQpFEX4WRVOhJw
Unfortunately I have not yet found the time to get the API into a state that I would be happy to freeze forever.

[myitcv]

Since they live in the same repo (x/tools), having stringer depend on go/loader would add little to the maintenance burden

Indeed; I more made the comment in the context of...

Unfortunately I have not yet found the time to get the API into a state that I would be happy to freeze forever.

... you having plenty of other things to do!

Thanks for clarifying.

[gopherbot]

CL https://golang.org/cl/40403 mentions this issue.

[robpike]

Reopening because the fixing CL was reverted.

[gopherbot]

Change https://golang.org/cl/121884 mentions this issue: cmd/stringer: revert CL 40403

[benesch]

Reverting that CL just broke any code that assumed stringer was safe to run without running go install -i first. And that was a valid assumption to make for the last twelve months. I realize it was slow (in fact I raised that very issue on the mailing list in May: https://groups.google.com/forum/#!topic/golang-dev/NL1zE4Sesg0), but better slow and correct than fast and subtly broken.

I know that x/tools is not subject to the standard compatibility promise, but we'd lived with slow stringer for a year. Couldn't we have waited a few more months for go/packages to land? Or put the fast-but-broken importer behind a flag, like -importer=gc? It's one thing for code to be broken, but it's another thing entirely to fix it, then intentionally break it again.

And when I say "broken," I understand that it's working as designed. I mean that it has a very hostile design. Running go build -i PKG beforehand isn't even a workable solution, since that often tries to install to an unwriteable GOROOT.

[robpike]

"Reverting that CL just broke any code that assumed stringer was safe to run without running go install -i first. " No it didn't, only some code, while not reverting it broke mine. The situation is unfortunately very messy. I for one have never run go install -i, not once.

It's not just that stringer was slow, it's that it didn't work as it was supposed to. It's meant to _create_ code; requiring the package it writes into to be fully correct beforehand is putting the cart before the horse. The idea behind stringer (other than being a plaything to test out "go generate") was that it would pick up some half-written code with a constant type defined and add a file that helps the package along. But various steps along the way, along with a lack of initial insight, have broken that intention by requiring the package it's in to be valid beforehand.

There is a fundamental conflict going on here. I doubt that go/packages will fix it, since I presume it too will require a complete type-checkable package before stringer will run.

I thought the resolution would be to go back to the beginning and roll out as much as possible of the dependence on types. Evaluation of constants would then require avoiding type checking and adding its own evaluator. I haven't done that yet; one reason is that in general one might have to look at another package to do that, for cases like const A = pkg.C, and doing that requires importing again. Back to square one.

It's possible there is literally no clean way to resolve this.

We could roll back my recent changes, and maybe should do so, but the original problem would then remain unaddressed.

[myitcv]

I doubt that go/packages will fix it, since I presume it too will require a complete type-checkable package before stringer will run.

Can we not simply loosen the requirement that the target fully type checks? e.g.:

https://go-review.googlesource.com/c/tools/+/122377

That way we can leverage the constant evaluation code etc in go/types.

Same approach could be followed with go/packages (which would also solve the "speed" problem as well as imports being current), although the configuration/usage would likely be a bit different.

The above CL allows the following to generate successfully:

package main

//go:generate stringer -type Banana

type Banana uint

const (
        B1 Banana = iota
)

var x Apple

func main() {
}

whereas with the current stringer it fails:

stringer: checking package: main.go:11:7: undeclared name: Apple
main.go:3: running "stringer": exit status 1

[alandonovan]

I doubt that go/packages will fix it, since I presume it too will require a complete type-checkable package before stringer will run.

go/packages is intended to be robust to metadata, syntax, and type errors in any given package and to record them in the returned Package. There are undoubtedly bugs but they should be fixable.

BTW, I measured the speed of go/packages doing the operation needed by the frustratingly slow 4s run of stringer discussed in another issue---I forget the package but it was gopkg.in/something---and it took under 300ms, so it should be fast enough even though it does some type-checking.

[benesch]

"Reverting that CL just broke any code that assumed stringer was safe to run without running go install -i first. " No it didn't, only some code, while not reverting it broke mine.

No, it subtly broke all code and workflows that do not involve running some equivalent of go install -i.

If I have a file in package github.com/benesch/stringertest/b like this

package b

type Garbage int

const (
  FooGarbage Garbage = iota
  BarGarbage
)

I can run stringer -type=Garbage ./b successfully. But as soon as I introduce a dependency on a new package, I run the risk of using outdated type information. Suppose I apply this diff:

diff --git a/b/b.go b/b/b.go
index ebfda0e..3c9bcb7 100644
--- a/b/b.go
+++ b/b/b.go
@@ -1,8 +1,11 @@
 package b

+import "github.com/benesch/stringertest/a"
+
 type Garbage int

 const (
   FooGarbage Garbage = iota
   BarGarbage
+  BazGarbage = a.Sym
 )

Now stringer fails because github.com/benesch/stringertest/a is not installed:

$ stringer -type=Garbage 
stringer: checking package: b.go:3:8: could not import github.com/benesch/stringertest/a (can't find import: "github.com/benesch/stringertest/a")

There are two ways to resolve this. I can run go install on every dependency of b (in this case, just go install ./a), or I can run go install -i ./b and let the Go toolchain sort out the dependency graph. The latter option is the only reasonable option when a package has hundreds of transitive dependencies and any one of them may have changed.

This was not a problem when stringer used the source importer. Here, github.com/benesch/stringertest/a is not installed, and source-importer stringer succeeds:

$ (cd $GOPATH/src/golang.org/x/tools && git checkout ffe8890^ && go install ./cmd/stringer)
$ stringer -type=Garbage 
# success

The situation is unfortunately very messy. I for one have never run go install -i, not once.

Yes, which is why you ran into the "could not import" error when trying to use stringer: https://github.com/golang/go/issues/25650#issuecomment-394513354

I thought the resolution would be to go back to the beginning and roll out as much as possible of the dependence on types. Evaluation of constants would then require avoiding type checking and adding its own evaluator. I haven't done that yet; one reason is that in general one might have to look at another package to do that, for cases like const A = pkg.C, and doing that requires importing again. Back to square one.

There are two orthogonal issues that are being conflated in this discussion. The first issue is how stringer loads a package's dependencies. The second is what sort of type checking stringer performs, if any.

I don't see why the first issue has any bearing on the second. Your change to pass IgnoreFuncBodies to the type checker is equally effective with the source importer as it is with the gc importer. Ditto for the CL @myitcv has proposed to ignore errors from the type-checker.

[robpike]

With great reluctance, I will roll back my changes and pray that Alan Donovan's work will fix this, if only to shut down this conversation.

I do believe there is a deep problem here that only I am worried about, but then if it's only me that worries about it, I guess it doesn't matter much.

[gopherbot]

Change https://golang.org/cl/122535 mentions this issue: cmd/stringer: revert back to source importer

[benesch]

With great reluctance, I will roll back my changes and pray that Alan Donovan's work will fix this, if only to shut down this conversation.

Thanks. It's appreciated.

I do believe there is a deep problem here that only I am worried about, but then if it's only me that worries about it, I guess it doesn't matter much.

I'm also worried about this problem now that you've raised it, so that makes at least two of us. As I've mentioned elsewhere, I think https://go-review.googlesource.com/c/121995/ is a step in the right direction and I hope you'll resubmit it.

[andig]

There are two orthogonal issues that are being conflated in this discussion. The first issue is how stringer loads a package's dependencies. The second is what sort of type checking stringer performs, if any.

I've come here from https://github.com/golang/go/issues/24661 via https://go-review.googlesource.com/c/tools/+/126535 and https://github.com/golang/go/issues/25650#issuecomment-394746629.

Although this issue is closed, the problem still exists in go1.11.5. As the issues above basically go round in circles I'd appreciate your attention.