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/build: ImportDir/Import no longer return os not found error for missing dir on local files #21923
Comments
In go 1.9 there was a change to the way ImportDir, or rather Import with a local "." package, responded when not found. Previously, the error was an OS not found error that could be detected by os.IsNotExist. Now the error is a custom go error using the format: fmt.Errorf("cannot find package %q in:\n\t%s", path, p.Dir) This change looks for both cases. For Go 1.9 is checks of it doesn't exist because there are similar errors in go/build. For more detail on the Go side of this see golang/go#21923
Thanks for the /cc. This is indeed related to the fix for #17863.
Unfortunately, The previous behavior of Lines 317 to 320 in bb2f0da
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 So, I think this is unfortunate but I don't see any way to make 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
FWIW, |
To add a little context...
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 Line 673 in c8aec40
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 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? |
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? |
I agree with @ianlancetaylor here. I think you should update your code and treat a non-
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 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. |
That said, there may be room for improvement of how |
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 |
@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? |
@shurcooL There are two problems...
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 |
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? |
@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 // 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...
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 (On the topic of name, I would also consider |
I worry about MultiplePackageError - which means "multiple packages in this directory" - suggesting that NoPackageError would mean "no packages in this directory" - when in fact that's NoGoError. Or vice versa if NoPackageError means "can't find package in this list of directories" then I'd expect MultiplePackageError to mean "found package in more than one of this list of directories". So I think I would stick with not using "PackageError" in the name for this one. NoGoError and MultiplePackageError are closely related but this error is different. Otherwise yes that's the proposal and it sounds good to me. |
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 thego/build
package) to look at the packages used by another package. If there is an error fromImportDir
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
In 1.8.3 (and before), the
Import
function would fall through toctxt.readDir
that usedio.ReadDir
...go/src/go/build/build.go
Lines 171 to 177 in 352996a
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
You can no longer rely on
os.IsNotExist
to detect missing package directories.This change was not documented in the release notes, either.
The text was updated successfully, but these errors were encountered: