]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/compile/internal/types2: better error reporting framework (starting point)
authorRobert Griesemer <gri@golang.org>
Thu, 25 Feb 2021 22:54:04 +0000 (14:54 -0800)
committerRobert Griesemer <gri@golang.org>
Wed, 10 Mar 2021 00:50:17 +0000 (00:50 +0000)
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
src/cmd/compile/internal/types2/check_test.go
src/cmd/compile/internal/types2/decl.go
src/cmd/compile/internal/types2/errors.go
src/cmd/compile/internal/types2/errors_test.go
src/cmd/compile/internal/types2/initorder.go
src/cmd/compile/internal/types2/labels.go
src/cmd/compile/internal/types2/resolver.go
src/cmd/compile/internal/types2/stmt.go
src/cmd/compile/internal/types2/typexpr.go

index 06b234c31d3451701aaf7768df3c99d807900f56..2de8c3fa60ac1e0dd222d3e41160e08271c8dc79 100644 (file)
@@ -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{
index 9c1d278520dfcd41cb353adf33ddd231b5da2e89..fc6f46b4b8e31fe43c480d12402a45e6074a9612 100644 (file)
@@ -146,11 +146,7 @@ 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)
-               }
+               errlist = append(errlist, err)
        }
        conf.Check(pkgName, files, nil)
 
index f0a037adb01ba64ade74374489dd0dbb0f7ae1d6..3528669bf916838b842c835ae430ddfeb036f7da 100644 (file)
@@ -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())
-                       check.reportAltDecl(alt)
+                       var err error_
+                       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
                }
 
index 62b1d39d836d2ca1023361b9ea163c18de29b6ff..01df50c8e37eab6cca1cd7cac33738db7a84708f 100644 (file)
@@ -29,19 +29,61 @@ func unreachable() {
        panic("unreachable")
 }
 
-func (check *Checker) qualifier(pkg *Package) string {
-       // Qualify the package unless it's the package being type-checked.
-       if pkg != check.pkg {
-               // If the same package name was used by multiple packages, display the full path.
-               if check.pkgCnt[pkg.name] > 1 {
-                       return strconv.Quote(pkg.path)
+// 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)
                }
-               return pkg.name
+               buf.WriteString(sprintf(qf, p.format, p.args...))
        }
-       return ""
+       return buf.String()
 }
 
-func (check *Checker) sprintf(format string, args ...interface{}) 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:
@@ -49,21 +91,44 @@ func (check *Checker) sprintf(format string, args ...interface{}) string {
                case operand:
                        panic("internal error: should always pass *operand")
                case *operand:
-                       arg = operandString(a, check.qualifier)
+                       arg = operandString(a, qf)
                case syntax.Pos:
                        arg = a.String()
                case syntax.Expr:
                        arg = syntax.String(a)
                case Object:
-                       arg = ObjectString(a, check.qualifier)
+                       arg = ObjectString(a, qf)
                case Type:
-                       arg = TypeString(a, check.qualifier)
+                       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 {
+               // If the same package name was used by multiple packages, display the full path.
+               if check.pkgCnt[pkg.name] > 1 {
+                       return strconv.Quote(pkg.path)
+               }
+               return pkg.name
+       }
+       return ""
+}
+
+func (check *Checker) sprintf(format string, args ...interface{}) string {
+       return sprintf(check.qualifier, format, args...)
+}
+
+func (check *Checker) report(err *error_) {
+       if err.empty() {
+               panic("internal error: reporting no error")
+       }
+       check.err(err.pos(), err.msg(check.qualifier), err.soft)
+}
+
 func (check *Checker) trace(pos syntax.Pos, format string, args ...interface{}) {
        fmt.Printf("%s:\t%s%s\n",
                pos,
index cb21ff1ad3d8308ddd8b49b2c7a7508c3dbfdcf1..e1f0e83fc97d16f43be86cf4eb169d78e1bba284 100644 (file)
@@ -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
index a9cabecdf27a427f5015c620873ba618b45c8399..40816276665117ba4bae14c040d515544df9654b 100644 (file)
@@ -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)
 }
 
 // ----------------------------------------------------------------------------
index b20b454dea2561ab514ee3abb2ee5ea1dc9dc0ec..cbbd65aa9afa16e126cb79884f78cac2e0093395 100644 (file)
@@ -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)
-                                       check.reportAltDecl(alt)
+                                       var err error_
+                                       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)
index 44fa51a8e52e722911f4dd88ca4fdedb79a5541b..fe551525c6a46a63c58b7f7e5ac1591beafc2782 100644 (file)
@@ -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())
-                                                               check.reportAltDecl(alt)
+                                                               var err error_
+                                                               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())
-                                       check.reportAltDecl(pkg)
+                                       err.errorf(alt, "%s already declared through import of %s", alt.Name(), pkg.Imported())
+                                       err.recordAltDecl(pkg)
                                } else {
-                                       check.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
-                                       check.reportAltDecl(obj)
+                                       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; recordAltDecl won't print anything
+                                       err.recordAltDecl(obj)
                                }
+                               check.report(&err)
                        }
                }
        }
index 490cd0fc1903572837d5149f73865b6bcf401b45..21244f60655a80d8d0115b1b93758fdf5932cfda 100644 (file)
@@ -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)
-                                       check.error(vt.pos, "\tprevious case") // secondary error, \t indented
+                                       var err error_
+                                       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)
-                               check.error(pos, "\tprevious case") // secondary error, \t indented
+                               var err error_
+                               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)
-                                               check.errorf(alt, "\tinner declaration of %s", obj)
+                                               var err error_
+                                               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
                                        }
                                }
index 02f9b2804d48c6c16c89f5a1b4552562882f7aa4..177fcf4215baf84bac45fa13d61a1061276e244a 100644 (file)
@@ -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())
-               check.reportAltDecl(alt)
+               var err error_
+               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())
-               check.reportAltDecl(alt)
+               var err error_
+               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)
-                       check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented
+                       var err error_
+                       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)
-                                       check.errorf(mpos[other.(*Func)], "\tother declaration of %s", m.name) // secondary error, \t indented
+                                       var err error_
+                                       err.errorf(pos, "duplicate method %s", m.name)
+                                       err.errorf(mpos[other.(*Func)], "other declaration of %s", m.name)
+                                       check.report(&err)
                                }
                        })
                }