code review 125990044: cmd/go: vet packages "foo" and "foo_test" separately

cmd/go: vet packages "foo" and "foo_test" separately

When the vet tool is invoked with a list of source files, it expects
them to all be in a single package.

Fixes issue 8511.

[mdempsky, 2014-08-12 00:00:02]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2014-08-12 00:46:38]

I'm not keen on adding another field to the package struct for just
one use case.

Can you do the accumulation of files at the point that go vet needs to
use them instead ?

On Tue, Aug 12, 2014 at 10:00 AM, mdempsky via golang-codereviews
<golang-codereviews@googlegroups.com> wrote:
> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/go: vet packages "foo" and "foo_test" separately
>
> When the vet tool is invoked with a list of source files, it expects
> them to all be in a single package.
>
> Fixes issue 8511.
>
> Please review this at https://codereview.appspot.com/125990044/
>
> Affected files (+16, -6 lines):
>   M src/cmd/go/fix.go
>   M src/cmd/go/fmt.go
>   M src/cmd/go/pkg.go
>   M src/cmd/go/vet.go
>
>
> Index: src/cmd/go/fix.go
> ===================================================================
> --- a/src/cmd/go/fix.go
> +++ b/src/cmd/go/fix.go
> @@ -22,7 +22,7 @@
>
>  func runFix(cmd *Command, args []string) {
>         for _, pkg := range packages(args) {
> -               // Use pkg.gofiles instead of pkg.Dir so that
> +               // Use pkg.allgofiles instead of pkg.Dir so that
>                 // the command only applies to this package,
>                 // not to packages in subdirectories.
>                 run(stringList(tool("fix"), relPaths(pkg.allgofiles)))
> Index: src/cmd/go/fmt.go
> ===================================================================
> --- a/src/cmd/go/fmt.go
> +++ b/src/cmd/go/fmt.go
> @@ -30,7 +30,7 @@
>
>  func runFmt(cmd *Command, args []string) {
>         for _, pkg := range packages(args) {
> -               // Use pkg.gofiles instead of pkg.Dir so that
> +               // Use pkg.allgofiles instead of pkg.Dir so that
>                 // the command only applies to this package,
>                 // not to packages in subdirectories.
>                 run(stringList("gofmt", "-l", "-w",
> relPaths(pkg.allgofiles)))
> Index: src/cmd/go/pkg.go
> ===================================================================
> --- a/src/cmd/go/pkg.go
> +++ b/src/cmd/go/pkg.go
> @@ -76,9 +76,10 @@
>         pkgdir       string // overrides build.PkgDir
>         imports      []*Package
>         deps         []*Package
> -       gofiles      []string // GoFiles+CgoFiles+TestGoFiles+XTestGoFiles
> files, absolute paths
> +       gofiles      []string // GoFiles+CgoFiles+TestGoFiles files,
> absolute paths
> +       xgofiles     []string // XTestGoFiles files, absolute paths
>         sfiles       []string
> -       allgofiles   []string             // gofiles + IgnoredGoFiles,
> absolute paths
> +       allgofiles   []string             // gofiles + xgofiles +
> IgnoredGoFiles, absolute paths
>         target       string               // installed file for this package
> (may be executable)
>         fake         bool                 // synthesized package
>         forceBuild   bool                 // this package must be rebuilt
> @@ -510,12 +511,18 @@
>
>         // Build list of full paths to all Go files in the package,
>         // for use by commands like go fmt.
> -       p.gofiles = stringList(p.GoFiles, p.CgoFiles, p.TestGoFiles,
> p.XTestGoFiles)
> +       p.gofiles = stringList(p.GoFiles, p.CgoFiles, p.TestGoFiles)
>         for i := range p.gofiles {
>                 p.gofiles[i] = filepath.Join(p.Dir, p.gofiles[i])
>         }
>         sort.Strings(p.gofiles)
>
> +       p.xgofiles = stringList(p.XTestGoFiles)
> +       for i := range p.xgofiles {
> +               p.xgofiles[i] = filepath.Join(p.Dir, p.xgofiles[i])
> +       }
> +       sort.Strings(p.xgofiles)
> +
>         p.sfiles = stringList(p.SFiles)
>         for i := range p.sfiles {
>                 p.sfiles[i] = filepath.Join(p.Dir, p.sfiles[i])
> @@ -526,7 +533,7 @@
>         for i := range p.allgofiles {
>                 p.allgofiles[i] = filepath.Join(p.Dir, p.allgofiles[i])
>         }
> -       p.allgofiles = append(p.allgofiles, p.gofiles...)
> +       p.allgofiles = append(append(p.allgofiles, p.gofiles...),
> p.xgofiles...)
>         sort.Strings(p.allgofiles)
>
>         // Check for case-insensitive collision of input files.
> Index: src/cmd/go/vet.go
> ===================================================================
> --- a/src/cmd/go/vet.go
> +++ b/src/cmd/go/vet.go
> @@ -33,5 +33,8 @@
>                 // the command only applies to this package,
>                 // not to packages in subdirectories.
>                 run(tool("vet"), relPaths(stringList(pkg.gofiles,
> pkg.sfiles)))
> +               if len(pkg.xgofiles) > 0 {
> +                       run(tool("vet"), relPaths(stringList(pkg.xgofiles)))
> +               }
>         }
>  }
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[mdempsky, 2014-08-12 02:08:28]

On 2014/08/12 00:46:38, dfc wrote:
> I'm not keen on adding another field to the package struct for just
> one use case.
> 
> Can you do the accumulation of files at the point that go vet needs to
> use them instead ?

Done.  I also went ahead and killed the gofiles/sfiles/allgofiles fields
altogether.  PTAL.

[rsc, 2014-08-12 15:32:44]

R=rsc

[mdempsky, 2014-08-27 19:34:07]

On 2014/08/12 15:32:44, rsc wrote:
> R=rsc

Ping?  In the interest of getting this fix in for Go 1.4, I've pruned it down to
the minimum / purely additive changes needed to fix issue 8511, and deferred the
cleanup work to a followup CL.

[rsc, 2014-10-20 18:53:39]

R=close

I missed this CL when it came in, sorry.
I ended up reconstructing this fix separately.

changeset:   21418:25ea6fb44f62
user:        Russ Cox <rsc@golang.org>
date:        Mon Oct 06 14:49:22 2014 -0400
summary:     cmd/go: fix 'go vet' of package with external tests