Fails if anything uses cgo #16

Closed
tv42 opened this Issue Mar 11, 2013 · 22 comments

Projects

None yet

6 participants

@tv42
tv42 commented Mar 11, 2013

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]$ 
@kisielk kisielk was assigned Mar 11, 2013
@kisielk
Owner
kisielk commented Sep 10, 2013

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)
@kisielk
Owner
kisielk commented Sep 10, 2013

Note that I had to set FakeCImport in both errcheck and honnef.co/go/importer to get that.

@shurcooL
Contributor

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.

@kisielk
Owner
kisielk commented Oct 14, 2013

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?

@shurcooL
Contributor

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.

@kisielk
Owner
kisielk commented Oct 14, 2013

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
@shurcooL
Contributor

@kisielk, note that ignoring types.Check() errors the way doc does takes exactly two things:

  1. Add Error: func(error) {} so it keeps going past first error, see doc.go#312.
  2. 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
 }
@kisielk
Owner
kisielk commented Oct 15, 2013

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.

@shurcooL
Contributor

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).

@shurcooL
Contributor

@kisielk, can you try errcheck with "github.com/shurcooL/go-importer" now?

@kisielk
Owner
kisielk commented Oct 15, 2013

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.

@dominikh
Collaborator

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.

@dominikh
Collaborator

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".

@dominikh
Collaborator

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.

@kisielk kisielk added a commit that referenced this issue Feb 21, 2014
@kisielk Add some preliminary cgo support.
Updates #16.
b8e64b0
@kisielk
Owner
kisielk commented Feb 21, 2014

I added some basic cgo support based on Domink's comment above. The README explains how it works.

@robfig robfig referenced this issue in opennota/check Sep 4, 2014
Closed

Ignore import errors if possible #4

@kisielk
Owner
kisielk commented Jan 14, 2015

It looks like go/loader can import some cgo stuff unless it uses certain features like pkg-config

@zimmski
Contributor
zimmski commented Oct 18, 2015

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"))
@kisielk
Owner
kisielk commented Oct 18, 2015

Looks like it, someone can always reopen if there are still problems.

@kisielk kisielk closed this Oct 18, 2015
@amacneil
amacneil commented Dec 1, 2015

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)

@shurcooL
Contributor
shurcooL commented Dec 5, 2015

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.

@amacneil
amacneil commented Dec 6, 2015

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.

@zimmski
Contributor
zimmski commented Jan 26, 2016

@amacneil: Can you please retest your problem and open up a new issue if it is still the case. Also, please include some public packages so we can test them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment