go/build: ImportDir/Import no longer return os not found error for missing dir on local files

[mattfarina]

Please answer these questions before submitting your issue. Thanks!

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

1.9

Does this issue reproduce with the latest release?

Yes. Was not present before latest release.

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mfarina/Code/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.9/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.9/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ml/55r2m1jd38x068q85txj8cvc0000gn/T/go-build428826794=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

I have code that uses ImportDir (from the go/build package) to look at the packages used by another package. If there is an error from ImportDir the automation acts on the error to try and fix the problem.

What did you expect to see?

I expected a missing directory to cause the returned error to be able to be detected by os.IsNotExist. This is how previous version of Go worked.

What did you see instead?

The returned error was in the form of

   fmt.Errorf("cannot find package %q in:\n\t%s", path, p.Dir)

In 1.8.3 (and before), the Import function would fall through to ctxt.readDir that used io.ReadDir...

go/src/go/build/build.go

Lines 171 to 177 in 352996a

	// readDir calls ctxt.ReadDir (if not nil) or else ioutil.ReadDir.
	func (ctxt *Context) readDir(path string) ([]os.FileInfo, error) {
	if f := ctxt.ReadDir; f != nil {
	return f(path)
	}
	return ioutil.ReadDir(path)
	}

In 1.9 The code is a little different for local files. Instead the code stops at...

go/src/go/build/build.go

Lines 687 to 695 in c8aec40

	// If it's a local import path, by the time we get here, we still haven't checked
	// that p.Dir directory exists. This is the right time to do that check.
	// We can't do it earlier, because we want to gather partial information for the
	// non-nil *Package returned when an error occurs.
	// We need to do this before we return early on FindOnly flag.
	if IsLocalImport(path) && !ctxt.isDir(p.Dir) {
	// package was not found
	return p, fmt.Errorf("cannot find package %q in:\n\t%s", path, p.Dir)
	}

You can no longer rely on os.IsNotExist to detect missing package directories.

This change was not documented in the release notes, either.

[ianlancetaylor]

CC @shurcooL

This was changed for #17863, #19769 and #20175.

Frankly I think the error we return now seems more useful for most users.

[dmitshur]

Thanks for the /cc. This is indeed related to the fix for #17863.

I expected a missing directory to cause the returned error to be able to be detected by os.IsNotExist. This is how previous version of Go worked.

Unfortunately, os.IsNotExist is very hard to satisfy with custom errors. Aside from some platform-specific errors in syscall, it only returns true for os.ErrNotExist, whose Error() string method always returns "file does not exist".

The previous behavior of build.Import when an error is encountered was quite inconsistent depending on the mode used. Consider the following 4 test cases from build_test.go:

go/src/go/build/build_test.go

Lines 317 to 320 in bb2f0da

	{"Import(full, 0)", "go/build/doesnotexist", "", 0},
	{"Import(local, 0)", "./doesnotexist", filepath.Join(ctxt.GOROOT, "src/go/build"), 0},
	{"Import(full, FindOnly)", "go/build/doesnotexist", "", FindOnly},
	{"Import(local, FindOnly)", "./doesnotexist", filepath.Join(ctxt.GOROOT, "src/go/build"), FindOnly},

Previous version of Go (1.8) would return an error if a package is not found for the first 3, but not the last one. Out of those 3, only the "Import(local, 0)" case error would be satisfied by os.IsNotExist. The other 2 would not, because they return a detailed message listing the places where the package was looked for.

So, I think this is unfortunate but I don't see any way to make os.IsNotExist satisfied without sacrificing the information returned in the error, and changing 2 other cases would probably break even more programs, and wouldn't be acceptable.

As far as I can tell, this was never a promised documented behavior, just an unintended property of the previously inconsistent handling of local import paths (with build.FindOnly mode on/off). The documentation only makes the promise that the error will be non-nil:

If an error occurs, Import returns a non-nil error and a non-nil *Package containing partial information.

FWIW, godoc used string matching to detect the error in its tests, and after the change, the string matching became simpler, because the error message has consistent format across platforms. See the simplification to godoc_test.go in golang/tools@f595fb5.

[mattfarina]

To add a little context...

1. This change was not documented which made it a little difficult to pick up.
2. Users are also code. In my case, the package manager glide. The code detects when something isn't on the OS and in some cases it goes out and fetches the missing repo (when it's repo level). For automation a change in the error makes a difference. The different types of errors matter.

Since a lot of automation acts on errors (e.g., automated correction), changes in errors are perceived as a change in interface. It's worth documenting them, IMHO.

I've not worked through the current logic in Import. There is an error with a quite similar message elsewhere in the function:

go/src/go/build/build.go

Line 673 in c8aec40

return p, fmt.Errorf("cannot find package %q in any of:\n%s", path, strings.Join(paths, "\n"))

Detecting between the two with simple string functions isn't easy. This error was in past versions of go (e.g., 1.8.3). Yet, using ImportDir it was never triggered so I never worked through the logic to get to it.

Does that case mean it's not on the file system or are there other things that could trigger it? Does this change make things more consistent?

[ianlancetaylor]

I'm sorry that the change broke your code, but I think that overall it is a good one and I don't think we should roll it back.

I'm not really sure whether or how to change the docs. Right now the docs just say that the method "returns a non-nil error." I gather that you want to distinguish different kinds of error cases, and we currently don't support that. Specifically, you want to distinguish "cannot find package" from a different sort of error. Is that correct?

[dmitshur]

I agree with @ianlancetaylor here.

I think you should update your code and treat a non-nil error to mean "a valid Go package does not exist here". You can also check if the error type is one of build.NoGoError or build.MultiplePackageError. That's what I do in my code, and I haven't run into false positives yet. If you want to report more detailed error messages, optionally, you can check that err.Error() has prefix "cannot find package ", which should work at this time (but keep in mind it's not guaranteed to, so it may change in a future point release). Edit: I see you've already done so in Masterminds/glide#910.

This change was not documented which made it a little difficult to pick up.

The list of changes at https://golang.org/doc/go1.9 describes changes to language spec, changes to documented contracts, etc. It cannot list all other changes that are within documented contract, because there may be too many and they may be very subtle and hard to describe. Ideally, your code should not depend on things that are outside the specified contract.

It's a known fact that error strings and other internal details can change between releases. This is neccessary for Go to be able to make forward progress. As such, users should only rely on documented contract (in this case, the only documented promise is that ImportDir returns non-nil error, there are no guarantees beyond that), or be ready and willing to update their code if something changes in a new release of Go that makes error string not match.

I highly recommend seeing this excellent talk by @dsnet on the topic of compatibility between point releases of Go at this year's GopherCon, https://www.youtube.com/watch?v=OuT8YYAOOVI.

[dmitshur]

That said, there may be room for improvement of how build.Import reports different types of errors, but that's an enhancement that's distinct from the Go 1.8 -> 1.9 change regarding os.IsNotExist satisfiability.

[mattfarina]

I apologize if I wasn't clear. The release notes did not contain this change. See https://golang.org/doc/go1.9. Having this response change there would have been useful and saved me a bit of time trying to figure out what happened.

This wouldn't be such a problem if Go didn't have the model of using go get to install an application. A change like this breaks backwards compatibility. That means you upgrade Go and apps you install via go get may work differently. This kind of pokes a hole in the model of master being stable when. It's not in the app developers hands.

[dmitshur]

@mattfarina Your latest comment goes counter to what I said in the 2nd half of this comment. It's not clear to me, is it because you didn't have a chance to read my comment, or is it because you disagree with it?

[mattfarina]

@shurcooL There are two problems...

1. There is an expectation of using go get to install applications. That means you, as an app developer, don't have full control so issues come in like this. With a compiled binary you have more control. This leads to problems like the one I encountered cropping up in the wild. This provides a greater opportunity for subtle changes in the language spec to have a bigger impact.
2. Code consuming errors will often look at the error and try to handle the situation rather than just report it. Errors are often seen as part of the contract. When I explained this situation in a technical sense to a few people they saw it as changing the contract in an annoying way.

Since the error string "cannot find package " is used more than once... does it mean the code is not present on the system? Could/should that be its own type? That way the string could change in the future but we could rely on the type not changing. In a similar way to build.NoGoError.

[rsc]

os.IsNotExist unwraps any os.PathError wrapping before checking for err == os.ErrNotExist. That definition seems right. But if the goal of the recent work is to explain things like "the following four directories all do not exist" then that is stretching it a bit. There's lots about error handling that's not quite satisfactory yet in Go, and this is one of those. The errors are not guaranteed except as documented, and this one is not guaranteed.

If tools actually need to find this error from Import, maybe there should be a new error type, like build.NoGoError. @shurcooL, what do you think about adding build.UnknownImportError?

[dmitshur]

@rsc I think I understand exactly what you're suggesting, but let my describe it so you can confirm that I got it right.

Are you imagining it looking roughly like this (similar to build.NoGoError and build.MultiplePackageError)?

// UnknownImportError is the error used by Import when none of
// the directory(s) that correspond to the import path exist.
type UnknownImportError struct {
	ImportPath string // Import path that cannot be found.
	Dirs []string     // Places looked.
}

func (e *UnknownImportError) Error() string {
	return fmt.Sprintf("cannot find package %q in any of:\n%s",
		e.ImportPath, strings.Join(e.Dirs, "\n"))
}

And the motivation is to help simplify/change current code that would look like this:

p, err := build.Import(path, srcDir, mode)
if err != nil && strings.HasPrefix(err.String(), "cannot find package ") {
    // behavior A (package with said import path was determined to not exist)
} else if err != nil {
    // behavior B (some unexpected I/O or other type of error)
}

To:

p, err := build.Import(path, srcDir, mode)
if _, ok := err.(*build.UnknownImportError); ok {
    // behavior A (package with said import path was determined to not exist)
} else if err != nil {
    // behavior B (some unexpected I/O or other type of error)
}

If so, then...

@shurcooL, what do you think about adding build.UnknownImportError?

I think there's a cost to increasing the API size, but it seems to be justified to help distinguish this common type of error (unknown import path) from other I/O errors. Using such an error type is consistent due to the precedent set by build.NoGoError and build.MultiplePackageError.

(On the topic of name, I would also consider build.NoPackageError. I think that may fit better, given the other 2 error type names.)