palantir / checks Goto Github PK

Repro:

1. Create a project with foo.go consisting of package foo and .hidden/vendor/github.com/bar/bar.go consisting of package bar
2. Run novendor

Expected

No unused packages should be reported

Actual

github.com/bar is reported as an unused package

The above scenario can occur in real-world situations due to things like Terraform, which may create a hidden directory in the project that contains vendor directories.

A fully generic solution would require a mechanism like specifying directories or patterns to exclude form consideration (or to specify that only a root-level vendor directory for a project will be checked). However, for now, a low-lift fix that should work for most of these scenarios is to simply not consider any packages in hidden directories as vendored packages.

Consider the following example:

vendor/github.com/org/project
  |--> foo
  |--> bar
vendor/github.com/org-2/project-2
  |--> baz

The main project code imports org/project/foo.
org/project/bar imports org-2/project-2/baz.

Currently, if novendor is run in this scenario, org-2/project-2/baz will be reported as unused. Internally, org/project/bar is also considered unused, but because the heuristic indicates that it is part of a "project" (org/project) that is used, it is not reported.

Based on this report, the logical course of action is to remove org-2/project-2. Although this will work, if a command like go build ./... or go install ./... is run, org/project/bar will complain because its dependency (org-2/project-2/baz) is missing.

novendor should provide an option that offers a way out of this scenario without explicit whitelisting. The idea would be that all "projects" that are considered by novendor should also be treated as source roots, and the dependency graphs for those packages should be included in the check.

In the scenario outlined above, running the check in this mode would not report any unused packages.

I believe golint or govet already catches unused variables, but we should check for unused functions as well.

The goal is to ensure that, if a package import is given an alias, then the alias for that package import is consistent across all of the different files in the project. This ensures readability and consistency across the files in a project.

These should be safe to run in parallel. Running the generators in parallel will cut down on the relatively long gogenerate times for big projects.

Add ability to check that a particular package does not import a different package. E.g.
github.com/foo/client can not import github.com/foo/impl, transitively or otherwise.

Related to #16, although less likely to occur in practice.

Consider the following example:

$GOPATH/src/github.com/myorg/myproject
  |--> main/main.go
  |--> foo/foo.go
  |--> bar/bar.go
  |--> vendor/github.com/baz/baz.go

• main imports foo
• foo does not import anything
• bar imports baz

In this scenario, if novendor is run specifically on main only, baz is reported as an unused vendored dependency. However, if project-level grouping is done, then technically it shouldn't be -- since main imports foo and bar is in the same "project" as foo, the fact that bar uses baz should make it considered not unused.

However, in practice I don't think this will occur very often (if ever) because it only really makes sense to run novendor on all project packages, so main, foo and bar would always be analyzed together. However, filing for completeness from a conceptual standpoint.

Once this is fixed, a case like the following in TestNovendor should pass:

    {
      name: "multi-level transitive imports: import a local package that has a project-level import that imports a vendored package",
      getArgs: func(projectDir string) (string, []string) {
        return projectDir, []string{"main"}
      },
      files: []gofiles.GoFileSpec{
        {
          RelPath: "main/main.go",
          Src:     `package main; //import _ "{{index . "foo/foo.go"}}";`,
        },
        {
          RelPath: "foo/foo.go",
          Src:     `package foo`,
        },
        {
          RelPath: "bar/bar.go",
          Src:     `package bar; import _ "github.com/baz";`,
        },
        {
          RelPath: "vendor/github.com/baz/baz.go",
          Src:     `package baz`,
        },
      },
      // if packages are not grouped by project, then any package not directly imported should be reported as unused
      noGroupOutputLines: func(files map[string]gofiles.GoFile) []string {
        return []string{
          files["vendor/github.com/baz/baz.go"].ImportPath,
        }
      },
    },

1. Vendor a package that has both a regular package and a main package that is excluded using build constraints that depends on another vendored package

• An example of such a package is https://github.com/google/go-github/tree/master/github -- most of the files are package github and this package imports https://github.com/google/go-querystring, but gen-accessors.go is a package main excluded using an "ignore" build directive

2. Run novendor on a project that vendors such a package

Expected

novendor should not report any output

Actual

novendor reports that the dependent package is unused

Issue

By default, novendor does package analysis in a matter that considers all files in a package regardless of build constraints. This is done so that the analysis is consistent across platforms (for example, if a linux build constraint files imports packages that are different from darwin build constraint files, we want to perform analysis for both).

However, this causes an issue when build directives are used to exclude files that declare different packages, which is often done for files with a main package that are used in a one-off manner (typically by a go generate directive) -- in this case, the Go importer can't process the package and thus can't determine the imports, and this causes the dependent packages to not be found.

The ideal solution would be to exclude only main package files, or to match all build constraints that are not ignore. However, neither of these approaches are possible using the default functions provided by Go's build package.

Since that's not possible, instead I'm going to do the following:

• Attempt to import the package including all files
• If the above fails, attempt importing again. This time, don't include all files, but match all build constraints for OS and Arch
  • linux, darwin, amd64, arm, etc. -- this list will be hard-coded
  • Rationale is that, even in fallback mode, we want all files for different OS/Archs to be considered so output is consistent across platforms

Pretty sure this should solve the issue.

Consider a package layout like this:

mypkg/
  - mypkg.go
  - subpkg/
     - subpkg.go

In subpkg.go, I have the following imports:

import (
  "net/http"

  "github.com/pkg/errors"

  "github.com/myorg/mypkg"
)

This follows the expected grouping of standard, third-party, and local packages. Running ptimports changes the grouping to:

import (
  "net/http"

  "github.com/myorg/mypkg"
  "github.com/pkg/errors"
)

In other words, my root package is considered a third-party package. I believe this is due to the trailing slash added here, which causes the prefix check here to fail.

I'm using ptimports via Godel 0.27.0.

ptimports formats import "C" like any other import which results in compilation errors if there was a C code defined above import "C"

novendor falls into an infinite loop if a directory of the following form is vendored:

• Contains valid file with valid package
• Contains a file excluded by a CGo build constraints that imports "C"
• Contains a file excluded by another build constraint

The first iteration parses the valid file and buckets the other 2 files as invalid. The next iteration parses the 2 invalid files, and results in 0 valid files and 2 invalid files. However, the logic does not break at this point, which results in an infinite loop.

Discovered by a project that vendored https://github.com/google/certificate-transparency-go/tree/master/x509.

If I configure novendor to ignore a package, it should automatically ignore dependencies of that package.

The current state of the world is a long list of exclusions that is difficult to keep up to date.

Currently, some of the checks have their main package in a main subdirectory: for example, for checks/gocd, the main is at checks/gocd/main/checks. However, this is unintuitive for go get calls.

It would be better to move the main package to the root of the check directory so that they can be fetched using commands like go get github.com/palantir/checks/gocd.