go/packages: smooth API documentation for release

Change-Id: Ia3d06a320228831115eb0e0e99d45a84a812da03
Reviewed-on: https://go-review.googlesource.com/129636
Reviewed-by: Ian Cottrell <iancottrell@google.com>

go/packages/doc.go

@@ -6,13 +6,14 @@
 Package packages loads Go packages for inspection and analysis.
 
 NOTE: THIS PACKAGE IS NOT YET READY FOR WIDESPREAD USE:
- - The interface is still being revised and is likely to change.
+ - The interface is still being revised and minor changes are likely.
- - The implementation depends on the Go 1.11 go command.
+ - The implementation depends on the Go 1.11 go command;
+   support for earlier versions will be added soon.
  - We intend to finalize the API before Go 1.11 is released.
 
 The Load function takes as input a list of patterns and return a list of Package
 structs describing individual packages matched by those patterns.
-The LoadMode controls the amounts of detail about the loaded packages.
+The LoadMode controls the amount of detail in the loaded packages.
 
 The patterns are used as arguments to the underlying build tool,
 such as the go command or Bazel, and are interpreted according to
@@ -40,18 +41,17 @@ can match multiple packages and that a package might be matched by
 multiple patterns: in general it is not possible to determine which
 packages correspond to which patterns.
 
-Note that the list returned by Load (LoadAllSyntax in this case)
+Note that the list returned by Load contains only the packages matched
-only contains the packages matched by the patterns. Their dependencies
+by the patterns. Their dependencies can be found by walking the import
-can be found by walking the import graph using the Imports fields.
+graph using the Imports fields.
 
-The Load function can be configured by passing a non-nil Config struct as
+The Load function can be configured by passing a pointer to a Config as
-the first argument. If you pass nil for the Config Load will
+the first argument. A nil Config is equivalent to the zero Config, which
-run in LoadAllSyntax mode, collecting the maximal amount of information
+causes Load to run in LoadFiles mode, collecting minimal information.
-it can.
 See the documentation for type Config for details.
 
-As noted earlier, the Config.Mode controls increasing amounts of detail
+As noted earlier, the Config.Mode controls the amount of detail
-about the loaded packages, with each mode returning all the data of the
+reported about the loaded packages, with each mode returning all the data of the
 previous mode with some extra added. See the documentation for type LoadMode
 for details.
 
@@ -114,12 +114,6 @@ about one package without visiting all its dependencies too, so there is
 no additional asymptotic cost to providing transitive information.
 (This property might not be true of a hypothetical 5th build system.)
 
-This package provides no parse-but-don't-typecheck operation because most tools
-that need only untyped syntax (such as gofmt, goimports, and golint)
-seem not to care about any files other than the ones they are directly
-instructed to look at.  Also, it is trivial for a client to supplement
-this functionality on top of a Metadata query.
-
 In calls to TypeCheck, all initial packages, and any package that
 transitively depends on one of them, must be loaded from source.
 Consider A->B->C->D->E: if A,C are initial, A,B,C must be loaded from
@@ -169,15 +163,13 @@ type-check again. This two-phase approach had four major problems:
    in several times in sequence as is now possible in WholeProgram mode.
    (TypeCheck mode has a similar one-shot restriction for a different reason.)
 
-Early drafts of this package supported "multi-shot" operation
+Early drafts of this package supported "multi-shot" operation.
-in the Metadata and WholeProgram modes, although this feature is not exposed
-through the API and will likely be removed.
 Although it allowed clients to make a sequence of calls (or concurrent
 calls) to Load, building up the graph of Packages incrementally,
 it was of marginal value: it complicated the API
 (since it allowed some options to vary across calls but not others),
 it complicated the implementation,
-it cannot be made to work in TypeCheck mode, as explained above,
+it cannot be made to work in Types mode, as explained above,
 and it was less efficient than making one combined call (when this is possible).
 Among the clients we have inspected, none made multiple calls to load
 but could not be easily and satisfactorily modified to make only a single call.
@@ -227,10 +219,6 @@ Questions & Tasks
   failed builds, import failures, import cycles, and so on, in a call to
   Load?
 
-- Do we need a GeneratedBy map that maps the name of each generated Go
-  source file in GoFiles to that of the original file, if known, or "" otherwise?
-  Or are //line directives and "Generated" comments in those files enough?
-
 - Support bazel, blaze, and go1.10 list, not just go1.11 list.
 
 - Handle (and test) various partial success cases, e.g.
@@ -252,18 +240,4 @@ Questions & Tasks
   order. I suspect this is due to the breadth-first resolution now used
   by go/types. Is that a bug? Discuss with gri.
 
-- https://github.com/golang/go/issues/25980 causes these commands to crash:
-  $ GOPATH=/none ./gopackages -all all
-  due to:
-  $ GOPATH=/none go list -e -test -json all
-  and:
-  $ go list -e -test ./relative/path
-
-- Modify stringer to use go/packages, perhaps initially under flag control.
-
-- Bug: "gopackages fmt a.go" doesn't produce an error.
-
-- If necessary, add back an IsTest boolean or expose ForTests on the Package struct.
-  IsTest was removed because we couldn't agree on a useful definition.
-
 */

go/packages/packages.go

@@ -21,16 +21,14 @@ import (
 	"golang.org/x/tools/go/gcexportdata"
 )
 
-// A LoadMode specifies the amount of detail to return when loading packages.
+// A LoadMode specifies the amount of detail to return when loading.
-// The modes are all strictly additive, as you go through the list it increases
+// Higher-numbered modes cause Load to return more information,
-// the amount of information available to you, but may also increase the cost
+// but may be slower. Load may return more information than requested.
-// of collecting the information.
-// Load is always allowed to return more information than requested.
 type LoadMode int
 
 const (
 	// LoadFiles finds the packages and computes their source file lists.
-	// Package fields: ID, Name, Errors, GoFiles, OtherFiles.
+	// Package fields: ID, Name, Errors, GoFiles, and OtherFiles.
 	LoadFiles LoadMode = iota
 
 	// LoadImports adds import information for each package
@@ -38,25 +36,26 @@ const (
 	// Package fields added: Imports.
 	LoadImports
 
-	// LoadTypes adds type information for the package's exported symbols for
+	// LoadTypes adds type information for package-level
-	// packages matching the patterns.
+	// declarations in the packages matching the patterns.
-	// Package fields added: Types, Fset, IllTyped.
+	// Package fields added: Types, Fset, and IllTyped.
-	// This will use the type information provided by the build system if
+	// This mode uses type information provided by the build system when
-	// possible, and the ExportFile field may be filled in.
+	// possible, and may fill in the ExportFile field.
 	LoadTypes
 
 	// LoadSyntax adds typed syntax trees for the packages matching the patterns.
-	// Package fields added: Syntax, TypesInfo, for direct pattern matches only.
+	// Package fields added: Syntax, and TypesInfo, for direct pattern matches only.
 	LoadSyntax
 
 	// LoadAllSyntax adds typed syntax trees for the packages matching the patterns
 	// and all dependencies.
-	// Package fields added: Types, Fset, IllTyped, Syntax, TypesInfo, for all
+	// Package fields added: Types, Fset, Illtyped, Syntax, and TypesInfo,
-	// packages in import graph.
+	// for all packages in the import graph.
 	LoadAllSyntax
 )
 
 // An Config specifies details about how packages should be loaded.
+// The zero value is a valid configuration.
 // Calls to Load do not modify this struct.
 type Config struct {
 	// Mode controls the level of information returned for each package.
@@ -68,14 +67,14 @@ type Config struct {
 	// If Context is nil, the load cannot be cancelled.
 	Context context.Context
 
-	// Dir is the directory in which to run the build system tool
+	// Dir is the directory in which to run the build system's query tool
 	// that provides information about the packages.
 	// If Dir is empty, the tool is run in the current directory.
 	Dir string
 
-	// Env is the environment to use when invoking the build system tool.
+	// Env is the environment to use when invoking the build system's query tool.
 	// If Env is nil, the current environment is used.
-	// Like in os/exec's Cmd, only the last value in the slice for
+	// As in os/exec's Cmd, only the last value in the slice for
 	// each environment key is used. To specify the setting of only
 	// a few variables, append to the current environment, as in:
 	//
@@ -84,21 +83,19 @@ type Config struct {
 	Env []string
 
 	// Flags is a list of command-line flags to be passed through to
-	// the underlying query tool.
+	// the build system's query tool.
 	Flags []string
 
-	// Error is called for each error encountered during package loading.
+	// Error is called for each error encountered during parsing and type-checking.
 	// It must be safe to call Error simultaneously from multiple goroutines.
-	// In addition to calling Error, the loader will record each error
+	// In addition to calling Error, the loader records each error
 	// in the corresponding Package's Errors list.
-	// If Error is nil, the loader will print errors to os.Stderr.
+	// If Error is nil, the loader prints errors to os.Stderr.
-	// To disable printing of errors, set opt.Error = func(error){}.
+	// To disable printing of errors, set opt.Error = func(error) {}.
-	// TODO(rsc): What happens in the Metadata loader? Currently nothing.
 	Error func(error)
 
-	// Fset is the token.FileSet to use when parsing source files or
+	// Fset provides source position information for syntax trees and types.
-	// type information provided by the build system.
+	// If Fset is nil, the loader will create a new FileSet.
-	// If Fset is nil, the loader will create one.
 	Fset *token.FileSet
 
 	// ParseFile is called to read and parse each file
@@ -176,7 +173,7 @@ func defaultDriver(cfg *Config, patterns ...string) (*driverResponse, error) {
 	return driver(cfg, patterns...)
 }
 
-// A Package describes a single loaded Go package.
+// A Package describes a loaded Go package.
 type Package struct {
 	// ID is a unique identifier for a package,
 	// in a syntax provided by the underlying build system.
@@ -189,14 +186,16 @@ type Package struct {
 	// Name is the package name as it appears in the package source code.
 	Name string
 
-	// This is the package path as used by the types package.
+	// PkgPath is the package path as used by the go/types package.
-	// This is used to map entries in the type information back to the package
-	// they come from.
 	PkgPath string
 
-	// Errors lists any errors encountered while loading the package.
+	// Errors contains any errors encountered while parsing or type-checking the package.
-	// TODO(rsc): Say something about the errors or at least their Strings,
+	// Possible error types include *scanner.ErrorList and types.Error,
-	// as far as file:line being at the beginning and so on.
+	// whose fields provide structured position information.
+	// Error strings are typically of the form "file:line: message" or
+	// "file:line:col: message".
+	// TODO(adonovan): export packageError as packages.Error
+	// and add that type to the list of structured errors.
 	Errors []error
 
 	// GoFiles lists the absolute file paths of the package's Go source files.
@@ -211,15 +210,15 @@ type Package struct {
 	// including assembly, C, C++, Fortran, Objective-C, SWIG, and so on.
 	OtherFiles []string
 
-	// ExportFile is the absolute path to a file containing the type information
+	// ExportFile is the absolute path to a file containing type
-	// provided by the build system.
+	// information for the package as provided by the build system.
 	ExportFile string
 
 	// Imports maps import paths appearing in the package's Go source files
 	// to corresponding loaded Packages.
 	Imports map[string]*Package
 
-	// Types is the type information for the package.
+	// Types provides type information for the package.
 	// Modes LoadTypes and above set this field for packages matching the
 	// patterns; type information for dependencies may be missing or incomplete.
 	// Mode LoadSyntaxAll sets this field for all packages, including dependencies.
@@ -233,13 +232,13 @@ type Package struct {
 	// It is set only when Types is set.
 	IllTyped bool
 
-	// Syntax is the package's syntax trees, for the files listed in GoFiles.
+	// Syntax is the package's syntax trees, for the files listed in CompiledGoFiles.
 	//
 	// Mode LoadSyntax sets this field for packages matching the patterns.
 	// Mode LoadSyntaxAll sets this field for all packages, including dependencies.
 	Syntax []*ast.File
 
-	// TypesInfo is the type-checking results for the package's syntax trees.
+	// TypesInfo provides type information about the package's syntax trees.
 	// It is set only when Syntax is set.
 	TypesInfo *types.Info
 }
@@ -276,6 +275,7 @@ type flatPackage struct {
 // The imports are written out as just a map of path to package id.
 // The errors are written using a custom type that tries to preserve the
 // structure of error types we know about.
+//
 // This method exists to enable support for additional build systems.  It is
 // not intended for use by clients of the API and we may change the format.
 func (p *Package) MarshalJSON() ([]byte, error) {
@@ -669,8 +669,8 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) {
 		for _, f := range lpkg.Syntax {
 			for _, imp := range f.Imports {
 				if imp.Path.Value == `"C"` {
-					appendError(fmt.Errorf(`%s: import "C" ignored`,
+					err := types.Error{Fset: ld.Fset, Pos: imp.Pos(), Msg: `import "C" ignored`}
-						lpkg.Fset.Position(imp.Pos())))
+					appendError(err)
 					break outer
 				}
 			}