cmd/vulnreport,internal/symbols: remove logging from internal/symbols

Return errors instead of logging in internal/symbols functions.

Change-Id: I43efd1e27a97a8f6f42abcc361d647fb9eee8f4c
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/562196
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Maceo Thompson <[email protected]>
Commit-Queue: Tatiana Bradley <[email protected]>
Reviewed-by: Zvonimir Pavlinovic <[email protected]>

Author: tatianab

cmd/vulnreport/create.go

@@ -230,7 +230,7 @@ func reportFromAliases(ctx context.Context, id, modulePath string, aliases []str
 	}
 
 	if *populateSymbols {
-		if err := symbols.Populate(r, log.Err); err != nil {
+		if err := symbols.Populate(r); err != nil {
 			log.Warnf("could not auto-populate symbols: %s", err)
 		}
 	}

cmd/vulnreport/fix.go

@@ -137,7 +137,7 @@ func checkReportSymbols(r *report.Report) error {
 				log.Infof("%s: skipping symbol checks for package %s (reason: %q)\n", r.ID, p.Package, p.SkipFix)
 				continue
 			}
-			syms, err := symbols.Exported(m, p, log.Errf, log.Err)
+			syms, err := symbols.Exported(m, p)
 			if err != nil {
 				return fmt.Errorf("package %s: %w", p.Package, err)
 			}

cmd/vulnreport/symbols.go

@@ -7,7 +7,6 @@ package main
 import (
 	"context"
 
-	"golang.org/x/vulndb/cmd/vulnreport/log"
 	"golang.org/x/vulndb/internal/report"
 	"golang.org/x/vulndb/internal/symbols"
 )
@@ -31,7 +30,7 @@ func (s *symbolsCmd) run(ctx context.Context, filename string) (err error) {
 		return err
 	}
 
-	if err = symbols.Populate(r, log.Err); err != nil {
+	if err = symbols.Populate(r); err != nil {
 		return err
 	}
 

internal/symbols/exported_functions.go

@@ -6,6 +6,7 @@ package symbols
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"go/types"
 	"os"
@@ -23,7 +24,7 @@ import (
 
 // Exported returns a set of vulnerable symbols, in the vuln
 // db format, exported by a package p from the module m.
-func Exported(m *report.Module, p *report.Package, errf logf, errln logln) (_ []string, err error) {
+func Exported(m *report.Module, p *report.Package) (_ []string, err error) {
 	defer derrors.Wrap(&err, "Exported(%q, %q)", m.Module, p.Package)
 
 	cleanup, err := changeToTempDir()
@@ -36,9 +37,9 @@ func Exported(m *report.Module, p *report.Package, errf logf, errln logln) (_ []
 		cmd := exec.Command(name, arg...)
 		out, err := cmd.CombinedOutput()
 		if err != nil {
-			errln(string(out))
+			return fmt.Errorf("%s: %v\nout:\n%s", name, err, string(out))
 		}
-		return err
+		return nil
 	}
 
 	// This procedure was developed through trial and error finding a way
@@ -111,23 +112,8 @@ func Exported(m *report.Module, p *report.Package, errf logf, errln logln) (_ []
 	// Check to see that all symbols actually exist in the package.
 	// This should perhaps be a lint check, but lint doesn't
 	// load/typecheck packages at the moment, so do it here for now.
-	for _, sym := range p.Symbols {
-		if typ, method, ok := strings.Cut(sym, "."); ok {
-			n, ok := pkg.Types.Scope().Lookup(typ).(*types.TypeName)
-			if !ok {
-				errf("package %s: %v: type not found\n", p.Package, typ)
-				continue
-			}
-			m, _, _ := types.LookupFieldOrMethod(n.Type(), true, pkg.Types, method)
-			if m == nil {
-				errf("package %s: %v: method not found\n", p.Package, sym)
-			}
-		} else {
-			_, ok := pkg.Types.Scope().Lookup(typ).(*types.Func)
-			if !ok {
-				errf("package %s: %v: func not found\n", p.Package, typ)
-			}
-		}
+	if err := checkSymbols(pkg, p.Symbols); err != nil {
+		return nil, fmt.Errorf("invalid symbol(s):\n%w", err)
 	}
 
 	newsyms, err := exportedFunctions(pkg, m)
@@ -153,10 +139,28 @@ func Exported(m *report.Module, p *report.Package, errf logf, errln logln) (_ []
 	return newslice, nil
 }
 
-// TODO(tatianabradley): Refactor functions that use these to return
-// info needed to construct logs instead of logging directly.
-type logf func(format string, v ...any)
-type logln func(v ...any)
+func checkSymbols(pkg *packages.Package, symbols []string) error {
+	var errs []error
+	for _, sym := range symbols {
+		if typ, method, ok := strings.Cut(sym, "."); ok {
+			n, ok := pkg.Types.Scope().Lookup(typ).(*types.TypeName)
+			if !ok {
+				errs = append(errs, fmt.Errorf("%v: type not found", typ))
+				continue
+			}
+			m, _, _ := types.LookupFieldOrMethod(n.Type(), true, pkg.Types, method)
+			if m == nil {
+				errs = append(errs, fmt.Errorf("%v: method not found", sym))
+			}
+		} else {
+			_, ok := pkg.Types.Scope().Lookup(typ).(*types.Func)
+			if !ok {
+				errs = append(errs, fmt.Errorf("%v: func not found", typ))
+			}
+		}
+	}
+	return errors.Join(errs...)
+}
 
 // exportedFunctions returns a set of vulnerable functions exported
 // by a packages from the module.

internal/symbols/populate.go

@@ -5,6 +5,7 @@
 package symbols
 
 import (
+	"errors"
 	"fmt"
 	"path/filepath"
 	"strings"
@@ -15,11 +16,11 @@ import (
 
 // Populate attempts to populate the report with symbols derived
 // from the patch link(s) in the report.
-func Populate(r *report.Report, errln logln) error {
-	return populate(r, Patched, errln)
+func Populate(r *report.Report) error {
+	return populate(r, Patched)
 }
 
-func populate(r *report.Report, patched func(string, string, string) (map[string][]string, error), errln logln) error {
+func populate(r *report.Report, patched func(string, string, string) (map[string][]string, error)) error {
 	var defaultFixes []string
 
 	for _, ref := range r.References {
@@ -32,7 +33,7 @@ func populate(r *report.Report, patched func(string, string, string) (map[string
 	if len(defaultFixes) == 0 {
 		return fmt.Errorf("no commit fix links found")
 	}
-
+	var errs []error
 	for _, mod := range r.Modules {
 		hasFixLink := mod.FixLink != ""
 		if hasFixLink {
@@ -44,7 +45,7 @@ func populate(r *report.Report, patched func(string, string, string) (map[string
 			fixRepo := strings.TrimSuffix(fixLink, "/commit/"+fixHash)
 			pkgsToSymbols, err := patched(mod.Module, fixRepo, fixHash)
 			if err != nil {
-				errln(err)
+				errs = append(errs, err)
 				continue
 			}
 			packages := mod.AllPackages()
@@ -69,7 +70,7 @@ func populate(r *report.Report, patched func(string, string, string) (map[string
 		}
 	}
 
-	return nil
+	return errors.Join(errs...)
 }
 
 // indexMax takes a slice of nonempty ints and returns the index of the maximum value

internal/symbols/populate_test.go

@@ -89,8 +89,7 @@ func TestPopulate(t *testing.T) {
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
-			discardLog := func(...any) {}
-			if err := populate(tc.input, patchedFake, discardLog); err != nil {
+			if err := populate(tc.input, patchedFake); err != nil {
 				t.Fatal(err)
 			}
 			got := tc.input