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

x/tools/go/packages: no way to easily distinguish the test version of the package from the real one #27910

Open
bcmills opened this issue Sep 27, 2018 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 27, 2018

When Config.Test is set to true, packages.Load may return multiple Package instances with the same PkgPath.

go list encodes the information about which package is which redundantly: both in the ForTest field and also in the reported ImportPath for test variants.

go/packages strips out the ImportPath annotations, but also fails to propagate the ForTest field. As a result, the only way to distinguish the two packages is by checking which one's GoFiles are a superset of the other's. That works most of the time, but I suspect it will fail in some cases (for example, when b imports a and a_test imports b, the test version of b for a_test is compile with exactly the same sources as the non-test copy of b).

CC: @matloob @alandonovan @ianthehat

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 27, 2018
@gopherbot gopherbot added this to the Unreleased milestone Sep 27, 2018
@bcmills
Copy link
Contributor Author

bcmills commented Sep 28, 2018

the only way to distinguish the two packages

There are a couple other alternatives I hadn't considered:

  • I can check for files with a _test.go suffix, assuming that all build systems will produce that suffix for test files (including generated ones).

  • If the Imports field is set, I can check for an import of the testing package, assuming that all tests actually import it. (But are they required to? What happens if a test defines func main() instead?)

At any rate, if there is some specific alternative that is expected to be reliable enough for all users of go/packages, it should be described in the go/packages documentation.

@nmiyake
Copy link
Contributor

nmiyake commented Dec 24, 2018

I'm running into this right now as well -- I'm looking at transitioning the deadcode tool to work with Go modules (#24661), and encountered this when transitioning to x/tools/go/packages.

I believe that adding a field like ForTest in the packages.Package struct would fix this.

Without this, right now I am left to depend on parsing output of the ID field to determine whether or not a package comes from a test, which is brittle (and explicitly against the advice of the documentations, which says that the ID field should not be parsed). However, my current guess is that this will have fewer edge cases than depending on the files of one version of the package being a superset of the other.

This is based on the output that I currently get from using packages.Load given a directory testdata/p3 with sample.go and sample_test.go (both of package p) in a module github.com/org/deadcode:

  • packages.Package{ID:"github.com/org/deadcode/testdata/p3", Name:"p", PkgPath:"github.com/org/deadcode/testdata/p3", ...
  • packages.Package{ID:"github.com/org/deadcode/testdata/p3 [github.com/org/deadcode/testdata/p3.test]", Name:"p", PkgPath:"github.com/org/deadcode/testdata/p3", ...
  • packages.Package{ID:"github.com/org/deadcode/testdata/p3.test", Name:"main", PkgPath:"github.com/org/deadcode/testdata/p3.test", ...

@nmiyake
Copy link
Contributor

nmiyake commented Dec 24, 2018

@bcmills with regards to your 2 other suggestions in your follow-on comment:

  • The first approach seems like it would work (with the caveat of build systems you note), although it does also require a check to distinguish between a test and external test package (verify that package name is p AND it has _test.go files -- otherwise, if package package p_test exists, that will also match)
  • I don't think the second approach will work generally -- besides the issue you mentioned in your comment, it's also possible (albeit unlikely) for non-test files to import the testing package, in which case you could get false positives

@matloob
Copy link
Contributor

matloob commented Jan 10, 2019

cc @ianthehat because he had a hat

It's not totally obvious where to draw the line of test vs non-test package. By my count, there's three categories of packages that can be considered tests:

  • The test main package (ForTest edges point to these)
  • The package containing test code (the test variant of the library being tested, and the xtest if it exists)
  • and most confusingly: the test variants of the dependencies of the test code package.

It's difficult to distinguish between the second group and the third group (at least without doing failure prone package name comparisons).

I think it would be confusing and misleading to mark all three as "tests", and I also think that the ForTest field isn't quite what people want.

@myitcv
Copy link
Member

myitcv commented Feb 14, 2019

Adding to @bcmills' description above, a concrete instance of where this is a problem is a go generate generator.

go generate passes you the GOPACKAGE variable, which is the name of the package. It also runs the generator in the directory of the package, so calling go/packages.Load with the current directory suffices.

The call to Load must set Config.Test to true when running for the non-xtest and xtest calls from go generate.

For a main package, you can see from the output of the following script that it is impossible to determine from the PkgPath and Name which of the returned packages is the "right" one. As @bcmills discusses above, you need to fall back to some other heuristic, or introspect the ID.

You can also see from the go list output that ForTest helps to make that distinction clear.

testscript script
go run . ./p ./q
go list -test -f '{{.ImportPath}}{{print "\n\t"}}Name: {{.Name}}{{print "\n\t"}}ForTest: {{.ForTest}}' ./p
go list -test -f '{{.ImportPath}}{{print "\n\t"}}Name: {{.Name}}{{print "\n\t"}}ForTest: {{.ForTest}}' ./q

-- go.mod --
module mod.com

require golang.org/x/tools v0.0.0-20190213192042-740235f6c0d8

-- main.go --
package main

import (
"flag"
"fmt"
"os"
"sort"

"golang.org/x/tools/go/packages"

)

func main() {
flag.Parse()

cfg := &packages.Config{
	Tests: true,
	Mode:  packages.LoadSyntax,
}
pkgs, err := packages.Load(cfg, flag.Args()...)
if err != nil {
	fmt.Fprintf(os.Stderr, "load: %v\n", err)
	os.Exit(1)
}
if packages.PrintErrors(pkgs) > 0 {
	os.Exit(1)
}
sort.Slice(pkgs, func(i, j int) bool {
	return pkgs[i].PkgPath < pkgs[j].PkgPath
})

for _, pkg := range pkgs {
	fmt.Printf("%v\n\tPkgPath: %v\n\tName: %v\n", pkg.ID, pkg.PkgPath, pkg.Name)
}

}

-- p/p.go --
package main

-- p/p_test.go --
package main_test

-- q/q.go --
package q

-- q/q_test.go --
package q_test

which gives the output:

> go run . ./p ./q
[stdout]
mod.com/p
        PkgPath: mod.com/p
        Name: main
mod.com/p [mod.com/p.test]
        PkgPath: mod.com/p
        Name: main
mod.com/p.test
        PkgPath: mod.com/p.test
        Name: main
mod.com/p_test [mod.com/p.test]
        PkgPath: mod.com/p_test
        Name: main_test
mod.com/q
        PkgPath: mod.com/q
        Name: q
mod.com/q.test
        PkgPath: mod.com/q.test
        Name: main
mod.com/q_test [mod.com/q.test]
        PkgPath: mod.com/q_test
        Name: q_test

> go list -test -f '{{.ImportPath}}{{print "\n\t"}}Name: {{.Name}}{{print "\n\t"}}ForTest: {{.ForTest}}' ./p
[stdout]
mod.com/p
        Name: main
        ForTest:
mod.com/p.test
        Name: main
        ForTest:
mod.com/p [mod.com/p.test]
        Name: main
        ForTest: mod.com/p
mod.com/p_test [mod.com/p.test]
        Name: main_test
        ForTest: mod.com/p

> go list -test -f '{{.ImportPath}}{{print "\n\t"}}Name: {{.Name}}{{print "\n\t"}}ForTest: {{.ForTest}}' ./q
[stdout]
mod.com/q
        Name: q
        ForTest:
mod.com/q.test
        Name: main
        ForTest:
mod.com/q
        Name: q
        ForTest:
mod.com/q_test [mod.com/q.test]
        Name: q_test
        ForTest: mod.com/q

My conclusion based on this is:

  • having two packages being returned that are indistinguishable by PkgPath and Name is not correct; they are different packages. Yes the opaque ID distinguishes them and it encodes the missing ForTest information, but we're telling people not to use the ID for the latter. Can someone remind me why PkgPath is the mangled form of .ImportPath from go list?
  • the ForTest information is useful and should be retained (it could be a type *go/packages.Package field, i.e. the resolved package). If this concept only exists in the go world, then it will not be populated for Blaze etc

Thoughts?

@matloob
Copy link
Contributor

matloob commented Feb 14, 2019

PkgPath is supposed to be the path of the package in a build. In each build, there will only be one package with a given package path, and that's the path of the package given by the reflect package, etc. It's extracted from .ImportPath because that's the only way to get it from go list, and we didn't want to add a mostly redundant field.

I don't think adding a field that's only populated in the go list world is a good idea. It means that tools that rely on it won't work outside that world.

I think we need a clear definition of what we're classifying as "test" and "non-test" that can apply for all build systems.

@adonovan
Copy link
Member

PkgPath is supposed to be the path of the package in a build. In each build, there will only be one package with a given package path,

I think we retreated from documenting this claim because it is not always true: main packages do not have unique PkgPaths, for one. There may be other examples.

@myitcv
Copy link
Member

myitcv commented Feb 14, 2019

PkgPath is supposed to be the path of the package in a build. In each build, there will only be one package with a given package path

To add to @adonovan's comment, the concrete example of where this breaks down is in the output above:

mod.com/p
        PkgPath: mod.com/p
        Name: main
mod.com/p [mod.com/p.test]
        PkgPath: mod.com/p
        Name: main
mod.com/p.test
        PkgPath: mod.com/p.test
        Name: main
mod.com/p_test [mod.com/p.test]
        PkgPath: mod.com/p_test
        Name: main_test

If that's the goal of PkgPath then I think it needs to be reconsidered.

I think we need a clear definition of what we're classifying as "test" and "non-test" that can apply for all build systems.

Alternatively would could identify the one package (in a directory in go terms, not sure how this maps to other build systems) that can be addressed via its import path:

mod.com/p
        ImportPath: mod.com/p
        PkgPath: mod.com/p
        Name: main
mod.com/p [mod.com/p.test]
        PkgPath: mod.com/p
        Name: main
mod.com/p.test
        PkgPath: mod.com/p.test
        Name: main
mod.com/p_test [mod.com/p.test]
        PkgPath: mod.com/p_test
        Name: main_test

You could either add a separate ImportPath field as indicated, or make PkgPath equal to the current value of ID, i.e. the go list reported .ImportPath.

Regardless of whether we add ForTest of the ImportPath solution, I don't think the value of PkgPath is correct/useful.

@matloob
Copy link
Contributor

matloob commented Feb 14, 2019

I think maybe I miscommunicated? By "in a build" I meant that each binary or library with transitive dependencies can contain at most one package with a given path. And go/packages and go list can return multiple package with a given path, but they won't both appear in a given binary or test, or any (library + transitive dependency) set.

Am I wrong about that? There can only be one main in each transitive dependency set, right? And mod.com/p and mod.com/p [mod.com/p.test] can't both appear in a transitive dependency set, right?

myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.
myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 15, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 17, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 17, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 17, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 18, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 19, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 20, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 20, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 20, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 20, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 21, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 21, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
@myitcv
Copy link
Member

myitcv commented Feb 22, 2019

@matloob and I followed up offline on this, so sharing those thoughts/findings back here (please correct any mistakes, @matloob)

For some context

In the context of testing, cmd/go allows for a single test variant of a package, a "scope" defined by the package files plus non-xtest _test.go files (modulo build tags). The xtest package is in effect another package scope entirely. Note that only the package itself can be addressed by cmd/go via its import path; the test variant, xtest and .test main package can only be accessed via the -test flag to go list.

In the Blaze world there can be multiple test variants, and these are defined by rules. But like the cmd/go build system, all of these packages are, in effect the "same package" from a .PkgPath perspective. Hence the .ImportPath solution discussed in #27910 (comment) would only serve to identify the package itself, but would not allows us to distinguish between the various test variants (granted in the cmd/go world it would by virtue of there being just one such package).

The .ID of a *go/packages.Package uniquely identifies these variants (that share the same .PkgPath). The .ID value is currently provided by the build system, is opaque, and not guaranteed to be stable.

Returning the conversation to the cmd/go world which, as mentioned, is simpler by virtue of there being just a single test variant. Just to reiterate the current status quo: the .PkgPath and .Name for a package and its test variant are the same; the xtest's .PkgPath and .Name differ from the other two package and hence we have no trouble identifying the xtest package.

The question still remains therefore, how to distinguish the test variant from the package itself?

We discussed two options, having first identified the (at most) two packages in the the slice returned by go/packages.Load(conf, ".") that share .PkgPath and .Name:

  1. seeing whether a package's .GoFiles contains _test.go files; this will be the test variant
  2. being given the package .ID

The first of these points builds on @bcmills's suggestions in #27910 (comment). This satisfies our requirement.

The second is interesting. Taking the specific example of go generate, it's entirely conceivable the package .ID value could be passed in the environment for a generator. Whilst a call to go/packages.Load(conf, ".") would still be required (because the test variant cannot be addressed in the cmd/go world), we would then be able to unambiguously identify the single test variant.

Whilst the first of these solutions seems to work in the cmd/go world, it is probably not a solution that transfers (well) to the Blaze world.

The second solution requires that *go/packages.Package.ID values become stable, which is something @matloob et al are going to consider.

myitcv added a commit to myitcv/x that referenced this issue Feb 26, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 26, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 26, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 26, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
myitcv added a commit to myitcv/x that referenced this issue Feb 27, 2019
We need to fight go/packages a bit to find the right package to use for
type information (see golang/go#27910 for more
details).

Also, our logic for surfacing embedded fields where the embedded type
had a valid go/types.Type was incorrect. It assumed that all such types
must be external to the package being generated, and hence required that
all fields be exported. However, it is possible for a package-local type
to fully type check and therefore be valid. In this case, non-exported
fields _are_ valid for surfacing.

Also a small fix in cmd/gg whereby xtest packages were incorrectly being
considered as not Done() because they were missing a hash (which they
will never have). Added a failsafe that errors in case any work was left
un-Done
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@adonovan
Copy link
Member

adonovan commented Aug 7, 2023

[...years pass...]

Returning the conversation to the cmd/go world which, as mentioned, is simpler by virtue of there being just a single test variant.

Even with the go command, there may be numerous "intermediate test variants". Consider a package P, and an external test package P_test that has significant extra dependencies. (A hypothetical example would be P=net/url if P_test were to depend upon a complete web server, net/http.) When building the test executable for P, every intermediate package that is "above" P but "below" P_test must be compiled in a variant mode in which any import of P is replaced by "P [P.test]", that is, the test variant of P, which may contain extra files. (FWIW, these intermediate test variants have the potential to greatly increase the size of the overall graph; gopls goes to some lengths to ignore them in most cases.)

This subtlety is a recurring source of confusion that we should clarify in both "go help list" and go/packages, however we resolve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants