diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index e4e82ce7..312dfd5b 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -4,13 +4,16 @@ package analysistest import ( "fmt" "go/token" + "go/types" "io/ioutil" "log" "os" "path/filepath" "regexp" + "sort" "strconv" "strings" + "text/scanner" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/internal/checker" @@ -60,8 +63,33 @@ type Testing interface { // Run applies an analysis to each named package. // It loads each package from the specified GOPATH-style project // directory using golang.org/x/tools/go/packages, runs the analysis on -// it, and checks that each the analysis generates the diagnostics -// specified by 'want "..."' comments in the package's source files. +// it, and checks that each the analysis emits the expected diagnostics +// and facts specified by the contents of '// want ...' comments in the +// package's source files. +// +// An expectation of a Diagnostic is specified by a string literal +// containing a regular expression that must match the diagnostic +// message. For example: +// +// fmt.Printf("%s", 1) // want `cannot provide int 1 to %s` +// +// An expectation of a Fact associated with an object is specified by +// 'name:"pattern"', where name is the name of the object, which must be +// declared on the same line as the comment, and pattern is a regular +// expression that must match the string representation of the fact, +// fmt.Sprint(fact). For example: +// +// func panicf(format string, args interface{}) { // want panicf:"printfWrapper" +// +// Package facts are specified by the name "package". +// +// A single 'want' comment may contain a mixture of diagnostic and fact +// expectations, including multiple facts about the same object: +// +// // want "diag" "diag2" x:"fact1" x:"fact2" y:"fact3" +// +// Unexpected diagnostics and facts, and unmatched expectations, are +// reported as errors to the Testing. // // You may wish to call this function from within a (*testing.T).Run // subtest to ensure that errors have adequate contextual description. @@ -76,13 +104,13 @@ func Run(t Testing, dir string, a *analysis.Analyzer, pkgnames ...string) { continue } - pass, diagnostics, err := checker.Analyze(pkg, a) + pass, diagnostics, facts, err := checker.Analyze(pkg, a) if err != nil { t.Errorf("analyzing %s: %v", pkgname, err) continue } - checkDiagnostics(t, dir, pass, diagnostics) + check(t, dir, pass, diagnostics, facts) } } @@ -95,13 +123,6 @@ func loadPackage(dir, pkgpath string) (*packages.Package, error) { // However there is no easy way to make go/packages to consume // a list of packages we generate and then do the parsing and // typechecking, though this feature seems to be a recurring need. - // - // It is possible to write a custom driver, but it's fairly - // involved and requires setting a global (environment) variable. - // - // Also, using the "go list" driver will probably not work in google3. - // - // TODO(adonovan): extend go/packages to allow bypassing the driver. cfg := &packages.Config{ Mode: packages.LoadAllSyntax, @@ -113,6 +134,9 @@ func loadPackage(dir, pkgpath string) (*packages.Package, error) { if err != nil { return nil, err } + if packages.PrintErrors(pkgs) > 0 { + return nil, fmt.Errorf("loading %s failed", pkgpath) + } if len(pkgs) != 1 { return nil, fmt.Errorf("pattern %q expanded to %d packages, want 1", pkgpath, len(pkgs)) @@ -121,57 +145,177 @@ func loadPackage(dir, pkgpath string) (*packages.Package, error) { return pkgs[0], nil } -// checkDiagnostics inspects an analysis pass on which the analysis has -// already been run, and verifies that all reported diagnostics match those -// specified by 'want "..."' comments in the package's source files, -// which must have been parsed with comments enabled. Surplus diagnostics -// and unmatched expectations are reported as errors to the Testing. -func checkDiagnostics(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis.Diagnostic) { +// check inspects an analysis pass on which the analysis has already +// been run, and verifies that all reported diagnostics and facts match +// specified by the contents of "// want ..." comments in the package's +// source files, which must have been parsed with comments enabled. +func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis.Diagnostic, facts map[types.Object][]analysis.Fact) { + // Read expectations out of comments. type key struct { file string line int } - wantErrs := make(map[key]*regexp.Regexp) + want := make(map[key][]expectation) for _, f := range pass.Files { for _, c := range f.Comments { posn := pass.Fset.Position(c.Pos()) sanitize(gopath, &posn) text := strings.TrimSpace(c.Text()) - if !strings.HasPrefix(text, "want") { - continue + + // Any comment starting with "want" is treated + // as an expectation, even without following whitespace. + if rest := strings.TrimPrefix(text, "want"); rest != text { + expects, err := parseExpectations(rest) + if err != nil { + t.Errorf("%s: in 'want' comment: %s", posn, err) + continue + } + if false { + log.Printf("%s: %v", posn, expects) + } + want[key{posn.Filename, posn.Line}] = expects } - text = strings.TrimSpace(text[len("want"):]) - pattern, err := strconv.Unquote(text) - if err != nil { - t.Errorf("%s: in 'want' comment: %v", posn, err) - continue + } + } + + checkMessage := func(posn token.Position, kind, name, message string) { + sanitize(gopath, &posn) + k := key{posn.Filename, posn.Line} + expects := want[k] + var unmatched []string + for i, exp := range expects { + if exp.kind == kind && exp.name == name { + if exp.rx.MatchString(message) { + // matched: remove the expectation. + expects[i] = expects[len(expects)-1] + expects = expects[:len(expects)-1] + want[k] = expects + return + } + unmatched = append(unmatched, fmt.Sprintf("%q", exp.rx)) } - rx, err := regexp.Compile(pattern) - if err != nil { - t.Errorf("%s: %v", posn, err) - continue - } - wantErrs[key{posn.Filename, posn.Line}] = rx + } + if unmatched == nil { + t.Errorf("%v: unexpected %s: %v", posn, kind, message) + } else { + t.Errorf("%v: %s %q does not match pattern %s", + posn, kind, message, strings.Join(unmatched, " or ")) } } // Check the diagnostics match expectations. for _, f := range diagnostics { posn := pass.Fset.Position(f.Pos) - sanitize(gopath, &posn) - rx, ok := wantErrs[key{posn.Filename, posn.Line}] - if !ok { - t.Errorf("%v: unexpected diagnostic: %v", posn, f.Message) - continue + checkMessage(posn, "diagnostic", "", f.Message) + } + + // Check the facts match expectations. + // Report errors in lexical order for determinism. + var objects []types.Object + for obj := range facts { + objects = append(objects, obj) + } + sort.Slice(objects, func(i, j int) bool { + return objects[i].Pos() < objects[j].Pos() + }) + for _, obj := range objects { + var posn token.Position + var name string + if obj != nil { + // Object facts are reported on the declaring line. + name = obj.Name() + posn = pass.Fset.Position(obj.Pos()) + } else { + // Package facts are reported at the start of the file. + name = "package" + posn = pass.Fset.Position(pass.Files[0].Pos()) + posn.Line = 1 } - delete(wantErrs, key{posn.Filename, posn.Line}) - if !rx.MatchString(f.Message) { - t.Errorf("%v: diagnostic %q does not match pattern %q", posn, f.Message, rx) + + for _, fact := range facts[obj] { + checkMessage(posn, "fact", name, fmt.Sprint(fact)) } } - for key, rx := range wantErrs { - t.Errorf("%s:%d: expected diagnostic matching %q", key.file, key.line, rx) + + // Reject surplus expectations. + var surplus []string + for key, expects := range want { + for _, exp := range expects { + err := fmt.Sprintf("%s:%d: no %s was reported matching %q", key.file, key.line, exp.kind, exp.rx) + surplus = append(surplus, err) + } + } + sort.Strings(surplus) + for _, err := range surplus { + t.Errorf("%s", err) + } +} + +type expectation struct { + kind string // either "fact" or "diagnostic" + name string // name of object to which fact belongs, or "package" ("fact" only) + rx *regexp.Regexp +} + +func (ex expectation) String() string { + return fmt.Sprintf("%s %s:%q", ex.kind, ex.name, ex.rx) // for debugging +} + +// parseExpectations parses the content of a "// want ..." comment +// and returns the expections, a mixture of diagnostics ("rx") and +// facts (name:"rx"). +func parseExpectations(text string) ([]expectation, error) { + var scanErr string + sc := new(scanner.Scanner).Init(strings.NewReader(text)) + sc.Error = func(s *scanner.Scanner, msg string) { + scanErr = msg // e.g. bad string escape + } + sc.Mode = scanner.ScanIdents | scanner.ScanStrings | scanner.ScanRawStrings + + scanRegexp := func(tok rune) (*regexp.Regexp, error) { + if tok != scanner.String && tok != scanner.RawString { + return nil, fmt.Errorf("got %s, want regular expression", + scanner.TokenString(tok)) + } + pattern, _ := strconv.Unquote(sc.TokenText()) // can't fail + return regexp.Compile(pattern) + } + + var expects []expectation + for { + tok := sc.Scan() + switch tok { + case scanner.String, scanner.RawString: + rx, err := scanRegexp(tok) + if err != nil { + return nil, err + } + expects = append(expects, expectation{"diagnostic", "", rx}) + + case scanner.Ident: + name := sc.TokenText() + tok = sc.Scan() + if tok != ':' { + return nil, fmt.Errorf("got %s after %s, want ':'", + scanner.TokenString(tok), name) + } + tok = sc.Scan() + rx, err := scanRegexp(tok) + if err != nil { + return nil, err + } + expects = append(expects, expectation{"fact", name, rx}) + + case scanner.EOF: + if scanErr != "" { + return nil, fmt.Errorf("%s", scanErr) + } + return expects, nil + + default: + return nil, fmt.Errorf("unexpected %s", scanner.TokenString(tok)) + } } } diff --git a/go/analysis/analysistest/analysistest_test.go b/go/analysis/analysistest/analysistest_test.go index 7217220b..30704c2a 100644 --- a/go/analysis/analysistest/analysistest_test.go +++ b/go/analysis/analysistest/analysistest_test.go @@ -17,12 +17,31 @@ func TestTheTest(t *testing.T) { filemap := map[string]string{"a/b.go": `package main func main() { - println("hello, world") // want "call of println" + // The expectation is ill-formed: + print() // want: "diagnostic" + print() // want foo"fact" + print() // want foo: + print() // want "\xZZ scan error" + + // A dignostic is reported at this line, but the expectation doesn't match: println("hello, world") // want "wrong expectation text" + + // An unexpected diagnostic is reported at this line: println() // trigger an unexpected diagnostic + + // No diagnostic is reported at this line: print() // want "unsatisfied expectation" - print() // want: "ill-formed 'want' comment" + + // OK + println("hello, world") // want "call of println" + + // OK (multiple expectations on same line) + println(); println() // want "call of println(...)" "call of println(...)" } + +// OK (facts and diagnostics on same line) +func println(...interface{}) { println() } // want println:"found" "call of println(...)" + `} dir, cleanup, err := analysistest.WriteFiles(filemap) if err != nil { @@ -35,10 +54,14 @@ func main() { analysistest.Run(t2, dir, findcall.Analyzer, "a") want := []string{ - `a/b.go:8:10: in 'want' comment: invalid syntax`, - `a/b.go:5:9: diagnostic "call of println(...)" does not match pattern "wrong expectation text"`, - `a/b.go:6:9: unexpected diagnostic: call of println(...)`, - `a/b.go:7: expected diagnostic matching "unsatisfied expectation"`, + `a/b.go:5:10: in 'want' comment: unexpected ":"`, + `a/b.go:6:10: in 'want' comment: got String after foo, want ':'`, + `a/b.go:7:10: in 'want' comment: got EOF, want regular expression`, + `a/b.go:8:10: in 'want' comment: illegal char escape`, + `a/b.go:11:9: diagnostic "call of println(...)" does not match pattern "wrong expectation text"`, + `a/b.go:14:9: unexpected diagnostic: call of println(...)`, + `a/b.go:11: no diagnostic was reported matching "wrong expectation text"`, + `a/b.go:17: no diagnostic was reported matching "unsatisfied expectation"`, } if !reflect.DeepEqual(got, want) { t.Errorf("got:\n%s\nwant:\n%s", diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index bbdaf2bd..68379073 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -153,10 +153,26 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) { // Analyze applies an analysis to a package (and their dependencies if // necessary) and returns the graph of results. // +// Facts about pkg are returned in a map keyed by object; package facts +// have a nil key. +// // It is exposed for use in testing. -func Analyze(pkg *packages.Package, a *analysis.Analyzer) (*analysis.Pass, []analysis.Diagnostic, error) { +func Analyze(pkg *packages.Package, a *analysis.Analyzer) (*analysis.Pass, []analysis.Diagnostic, map[types.Object][]analysis.Fact, error) { act := analyze([]*packages.Package{pkg}, []*analysis.Analyzer{a})[0] - return act.pass, act.diagnostics, act.err + + facts := make(map[types.Object][]analysis.Fact) + for key, fact := range act.objectFacts { + if key.obj.Pkg() == pkg.Types { + facts[key.obj] = append(facts[key.obj], fact) + } + } + for key, fact := range act.packageFacts { + if key.pkg == pkg.Types { + facts[nil] = append(facts[nil], fact) + } + } + + return act.pass, act.diagnostics, facts, act.err } func analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action { @@ -404,13 +420,13 @@ type action struct { } type objectFactKey struct { - types.Object - reflect.Type + obj types.Object + typ reflect.Type } type packageFactKey struct { - *types.Package - reflect.Type + pkg *types.Package + typ reflect.Type } func (act *action) String() string { @@ -538,9 +554,9 @@ func inheritFacts(act, dep *action) { // Filter out facts related to objects // that are irrelevant downstream // (equivalently: not in the compiler export data). - if !exportedFrom(key.Object, dep.pkg.Types) { + if !exportedFrom(key.obj, dep.pkg.Types) { if false { - log.Printf("%v: discarding %T fact from %s for %s: %s", act, fact, dep, key.Object, fact) + log.Printf("%v: discarding %T fact from %s for %s: %s", act, fact, dep, key.obj, fact) } continue } @@ -556,7 +572,7 @@ func inheritFacts(act, dep *action) { } if false { - log.Printf("%v: inherited %T fact for %s: %s", act, fact, key.Object, fact) + log.Printf("%v: inherited %T fact for %s: %s", act, fact, key.obj, fact) } act.objectFacts[key] = fact } @@ -578,7 +594,7 @@ func inheritFacts(act, dep *action) { } if false { - log.Printf("%v: inherited %T fact for %s: %s", act, fact, key.Package.Path(), fact) + log.Printf("%v: inherited %T fact for %s: %s", act, fact, key.pkg.Path(), fact) } act.packageFacts[key] = fact } diff --git a/go/analysis/passes/findcall/findcall.go b/go/analysis/passes/findcall/findcall.go index ce8a81dc..31f3f0b5 100644 --- a/go/analysis/passes/findcall/findcall.go +++ b/go/analysis/passes/findcall/findcall.go @@ -5,6 +5,7 @@ package findcall import ( "go/ast" + "go/types" "golang.org/x/tools/go/analysis" ) @@ -17,6 +18,7 @@ The findcall analysis reports calls to functions or methods of a particular name.`, Run: findcall, RunDespiteErrors: true, + FactTypes: []analysis.Fact{new(foundFact)}, } var name = "println" // -name flag @@ -44,5 +46,28 @@ func findcall(pass *analysis.Pass) (interface{}, error) { }) } + // Export a fact for each matching function. + // + // These facts are produced only to test the testing + // infrastructure in the analysistest package. + // They are not consumed by the findcall Analyzer + // itself, as would happen in a more realistic example. + for _, f := range pass.Files { + for _, decl := range f.Decls { + if decl, ok := decl.(*ast.FuncDecl); ok && decl.Name.Name == name { + if obj, ok := pass.TypesInfo.Defs[decl.Name].(*types.Func); ok { + pass.ExportObjectFact(obj, new(foundFact)) + } + } + } + } + return nil, nil } + +// foundFact is a fact associated with functions that match -name. +// We use it to exercise the fact machinery in tests. +type foundFact struct{} + +func (*foundFact) String() string { return "found" } +func (*foundFact) AFact() {} diff --git a/go/analysis/passes/pkgfact/pkgfact.go b/go/analysis/passes/pkgfact/pkgfact.go index ddb18341..34273c15 100644 --- a/go/analysis/passes/pkgfact/pkgfact.go +++ b/go/analysis/passes/pkgfact/pkgfact.go @@ -45,7 +45,8 @@ var Analyzer = &analysis.Analyzer{ // Elements are ordered by keys, which are unique. type pairsFact []string -func (*pairsFact) AFact() {} +func (f *pairsFact) AFact() {} +func (f *pairsFact) String() string { return "pairs(" + strings.Join(*f, ", ") + ")" } func run(pass *analysis.Pass) (interface{}, error) { result := make(map[string]string) diff --git a/go/analysis/passes/pkgfact/testdata/src/c/c.go b/go/analysis/passes/pkgfact/testdata/src/c/c.go index 83dac7ef..605abfdf 100644 --- a/go/analysis/passes/pkgfact/testdata/src/c/c.go +++ b/go/analysis/passes/pkgfact/testdata/src/c/c.go @@ -1,3 +1,5 @@ +// want package:`pairs\(audience="world", greeting="hello", pi=3.14159\)` + package c import _ "b" // want `audience="world" greeting="hello" pi=3.14159`