From: Robert Griesemer Date: Thu, 25 Feb 2021 22:54:04 +0000 (-0800) Subject: cmd/compile/internal/types2: better error reporting framework (starting point) X-Git-Tag: go1.17beta1~1213 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=acd7cb5887;p=gostls13.git 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 Reviewed-by: Robert Findley --- diff --git a/src/cmd/compile/internal/noder/irgen.go b/src/cmd/compile/internal/noder/irgen.go index 06b234c31d..2de8c3fa60 100644 --- a/src/cmd/compile/internal/noder/irgen.go +++ b/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{ diff --git a/src/cmd/compile/internal/types2/check_test.go b/src/cmd/compile/internal/types2/check_test.go index 9c1d278520..fc6f46b4b8 100644 --- a/src/cmd/compile/internal/types2/check_test.go +++ b/src/cmd/compile/internal/types2/check_test.go @@ -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) diff --git a/src/cmd/compile/internal/types2/decl.go b/src/cmd/compile/internal/types2/decl.go index f0a037adb0..3528669bf9 100644 --- a/src/cmd/compile/internal/types2/decl.go +++ b/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()) - 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 } diff --git a/src/cmd/compile/internal/types2/errors.go b/src/cmd/compile/internal/types2/errors.go index 62b1d39d83..01df50c8e3 100644 --- a/src/cmd/compile/internal/types2/errors.go +++ b/src/cmd/compile/internal/types2/errors.go @@ -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, diff --git a/src/cmd/compile/internal/types2/errors_test.go b/src/cmd/compile/internal/types2/errors_test.go index cb21ff1ad3..e1f0e83fc9 100644 --- a/src/cmd/compile/internal/types2/errors_test.go +++ b/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 = ": 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 = ": foo 42\n\t: 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 diff --git a/src/cmd/compile/internal/types2/initorder.go b/src/cmd/compile/internal/types2/initorder.go index a9cabecdf2..4081627666 100644 --- a/src/cmd/compile/internal/types2/initorder.go +++ b/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) } // ---------------------------------------------------------------------------- diff --git a/src/cmd/compile/internal/types2/labels.go b/src/cmd/compile/internal/types2/labels.go index b20b454dea..cbbd65aa9a 100644 --- a/src/cmd/compile/internal/types2/labels.go +++ b/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) - 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) diff --git a/src/cmd/compile/internal/types2/resolver.go b/src/cmd/compile/internal/types2/resolver.go index 44fa51a8e5..fe551525c6 100644 --- a/src/cmd/compile/internal/types2/resolver.go +++ b/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()) - 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) } } } diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go index 490cd0fc19..21244f6065 100644 --- a/src/cmd/compile/internal/types2/stmt.go +++ b/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) - 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 } } diff --git a/src/cmd/compile/internal/types2/typexpr.go b/src/cmd/compile/internal/types2/typexpr.go index 02f9b2804d..177fcf4215 100644 --- a/src/cmd/compile/internal/types2/typexpr.go +++ b/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()) - 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) } }) }