Skip to content
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

Support go modules #148

Merged
merged 9 commits into from
Aug 7, 2018
Merged

Conversation

suzmue
Copy link
Contributor

@suzmue suzmue commented Aug 1, 2018

Use golang.org/x/tools/go/packages to load and type check packages.

This provides support for projects using go modules to also be able to
use errcheck.

errcheck can support packages that import "C", though there are
limitations when run with versions of Go that are earlier than Go 1.11.

There is also support in go/packages for finding the package that "contains" a file, so should be easy to add support for #106.

Use golang.org/x/tools/go/packages to load and type check packages.
This provides support for projects using go modules to also be able to
use errcheck.

Tests for vendored packages must be run in a GOPATH.
errcheck can now support packages importing "C", though there are
limitations with versions of Go that are earlier than Go 1.11.
@kisielk
Copy link
Owner

kisielk commented Aug 1, 2018

Hi Suzy, thanks very much for this update!

Based on the tests it looks like this will only work on Go 1.9 or above? Since we have tagged releases I'm fine with dropping support for 1.8 and earlier but the Readme should probably be updated to indicate that.

@suzmue
Copy link
Contributor Author

suzmue commented Aug 1, 2018

A couple of notes on the failed tests (including a couple of questions for you):

Type check errors:

  • The extra messages printed out are type checking errors, and in many cases having some unresolved imports works fine, as the type checker deals well with missing imports and broken code.
  • Would you prefer to stop printing type check errors to os.Stderr (they are still available in Package.Errors to look at)? I think it is probably best to have it stop printing them out as it can be confusing.
  • Do you want errcheck to fail if there are type checking errors in the initial packages? I noticed in one of the other issues I was reading you were concerned about false negatives, so if you want errcheck to bail early I can add that.

Support for < Go 1.9:

  • golang.org/x/tools/go/packages currently attempts to support Go 1.10 and earlier using the go command, but it looks like there was a change in Go 1.9 that makes that fallback not work for earlier version.
  • I will update the documentation to include that, thanks for letting me know!

@kisielk
Copy link
Owner

kisielk commented Aug 1, 2018

I think it's better not to print the messages to os.Stderr, I don't really think they provide much for the user to act on and will probably just create confusion.

@kisielk
Copy link
Owner

kisielk commented Aug 1, 2018

As far as errors in type checking the initial package, I think it's better to fail by default. We could provide a flag that allows you to continue checking even if the type checking fails but I think it should not be enabled by default.

If there are type checking errors in the initial packages, do not
continue with checking. Stop early to avoid a situation where there are false
negatives because the whole program could not be fully checked for
unchecked errors.
@suzmue
Copy link
Contributor Author

suzmue commented Aug 2, 2018

Hey Kamil, I made the changes we discussed (bailing early with type check errors in initial packages and not printing all type check errors to stderr) and I'm going to look into the breaking issue at tip and I'll give you an update when I know more!

@kisielk
Copy link
Owner

kisielk commented Aug 2, 2018

Looks like it's passing now. Do you think this could be made to work for 1.8? or does that just need to be dropped? I assumed the errors in the tests were because of incompatibilities there but it seems it was actually the type checking errors, which should now be fixed.

@suzmue
Copy link
Contributor Author

suzmue commented Aug 2, 2018

Ok, I'll run the tests back to Go 1.7 to see if the issue was fixed. I believe that a change to the go command between Go 1.8 and 1.9 may have broken it, which is a different issue than the type checking errors.

@suzmue
Copy link
Contributor Author

suzmue commented Aug 3, 2018

The incompatibility with Go < 1.9 is still there, but the other issues have been resolved. I pushed one final commit just to run the main test on the _test files as well.

@kisielk
Copy link
Owner

kisielk commented Aug 7, 2018

Great. Thanks very much for your contribution!

dominikh added a commit that referenced this pull request Aug 7, 2018
@myitcv
Copy link

myitcv commented Oct 31, 2018

@kisielk - can you confirm that we can mark errcheck in golang/go#24661 as "complete"? Thanks

@domgreen
Copy link
Contributor

Should be able to get this running again by updating internal/errcheck/errcheck.go to:

func (c *Checker) load(paths ...string) ([]*packages.Package, error) {
	cfg := &packages.Config{
		Mode:       packages.LoadAllSyntax,
		Tests:      !c.WithoutTests,
		BuildFlags: []string{fmt.Sprintf("-tags=%s", strings.Join(c.Tags, " "))},
	}
	return loadPackages(cfg, paths...)
}

@domgreen
Copy link
Contributor

@suzmue have sent you a PR against your branch 👍

@jared2501
Copy link

Oh no looks like this was reverted! getting errcheck onto go modules would be really nice :)

@tschellenbach
Copy link

yup that would be nice

coopernurse pushed a commit to coopernurse/errcheck that referenced this pull request Dec 26, 2018
coopernurse pushed a commit to coopernurse/errcheck that referenced this pull request Dec 28, 2018
coopernurse pushed a commit to coopernurse/errcheck that referenced this pull request Jan 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants