Fails if anything uses cgo
tv42 opened this issue · 22 comments
This is probably a known limitation, but I couldn't find a ticket for it, so here goes:
errcheck fails if a dependency uses cgo (has import "C").
Additionally, it seems impossible to ignore this lack of "C" via -ignorepkg (or I just don't know how to use the thing).
[0 tv@brute ~/src/2013/errcheck-bug-cgo]$ ls
repro.go
[0 tv@brute ~/src/2013/errcheck-bug-cgo]$ cat repro.go
package main
import _ "github.com/jmhodges/levigo"
func main() {
}
[0 tv@brute ~/src/2013/errcheck-bug-cgo]$ errcheck .
error:failed to check package: could not type check: repro.go:3:10: could not import github.com/jmhodges/levigo (/home/tv/go/src/github.com/jmhodges/levigo/conv.go:4:8: could not import C (cannot find package "C" in any of:
/home/tv/src/go/src/pkg/C (from $GOROOT)
/home/tv/go/src/C (from $GOPATH)))
[1 tv@brute ~/src/2013/errcheck-bug-cgo]$ errcheck -ignorepkg=github.com/jmhodges/levigo .
error:failed to check package: could not type check: repro.go:3:10: could not import github.com/jmhodges/levigo (/home/tv/go/src/github.com/jmhodges/levigo/cache.go:6:8: could not import C (cannot find package "C" in any of:
/home/tv/src/go/src/pkg/C (from $GOROOT)
/home/tv/go/src/C (from $GOPATH)))
[1 tv@brute ~/src/2013/errcheck-bug-cgo]$ errcheck -ignorepkg=C .
error:failed to check package: could not type check: repro.go:3:10: could not import github.com/jmhodges/levigo (/home/tv/go/src/github.com/jmhodges/levigo/iterator.go:6:8: could not import C (cannot find package "C" in any of:
/home/tv/src/go/src/pkg/C (from $GOROOT)
/home/tv/go/src/C (from $GOPATH)))
[1 tv@brute ~/src/2013/errcheck-bug-cgo]$
types.Config now has a FakeCImports field, but it doesn't solve this problem.
Instead we just get:
$ errcheck repro
error: failed to check package repro: could not type check: /home/kamil/src/repro/repro.go:3:10: could not import github.com/jmhodges/levigo (/home/kamil/src/github.com/jmhodges/levigo/batch.go:23:21: cannot use wb (variable of type invalid type) as *invalid type value in struct literal)
Note that I had to set FakeCImport in both errcheck and honnef.co/go/importer to get that.
So it seems the problem is that with FakeImportC, none of the types defined in the C libs are processed, so you get errors like variable with invalid type for types like C.int, etc.
How can we fix this? Not being able to use types stuff on packages that (indirectly) import "C" is a deal-breaker for my needs.
Good question. It's not really important for us to recognize C types, since we only look for the error interface. Maybe we can paper over that somehow?
I've made some progress on this, see discussion at https://github.com/dominikh/implements/issues/6. If you don't care about C types, you can ignore them like doc does.
No luck, I tried this:
--- a/lib/errcheck.go
+++ b/lib/errcheck.go
@@ -98,7 +98,11 @@ func typeCheck(p package_) (typedPackage, error) {
Types: tp.callTypes,
Objects: tp.identObjs,
}
- context := types.Config{Import: importer.NewImporter().Import}
+ context := types.Config{
+ Import: importer.NewImporter().Import,
+ FakeImportC: true,
+ Error: func(error) {}, // Ignore errors so we can support packages that import "C"
+ }
_, err := context.Check(p.path, p.fset, p.astFiles, &info)
return tp, err
diff --git a/lib/util.go b/lib/util.go
index f3e4978..5c20dfe 100644
--- a/lib/util.go
+++ b/lib/util.go
@@ -15,7 +15,7 @@ func findPackage(path string) (*build.Package, error) {
)
ctx := build.Default
- ctx.CgoEnabled = false
+ ctx.CgoEnabled = true
// First try to treat path as import path...
pkg, err1 = ctx.Import(path, ".", 0)
@@ -35,9 +35,12 @@ func findPackage(path string) (*build.Package, error) {
// getFiles returns all the Go files found in a package
func getFiles(pkg *build.Package) []string {
- files := make([]string, len(pkg.GoFiles))
+ files := make([]string, len(pkg.GoFiles)+len(pkg.CgoFiles))
for i, fileName := range pkg.GoFiles {
files[i] = filepath.Join(pkg.Dir, fileName)
}
+ for i, fileName := range pkg.CgoFiles {
+ files[len(pkg.GoFiles)+i] = filepath.Join(pkg.Dir, fileName)
+ }
return files
}
And also set FakeImportC: true in importer. I now get an error like:
$ ./errcheck github.com/kisielk/go-hdf5
error: failed to check package github.com/kisielk/go-hdf5: could not type check: /Users/kamil/src/github.com/kisielk/go-hdf5/h5f.go:30:25: cannot convert 0 (untyped integer constant) to invalid type
@kisielk, note that ignoring types.Check() errors the way doc does takes exactly two things:
- Add
Error: func(error) {}so it keeps going past first error, see doc.go#312. - Ignore error value returned from
types.Check(), see doc.go#328.
From your diff above, it looks like you've done 1, but not done 2. You've also FakeImportC to true, which doesn't seem necessary.
Try it like this:
@@ func typeCheck(p package_) (typedPackage, error) {
Types: tp.callTypes,
Objects: tp.identObjs,
}
- context := types.Config{Import: importer.NewImporter().Import}
+ context := types.Config{
+ Import: importer.NewImporter().Import,
+ Error: func(error) {}, // Ignore errors so we can support packages that import "C"
+ }
- _, err := context.Check(p.path, p.fset, p.astFiles, &info)
- return tp, err
+ // Also ignore error from types.Check() so we can support packages that import "C"
+ _, _ := context.Check(p.path, p.fset, p.astFiles, &info)
+ return tp, nil
}I don't think ignoring the error from context.Check is a good idea. There could be other problems with the code that will keep the errors from being type-checked correctly. I really want to avoid false negatives since they undermine the purpose of using such a tool.
That's fair; I was primarily describing how doc.go goes about solving the problem.
That said, it seems I've found a great way to handle packages that import "C", if they happen to have a compiled version (i.e. an *.a file in $GOPATH/pkg/). If parsing from source fails due to import "C", just fall back to GcParse, which works on cgo-using libraries that are compiled. See here for details.
I'm gonna submit a PR (edit: submitted) with that change to "honnef.co/go/importer" which you use. It'd be great if you could test with that. Just temporarily change your import from "honnef.co/go/importer" to "github.com/shurcooL/go-importer", once it's there (within half an hour).
I'm not extremely familiar with errcheck, but from what I understand, this fix should be general and shouldn't make things worse, only better in some cases (if the cgo-using package is not compiled, it won't help).
I still get the invalid type error. It's also pretty clear the go/types stops processing when it encounters that since there's other parts of the code later on that should raise the same error.
The way errcheck works, the supposed fix in the importer will (should) only help with dependencies, not with the package you're checking.
So if you're checking foo, which imports bar that has a "C" import, the fix should work, as long as bar has been go install'd. It won't help if foo imports "C" or if bar isn't installed.
The idea of the fix is that by using the GcImporter, go/types will not have to parse the dependencies and can just get the type information from the gc generated file, in which case it should have no trouble with "C".
To concretize, my importer has a new option UseGcFallback, which will attempt to use GcImport for dependencies that import "C". It will also populate a field Fallbacks with all imports that it had to rely on GcImport for, so that one can print a warning to its users (since the gc data is potentially not in sync with the sources).
For errcheck that means that it could check any package A that imports package B that imports "C". It still won't be able to check A if A imports "C" directly.
UseGcFallback defaults to false though, to preserve the current behaviour.
I added some basic cgo support based on Domink's comment above. The README explains how it works.
It looks like go/loader can import some cgo stuff unless it uses certain features like pkg-config
I guess this issue can be closed?
zimmski@schroeder:~/go/src/github.com/zimmski/bla> cat main.go
package main
import _ "github.com/jmhodges/levigo"
func main() {
}
zimmski@schroeder:~/go/src/github.com/zimmski/bla> errcheck .
zimmski@schroeder:~/go/src/github.com/zimmski/bla> errcheck github.com/jmhodges/levigo/...
github.com/jmhodges/levigo/leveldb_test.go:188:17 DestroyDatabase(dbname, options)
github.com/jmhodges/levigo/leveldb_test.go:239:8 db.Put(wo, nil, []byte("love"))
github.com/jmhodges/levigo/leveldb_test.go:288:8 db.Put(wo, []byte("bat"), []byte("somedata"))
github.com/jmhodges/levigo/leveldb_test.go:289:8 db.Put(wo, []byte("done"), []byte("somedata"))
Looks like it, someone can always reopen if there are still problems.
This may be obvious to some, but recording here for future searchers.
I was getting many cgo related errors (when running errcheck inside an alpine linux docker container with no gcc). Fixed them by running:
CGO_ENABLED=0 errcheck ./...
(edit: set to 0 not 1)
It seems surprising to me that CGO_ENABLED needs to be set in the general case. Can anyone confirm?
Edit: Even if the value should be 0 rather than 1, I still find it surprising.
Sorry major typo there. I mean CGO_ENABLED=0. I needed to explicitly disable CGO when running errcheck, even though go build and go install could compile the project fine without gcc.