cmd/compile/internal/types2: better error reporting framework (starting point)

Until now, errors which came with additional details (e.g., a declaration
cycle error followed by the list of objects involved in the cycle, one per
line) were reported as an ordinary error followed by "secondary" errors,
with the secondary errors marked as such by having a tab-indented error
message.

This approach often required clients to filter these secondary errors
(as they are not new errors, they are just clarifying a previously
reported error).

This CL introduces a new internal error_ type which permits accumulating
various error information that may then be reported as a single error.

Change-Id: I25b2f094facd37e12737e517f7ef8853d465ff77
Reviewed-on: https://go-review.googlesource.com/c/go/+/296689
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

src/cmd/compile/internal/noder/irgen.go

@@ -41,14 +41,6 @@ func check2(noders []*noder) {
 		CompilerErrorMessages: true, // use error strings matching existing compiler errors
 		Error: func(err error) {
 			terr := err.(types2.Error)
-			if len(terr.Msg) > 0 && terr.Msg[0] == '\t' {
-				// types2 reports error clarifications via separate
-				// error messages which are indented with a tab.
-				// Ignore them to satisfy tools and tests that expect
-				// only one error in such cases.
-				// TODO(gri) Need to adjust error reporting in types2.
-				return
-			}
 			base.ErrorfAt(m.makeXPos(terr.Pos), "%s", terr.Msg)
 		},
 		Importer: &gcimports{

src/cmd/compile/internal/types2/check_test.go

@@ -146,12 +146,8 @@ func checkFiles(t *testing.T, sources []string, goVersion string, colDelta uint,
 			t.Error(err)
 			return
 		}
-		// Ignore secondary error messages starting with "\t";
-		// they are clarifying messages for a primary error.
-		if !strings.Contains(err.Error(), ": \t") {
 		errlist = append(errlist, err)
 	}
-	}
 	conf.Check(pkgName, files, nil)
 
 	if *listErrors {

src/cmd/compile/internal/types2/decl.go

@@ -11,12 +11,12 @@ import (
 	"go/constant"
 )
 
-func (check *Checker) reportAltDecl(obj Object) {
+func (err *error_) recordAltDecl(obj Object) {
 	if pos := obj.Pos(); pos.IsKnown() {
 		// We use "other" rather than "previous" here because
 		// the first declaration seen may not be textually
 		// earlier in the source.
-		check.errorf(pos, "\tother declaration of %s", obj.Name()) // secondary error, \t indented
+		err.errorf(pos, "other declaration of %s", obj.Name())
 	}
 }
 
@@ -27,8 +27,10 @@ func (check *Checker) declare(scope *Scope, id *syntax.Name, obj Object, pos syn
 	// binding."
 	if obj.Name() != "_" {
 		if alt := scope.Insert(obj); alt != nil {
-			check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name())
+			var err error_
-			check.reportAltDecl(alt)
+			err.errorf(obj, "%s redeclared in this block", obj.Name())
+			err.recordAltDecl(alt)
+			check.report(&err)
 			return
 		}
 		obj.setScopePos(pos)
@@ -364,20 +366,22 @@ func (check *Checker) cycleError(cycle []Object) {
 	//           cycle? That would be more consistent with other error messages.
 	i := firstInSrc(cycle)
 	obj := cycle[i]
+	var err error_
 	if check.conf.CompilerErrorMessages {
-		check.errorf(obj.Pos(), "invalid recursive type %s", obj.Name())
+		err.errorf(obj, "invalid recursive type %s", obj.Name())
 	} else {
-		check.errorf(obj.Pos(), "illegal cycle in declaration of %s", obj.Name())
+		err.errorf(obj, "illegal cycle in declaration of %s", obj.Name())
 	}
 	for range cycle {
-		check.errorf(obj.Pos(), "\t%s refers to", obj.Name()) // secondary error, \t indented
+		err.errorf(obj, "%s refers to", obj.Name())
 		i++
 		if i >= len(cycle) {
 			i = 0
 		}
 		obj = cycle[i]
 	}
-	check.errorf(obj.Pos(), "\t%s", obj.Name())
+	err.errorf(obj, "%s", obj.Name())
+	check.report(&err)
 }
 
 // TODO(gri) This functionality should probably be with the Pos implementation.
@@ -787,19 +791,21 @@ func (check *Checker) collectMethods(obj *TypeName) {
 		// to it must be unique."
 		assert(m.name != "_")
 		if alt := mset.insert(m); alt != nil {
+			var err error_
 			switch alt.(type) {
 			case *Var:
-				check.errorf(m.pos, "field and method with the same name %s", m.name)
+				err.errorf(m.pos, "field and method with the same name %s", m.name)
 			case *Func:
 				if check.conf.CompilerErrorMessages {
-					check.errorf(m.pos, "%s.%s redeclared in this block", obj.Name(), m.name)
+					err.errorf(m.pos, "%s.%s redeclared in this block", obj.Name(), m.name)
 				} else {
-					check.errorf(m.pos, "method %s already declared for %s", m.name, obj)
+					err.errorf(m.pos, "method %s already declared for %s", m.name, obj)
 				}
 			default:
 				unreachable()
 			}
-			check.reportAltDecl(alt)
+			err.recordAltDecl(alt)
+			check.report(&err)
 			continue
 		}
 

src/cmd/compile/internal/types2/errors.go

@@ -29,6 +29,83 @@ func unreachable() {
 	panic("unreachable")
 }
 
+// An error_ represents a type-checking error.
+// To report an error_, call Checker.report.
+type error_ struct {
+	desc []errorDesc
+	soft bool // TODO(gri) eventually determine this from an error code
+}
+
+// An errorDesc describes part of a type-checking error.
+type errorDesc struct {
+	pos    syntax.Pos
+	format string
+	args   []interface{}
+}
+
+func (err *error_) empty() bool {
+	return err.desc == nil
+}
+
+func (err *error_) pos() syntax.Pos {
+	if err.empty() {
+		return nopos
+	}
+	return err.desc[0].pos
+}
+
+func (err *error_) msg(qf Qualifier) string {
+	if err.empty() {
+		return "no error"
+	}
+	var buf bytes.Buffer
+	for i := range err.desc {
+		p := &err.desc[i]
+		if i > 0 {
+			fmt.Fprintf(&buf, "\n\t%s: ", p.pos)
+		}
+		buf.WriteString(sprintf(qf, p.format, p.args...))
+	}
+	return buf.String()
+}
+
+// String is for testing.
+func (err *error_) String() string {
+	if err.empty() {
+		return "no error"
+	}
+	return fmt.Sprintf("%s: %s", err.pos(), err.msg(nil))
+}
+
+// errorf adds formatted error information to err.
+// It may be called multiple times to provide additional information.
+func (err *error_) errorf(at poser, format string, args ...interface{}) {
+	err.desc = append(err.desc, errorDesc{posFor(at), format, args})
+}
+
+func sprintf(qf Qualifier, format string, args ...interface{}) string {
+	for i, arg := range args {
+		switch a := arg.(type) {
+		case nil:
+			arg = "<nil>"
+		case operand:
+			panic("internal error: should always pass *operand")
+		case *operand:
+			arg = operandString(a, qf)
+		case syntax.Pos:
+			arg = a.String()
+		case syntax.Expr:
+			arg = syntax.String(a)
+		case Object:
+			arg = ObjectString(a, qf)
+		case Type:
+			arg = TypeString(a, qf)
+		}
+		args[i] = arg
+	}
+	return fmt.Sprintf(format, args...)
+}
+
 func (check *Checker) qualifier(pkg *Package) string {
 	// Qualify the package unless it's the package being type-checked.
 	if pkg != check.pkg {
@@ -42,26 +119,14 @@ func (check *Checker) qualifier(pkg *Package) string {
 }
 
 func (check *Checker) sprintf(format string, args ...interface{}) string {
-	for i, arg := range args {
+	return sprintf(check.qualifier, format, args...)
-		switch a := arg.(type) {
+}
-		case nil:
+
-			arg = "<nil>"
+func (check *Checker) report(err *error_) {
-		case operand:
+	if err.empty() {
-			panic("internal error: should always pass *operand")
+		panic("internal error: reporting no error")
-		case *operand:
-			arg = operandString(a, check.qualifier)
-		case syntax.Pos:
-			arg = a.String()
-		case syntax.Expr:
-			arg = syntax.String(a)
-		case Object:
-			arg = ObjectString(a, check.qualifier)
-		case Type:
-			arg = TypeString(a, check.qualifier)
 	}
-		args[i] = arg
+	check.err(err.pos(), err.msg(check.qualifier), err.soft)
-	}
-	return fmt.Sprintf(format, args...)
 }
 
 func (check *Checker) trace(pos syntax.Pos, format string, args ...interface{}) {

src/cmd/compile/internal/types2/errors_test.go

@@ -6,6 +6,26 @@ package types2
 
 import "testing"
 
+func TestError(t *testing.T) {
+	var err error_
+	want := "no error"
+	if got := err.String(); got != want {
+		t.Errorf("empty error: got %q, want %q", got, want)
+	}
+
+	want = "<unknown position>: foo 42"
+	err.errorf(nopos, "foo %d", 42)
+	if got := err.String(); got != want {
+		t.Errorf("simple error: got %q, want %q", got, want)
+	}
+
+	want = "<unknown position>: foo 42\n\t<unknown position>: bar 43"
+	err.errorf(nopos, "bar %d", 43)
+	if got := err.String(); got != want {
+		t.Errorf("simple error: got %q, want %q", got, want)
+	}
+}
+
 func TestStripAnnotations(t *testing.T) {
 	for _, test := range []struct {
 		in, want string

src/cmd/compile/internal/types2/initorder.go

@@ -151,18 +151,20 @@ func findPath(objMap map[Object]*declInfo, from, to Object, seen map[Object]bool
 // reportCycle reports an error for the given cycle.
 func (check *Checker) reportCycle(cycle []Object) {
 	obj := cycle[0]
+	var err error_
 	if check.conf.CompilerErrorMessages {
-		check.errorf(obj, "initialization loop for %s", obj.Name())
+		err.errorf(obj, "initialization loop for %s", obj.Name())
 	} else {
-		check.errorf(obj, "initialization cycle for %s", obj.Name())
+		err.errorf(obj, "initialization cycle for %s", obj.Name())
 	}
 	// subtle loop: print cycle[i] for i = 0, n-1, n-2, ... 1 for len(cycle) = n
 	for i := len(cycle) - 1; i >= 0; i-- {
-		check.errorf(obj, "\t%s refers to", obj.Name()) // secondary error, \t indented
+		err.errorf(obj, "%s refers to", obj.Name())
 		obj = cycle[i]
 	}
 	// print cycle[0] again to close the cycle
-	check.errorf(obj, "\t%s", obj.Name())
+	err.errorf(obj, "%s", obj.Name())
+	check.report(&err)
 }
 
 // ----------------------------------------------------------------------------

src/cmd/compile/internal/types2/labels.go

@@ -128,8 +128,11 @@ func (check *Checker) blockBranches(all *Scope, parent *block, lstmt *syntax.Lab
 			if name := s.Label.Value; name != "_" {
 				lbl := NewLabel(s.Label.Pos(), check.pkg, name)
 				if alt := all.Insert(lbl); alt != nil {
-					check.softErrorf(lbl.pos, "label %s already declared", name)
+					var err error_
-					check.reportAltDecl(alt)
+					err.soft = true
+					err.errorf(lbl.pos, "label %s already declared", name)
+					err.recordAltDecl(alt)
+					check.report(&err)
 					// ok to continue
 				} else {
 					b.insert(s)

src/cmd/compile/internal/types2/resolver.go

@@ -305,8 +305,10 @@ func (check *Checker) collectObjects() {
 							// the object may be imported into more than one file scope
 							// concurrently. See issue #32154.)
 							if alt := fileScope.Insert(obj); alt != nil {
-								check.errorf(s.LocalPkgName, "%s redeclared in this block", obj.Name())
+								var err error_
-								check.reportAltDecl(alt)
+								err.errorf(s.LocalPkgName, "%s redeclared in this block", obj.Name())
+								err.recordAltDecl(alt)
+								check.report(&err)
 							} else {
 								check.dotImportMap[dotImportKey{fileScope, obj}] = pkgName
 							}
@@ -456,14 +458,16 @@ func (check *Checker) collectObjects() {
 	for _, scope := range fileScopes {
 		for _, obj := range scope.elems {
 			if alt := pkg.scope.Lookup(obj.Name()); alt != nil {
+				var err error_
 				if pkg, ok := obj.(*PkgName); ok {
-					check.errorf(alt, "%s already declared through import of %s", alt.Name(), pkg.Imported())
+					err.errorf(alt, "%s already declared through import of %s", alt.Name(), pkg.Imported())
-					check.reportAltDecl(pkg)
+					err.recordAltDecl(pkg)
 				} else {
-					check.errorf(alt, "%s already declared through dot-import of %s", alt.Name(), obj.Pkg())
+					err.errorf(alt, "%s already declared through dot-import of %s", alt.Name(), obj.Pkg())
-					// TODO(gri) dot-imported objects don't have a position; reportAltDecl won't print anything
+					// TODO(gri) dot-imported objects don't have a position; recordAltDecl won't print anything
-					check.reportAltDecl(obj)
+					err.recordAltDecl(obj)
 				}
+				check.report(&err)
 			}
 		}
 	}

src/cmd/compile/internal/types2/stmt.go

@@ -253,8 +253,10 @@ L:
 			// (quadratic algorithm, but these lists tend to be very short)
 			for _, vt := range seen[val] {
 				if check.identical(v.typ, vt.typ) {
-					check.errorf(&v, "duplicate case %s in expression switch", &v)
+					var err error_
-					check.error(vt.pos, "\tprevious case") // secondary error, \t indented
+					err.errorf(&v, "duplicate case %s in expression switch", &v)
+					err.errorf(vt.pos, "previous case")
+					check.report(&err)
 					continue L
 				}
 			}
@@ -282,8 +284,10 @@ L:
 				if T != nil {
 					Ts = T.String()
 				}
-				check.errorf(e, "duplicate case %s in type switch", Ts)
+				var err error_
-				check.error(pos, "\tprevious case") // secondary error, \t indented
+				err.errorf(e, "duplicate case %s in type switch", Ts)
+				err.errorf(pos, "previous case")
+				check.report(&err)
 				continue L
 			}
 		}
@@ -430,8 +434,10 @@ func (check *Checker) stmt(ctxt stmtContext, s syntax.Stmt) {
 				// with the same name as a result parameter is in scope at the place of the return."
 				for _, obj := range res.vars {
 					if alt := check.lookup(obj.name); alt != nil && alt != obj {
-						check.errorf(s, "result parameter %s not in scope at return", obj.name)
+						var err error_
-						check.errorf(alt, "\tinner declaration of %s", obj)
+						err.errorf(s, "result parameter %s not in scope at return", obj.name)
+						err.errorf(alt, "inner declaration of %s", obj)
+						check.report(&err)
 						// ok to continue
 					}
 				}

src/cmd/compile/internal/types2/typexpr.go

@@ -352,8 +352,10 @@ func (check *Checker) funcType(sig *Signature, recvPar *syntax.Field, tparams []
 	params, variadic := check.collectParams(scope, ftyp.ParamList, nil, true)
 	results, _ := check.collectParams(scope, ftyp.ResultList, nil, false)
 	scope.Squash(func(obj, alt Object) {
-		check.errorf(obj, "%s redeclared in this block", obj.Name())
+		var err error_
-		check.reportAltDecl(alt)
+		err.errorf(obj, "%s redeclared in this block", obj.Name())
+		err.recordAltDecl(alt)
+		check.report(&err)
 	})
 
 	if recvPar != nil {
@@ -796,8 +798,10 @@ func (check *Checker) collectParams(scope *Scope, list []*syntax.Field, type0 sy
 
 func (check *Checker) declareInSet(oset *objset, pos syntax.Pos, obj Object) bool {
 	if alt := oset.insert(obj); alt != nil {
-		check.errorf(pos, "%s redeclared", obj.Name())
+		var err error_
-		check.reportAltDecl(alt)
+		err.errorf(pos, "%s redeclared", obj.Name())
+		err.recordAltDecl(alt)
+		check.report(&err)
 		return false
 	}
 	return true
@@ -940,8 +944,10 @@ func (check *Checker) completeInterface(pos syntax.Pos, ityp *Interface) {
 			methods = append(methods, m)
 			mpos[m] = pos
 		case explicit:
-			check.errorf(pos, "duplicate method %s", m.name)
+			var err error_
-			check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented
+			err.errorf(pos, "duplicate method %s", m.name)
+			err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
+			check.report(&err)
 		default:
 			// We have a duplicate method name in an embedded (not explicitly declared) method.
 			// Check method signatures after all types are computed (issue #33656).
@@ -950,8 +956,10 @@ func (check *Checker) completeInterface(pos syntax.Pos, ityp *Interface) {
 			// error message.
 			check.atEnd(func() {
 				if !check.allowVersion(m.pkg, 1, 14) || !check.identical(m.typ, other.Type()) {
-					check.errorf(pos, "duplicate method %s", m.name)
+					var err error_
-					check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented
+					err.errorf(pos, "duplicate method %s", m.name)
+					err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
+					check.report(&err)
 				}
 			})
 		}