From 308f0c7c09fc9a2cba898a7445d92edc31bf2a85 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Wed, 26 Sep 2018 16:34:31 -0400
Subject: [PATCH] go/analysis: revert UsesFacts to FactTypes

Forcing clients to register their Fact types with gob made for an
unfriendly API. Now the driver is again responsible for doing it.

The FactTypes field is now a slice of Fact values (not reflect.Types)
used only for their dynamic type, which is slightly more convenient.

Change-Id: I01219edb24bd2371ba642bb56508aa80c19a9b61
Reviewed-on: https://go-review.googlesource.com/137836
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
---
 go/analysis/analysis.go                 | 12 ++++++---
 go/analysis/internal/checker/checker.go |  4 +--
 go/analysis/passes/pkgfact/pkgfact.go   |  2 +-
 go/analysis/validate.go                 | 34 ++++++++++++++++++++-----
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/go/analysis/analysis.go b/go/analysis/analysis.go
index 127f1d3f..8fa329c1 100644
--- a/go/analysis/analysis.go
+++ b/go/analysis/analysis.go
@@ -11,6 +11,9 @@
 //
 // The principal data types of this package are structs, not interfaces,
 // to permit later addition of optional fields as the API evolves.
+//
+// THIS INTERFACE IS EXPERIMENTAL AND SUBJECT TO CHANGE.
+// We aim to finalize it by November 2018.
 package analysis
 
 import (
@@ -67,14 +70,15 @@ type Analyzer struct {
 	// ResultType is the type of the optional result of the Run function.
 	ResultType reflect.Type
 
-	// UsesFacts indicates that this analyzer produces and consumes Facts.
+	// FactTypes indicates that this analyzer imports and exports
+	// Facts of the specified concrete types.
 	// An analyzer that uses facts may assume that its import
 	// dependencies have been similarly analyzed before it runs.
-	// Facts are pointers.
+	// Facts must be pointers.
 	//
-	// UsesFacts establishes a "vertical" dependency between
+	// FactTypes establishes a "vertical" dependency between
 	// analysis passes (same analyzer, different packages).
-	UsesFacts bool
+	FactTypes []Fact
 }
 
 func (a *Analyzer) String() string { return a.Name }
diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go
index d4388c53..d9647c0e 100644
--- a/go/analysis/internal/checker/checker.go
+++ b/go/analysis/internal/checker/checker.go
@@ -188,7 +188,7 @@ func analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action
 
 			// An analysis that consumes/produces facts
 			// must run on the package's dependencies too.
-			if a.UsesFacts {
+			if len(a.FactTypes) > 0 {
 				paths := make([]string, 0, len(pkg.Imports))
 				for path := range pkg.Imports {
 					paths = append(paths, path)
@@ -374,7 +374,7 @@ func needFacts(analyzers []*analysis.Analyzer) bool {
 		q = q[1:]
 		if !seen[a] {
 			seen[a] = true
-			if a.UsesFacts {
+			if len(a.FactTypes) > 0 {
 				return true
 			}
 			q = append(q, a.Requires...)
diff --git a/go/analysis/passes/pkgfact/pkgfact.go b/go/analysis/passes/pkgfact/pkgfact.go
index 4c98cb7a..ddb18341 100644
--- a/go/analysis/passes/pkgfact/pkgfact.go
+++ b/go/analysis/passes/pkgfact/pkgfact.go
@@ -35,7 +35,7 @@ var Analyzer = &analysis.Analyzer{
 	Name:       "pkgfact",
 	Doc:        "gather name/value pairs from constant declarations",
 	Run:        run,
-	UsesFacts:  true, // *pairsFact
+	FactTypes:  []analysis.Fact{new(pairsFact)},
 	ResultType: reflect.TypeOf(map[string]string{}),
 }
 
diff --git a/go/analysis/validate.go b/go/analysis/validate.go
index e91729ca..ff176e48 100644
--- a/go/analysis/validate.go
+++ b/go/analysis/validate.go
@@ -2,38 +2,60 @@ package analysis
 
 import (
 	"fmt"
+	"reflect"
 	"unicode"
 )
 
 // Validate reports an error if any of the analyzers are misconfigured.
 // Checks include:
 // - that the name is a valid identifier;
-// - that analysis names are unique;
-// - that the Requires graph is acylic.
+// - that analyzer names are unique;
+// - that the Requires graph is acylic;
+// - that analyzer fact types are unique;
+// - that each fact type is a pointer.
 func Validate(analyzers []*Analyzer) error {
 	names := make(map[string]bool)
 
+	// Map each fact type to its sole generating analyzer.
+	factTypes := make(map[reflect.Type]*Analyzer)
+
 	// Traverse the Requires graph, depth first.
 	color := make(map[*Analyzer]uint8) // 0=white 1=grey 2=black
 	var visit func(a *Analyzer) error
 	visit = func(a *Analyzer) error {
 		if a == nil {
-			return fmt.Errorf("nil *Analysis")
+			return fmt.Errorf("nil *Analyzer")
 		}
 		if color[a] == 0 { // white
 			color[a] = 1 // grey
 
 			// names
 			if !validIdent(a.Name) {
-				return fmt.Errorf("invalid analysis name %q", a)
+				return fmt.Errorf("invalid analyzer name %q", a)
 			}
 			if names[a.Name] {
-				return fmt.Errorf("duplicate analysis name %q", a)
+				return fmt.Errorf("duplicate analyzer name %q", a)
 			}
 			names[a.Name] = true
 
 			if a.Doc == "" {
-				return fmt.Errorf("analysis %q is undocumented", a)
+				return fmt.Errorf("analyzer %q is undocumented", a)
+			}
+
+			// fact types
+			for _, f := range a.FactTypes {
+				if f == nil {
+					return fmt.Errorf("analyzer %s has nil FactType", a)
+				}
+				t := reflect.TypeOf(f)
+				if prev := factTypes[t]; prev != nil {
+					return fmt.Errorf("fact type %s registered by two analyzers: %v, %v",
+						t, a, prev)
+				}
+				if t.Kind() != reflect.Ptr {
+					return fmt.Errorf("%s: fact type %s is not a pointer", a, t)
+				}
+				factTypes[t] = a
 			}
 
 			// recursion