go/types: factor out some methods that compute a single error

In order to generate more accurate or informative error messages from
the type checker, it can be helpful to interpret error messages in
context. This is currently achieved in a number of ways:

 + Return a boolean value, and then reverse-engineer the error at the
   callsite (as in representable->representableConst).
 + Return a value causing the error (as in Checker.missingMethod), and
   add the error at the callsite.
 + Pass a "reason" string pointer to capture the error (as in
   Checker.assignableTo), and add the error at the callsite.
 + Pass a "context" string pointer, and use this when writing errors in
   the delegated method.

In all cases, it is the responsibility of whatever code calls
Checker.error* to set the operand mode to invalid.

These methods are used as appropriate, depending on whether multiple
errors are generated, whether additional context is needed, and whether
the mere presence of an error needs to be interpreted at the callsite.
However, this practice has some downsides: the plurality of error
handling techniques can be a barrier to readability and composability.

In this CL, we introduce Yet Another Pattern, with the hope that it can
replace some or all of the existing techniques: factor out side-effect
free functions that evaluate a single error, and add helpers for
recording this error in the Checker.

As a proof of concept this is done for Checker.representable and
Checker.convertUntyped. If the general pattern does not seem appropriate
for replacing some or all of the error-handling techniques listed above,
we should revert to an established technique.

Some internal error APIs are refactored to operate on an error, rather
than a types.Error, with internal error metadata extracted using
errors.As. This seemed to have negligible impact on performance, but we
should be careful about actually wrapping errors: I expect that many
users will expect err to be a types.Error.

Change-Id: Ic5c6edcdc02768cd84e04638fad648934bcf3c17
Reviewed-on: https://go-review.googlesource.com/c/go/+/242082
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

diff --git a/src/go/types/errors.go b/src/go/types/errors.go
index 91b077163c403f1080a09e69ac3c8e5c3f817595..88e41c5713267c94506cc1feb0e8f2af185a0640 100644 (file)
--- a/src/go/types/errors.go
+++ b/src/go/types/errors.go
@@ -7,6 +7,7 @@
 package types
 
 import (
+       "errors"
        "fmt"
        "go/ast"
        "go/token"
@@ -72,22 +73,33 @@ func (check *Checker) dump(format string, args ...interface{}) {
        fmt.Println(check.sprintf(format, args...))
 }
 
-func (check *Checker) err(pos token.Pos, msg string, soft bool) {
+func (check *Checker) err(err error) {
+       if err == nil {
+               return
+       }
+       var e Error
+       isInternal := errors.As(err, &e)
        // Cheap trick: Don't report errors with messages containing
        // "invalid operand" or "invalid type" as those tend to be
        // follow-on errors which don't add useful information. Only
        // exclude them if these strings are not at the beginning,
        // and only if we have at least one error already reported.
-       if check.firstErr != nil && (strings.Index(msg, "invalid operand") > 0 || strings.Index(msg, "invalid type") > 0) {
+       isInvalidErr := isInternal && (strings.Index(e.Msg, "invalid operand") > 0 || strings.Index(e.Msg, "invalid type") > 0)
+       if check.firstErr != nil && isInvalidErr {
                return
        }
 
-       err := Error{check.fset, pos, msg, soft}
        if check.firstErr == nil {
                check.firstErr = err
        }
 
        if trace {
+               pos := e.Pos
+               msg := e.Msg
+               if !isInternal {
+                       msg = err.Error()
+                       pos = token.NoPos
+               }
                check.trace(pos, "ERROR: %s", msg)
        }
 
@@ -99,15 +111,30 @@ func (check *Checker) err(pos token.Pos, msg string, soft bool) {
 }
 
 func (check *Checker) error(pos token.Pos, msg string) {
-       check.err(pos, msg, false)
+       check.err(Error{Fset: check.fset, Pos: pos, Msg: msg})
+}
+
+// newErrorf creates a new Error, but does not handle it.
+func (check *Checker) newErrorf(pos token.Pos, format string, args ...interface{}) error {
+       return Error{
+               Fset: check.fset,
+               Pos:  pos,
+               Msg:  check.sprintf(format, args...),
+               Soft: false,
+       }
 }
 
 func (check *Checker) errorf(pos token.Pos, format string, args ...interface{}) {
-       check.err(pos, check.sprintf(format, args...), false)
+       check.error(pos, check.sprintf(format, args...))
 }
 
 func (check *Checker) softErrorf(pos token.Pos, format string, args ...interface{}) {
-       check.err(pos, check.sprintf(format, args...), true)
+       check.err(Error{
+               Fset: check.fset,
+               Pos:  pos,
+               Msg:  check.sprintf(format, args...),
+               Soft: true,
+       })
 }
 
 func (check *Checker) invalidAST(pos token.Pos, format string, args ...interface{}) {

diff --git a/src/go/types/expr.go b/src/go/types/expr.go
index d1e892a9b70d8a9af3de2f194640e6de4b745c9c..8503a521f674c6f70d385c517e0d55b8a9284540 100644 (file)
--- a/src/go/types/expr.go
+++ b/src/go/types/expr.go
@@ -329,8 +329,16 @@ func representableConst(x constant.Value, check *Checker, typ *Basic, rounded *c
        return false
 }
 
-// representable checks that a constant operand is representable in the given basic type.
+// representable checks that a constant operand is representable in the given
+// basic type.
 func (check *Checker) representable(x *operand, typ *Basic) {
+       if err := check.isRepresentable(x, typ); err != nil {
+               x.mode = invalid
+               check.err(err)
+       }
+}
+
+func (check *Checker) isRepresentable(x *operand, typ *Basic) error {
        assert(x.mode == constant_)
        if !representableConst(x.val, check, typ, &x.val) {
                var msg string
@@ -350,9 +358,9 @@ func (check *Checker) representable(x *operand, typ *Basic) {
                } else {
                        msg = "cannot convert %s to %s"
                }
-               check.errorf(x.pos(), msg, x, typ)
-               x.mode = invalid
+               return check.newErrorf(x.pos(), msg, x, typ)
        }
+       return nil
 }
 
 // updateExprType updates the type of x to typ and invokes itself
@@ -488,10 +496,16 @@ func (check *Checker) updateExprVal(x ast.Expr, val constant.Value) {
 
 // convertUntyped attempts to set the type of an untyped value to the target type.
 func (check *Checker) convertUntyped(x *operand, target Type) {
-       if x.mode == invalid || isTyped(x.typ) || target == Typ[Invalid] {
-               return
+       if err := check.canConvertUntyped(x, target); err != nil {
+               x.mode = invalid
+               check.err(err)
        }
+}
 
+func (check *Checker) canConvertUntyped(x *operand, target Type) error {
+       if x.mode == invalid || isTyped(x.typ) || target == Typ[Invalid] {
+               return nil
+       }
        // TODO(gri) Sloppy code - clean up. This function is central
        //           to assignment and expression checking.
 
@@ -507,16 +521,15 @@ func (check *Checker) convertUntyped(x *operand, target Type) {
                } else if xkind != tkind {
                        goto Error
                }
-               return
+               return nil
        }
 
        // typed target
        switch t := target.Underlying().(type) {
        case *Basic:
                if x.mode == constant_ {
-                       check.representable(x, t)
-                       if x.mode == invalid {
-                               return
+                       if err := check.isRepresentable(x, t); err != nil {
+                               return err
                        }
                        // expression value may have been rounded - update if needed
                        check.updateExprVal(x.expr, x.val)
@@ -576,11 +589,10 @@ func (check *Checker) convertUntyped(x *operand, target Type) {
 
        x.typ = target
        check.updateExprType(x.expr, target, true) // UntypedNils are final
-       return
+       return nil
 
 Error:
-       check.errorf(x.pos(), "cannot convert %s to %s", x, target)
-       x.mode = invalid
+       return check.newErrorf(x.pos(), "cannot convert %s to %s", x, target)
 }
 
 func (check *Checker) comparison(x, y *operand, op token.Token) {