go.tools/go/types: improved operand printing in error messages

Also:
- removed operand.val hack for built-in encoding; added id field
- minor cleanups

R=adonovan
CC=golang-dev
https://golang.org/cl/13840046

go/types/builtins.go

@@ -405,8 +405,7 @@ func (check *checker) builtin(x *operand, call *ast.CallExpr, id builtinId) {
 		}
 
 	default:
-		check.invalidAST(call.Pos(), "unknown builtin id %d", id)
-		goto Error
+		panic("unreachable")
 	}
 
 	x.expr = call

go/types/call.go

@@ -9,19 +9,6 @@ package types
 import (
 	"go/ast"
 	"go/token"
-
-	"code.google.com/p/go.tools/go/exact"
-)
-
-// exprKind describes the kind of an expression;
-// the kind determines if an expression is valid
-// in 'statement context'.
-type exprKind int
-
-const (
-	conversion exprKind = iota
-	expression
-	statement
 )
 
 func (check *checker) call(x *operand, e *ast.CallExpr) exprKind {
@@ -61,8 +48,8 @@ func (check *checker) call(x *operand, e *ast.CallExpr) exprKind {
 		return conversion
 
 	case builtin:
-		id, _ := exact.Int64Val(x.val)
-		check.builtin(x, e, builtinId(id))
+		id := x.id
+		check.builtin(x, e, id)
 		return predeclaredFuncs[id].kind
 
 	default:
@@ -239,7 +226,7 @@ func (check *checker) selector(x *operand, e *ast.SelectorExpr) {
 			case *Builtin:
 				x.mode = builtin
 				x.typ = exp.typ
-				x.val = exact.MakeInt64(int64(exp.id))
+				x.id = exp.id
 			default:
 				unreachable()
 			}

go/types/check_test.go

@@ -45,6 +45,7 @@ var (
 
 // Each tests entry is list of files belonging to the same package.
 var tests = [][]string{
+	{"testdata/errors.src"},
 	{"testdata/importdecl0a.src", "testdata/importdecl0b.src"},
 	{"testdata/cycles.src"},
 	{"testdata/decls0.src"},

go/types/expr.go

@@ -14,15 +14,6 @@ import (
 	"code.google.com/p/go.tools/go/exact"
 )
 
-// TODO(gri) Internal cleanups
-// - don't print error messages referring to invalid types (they are likely spurious errors)
-// - simplify invalid handling: maybe just use Typ[Invalid] as marker, get rid of invalid Mode for values?
-// - rethink error handling: should all callers check if x.mode == valid after making a call?
-// - consider storing error messages in invalid operands for better error messages/debugging output
-
-// TODO(gri) Test issues
-// - API tests are missing (e.g., identifiers should be handled as expressions in callbacks)
-
 /*
 Basic algorithm:
 
@@ -840,6 +831,16 @@ func (check *checker) indexedElts(elts []ast.Expr, typ Type, length int64) int64
 	return max
 }
 
+// exprKind describes the kind of an expression; the kind
+// determines if an expression is valid in 'statement context'.
+type exprKind int
+
+const (
+	conversion exprKind = iota
+	expression
+	statement
+)
+
 // rawExpr typechecks expression e and initializes x with the expression
 // value or type. If an error occurred, x.mode is set to invalid.
 // If hint != nil, it is the type of a composite literal element.

go/types/operand.go

@@ -8,7 +8,6 @@ package types
 
 import (
 	"bytes"
-	"fmt"
 	"go/ast"
 	"go/token"
 
@@ -30,20 +29,20 @@ const (
 )
 
 var operandModeString = [...]string{
-	invalid:  "invalid",
+	invalid:  "invalid operand",
 	novalue:  "no value",
-	builtin:  "builtin",
+	builtin:  "built-in",
 	typexpr:  "type",
 	constant: "constant",
 	variable: "variable",
 	value:    "value",
-	valueok:  "value,ok",
+	valueok:  "value, ok",
 }
 
 // An operand represents an intermediate value during type checking.
 // Operands have an (addressing) mode, the expression evaluating to
-// the operand, the operand's type, and a value if mode == constant
-// or builtin. The built-in id is encoded as an exact Int64 in val.
+// the operand, the operand's type, a value for constants, and an id
+// for built-in functions.
 // The zero value of operand is a ready to use invalid operand.
 //
 type operand struct {
@@ -51,6 +50,7 @@ type operand struct {
 	expr ast.Expr
 	typ  Type
 	val  exact.Value
+	id   builtinId
 }
 
 // pos returns the position of the expression corresponding to x.
@@ -64,29 +64,95 @@ func (x *operand) pos() token.Pos {
 	return x.expr.Pos()
 }
 
+// Operand string formats
+// (not all "untyped" cases can appear due to the type system,
+// but they fall out naturally here)
+//
+// mode       format
+//
+// invalid    <expr> (               <mode>                    )
+// novalue    <expr> (               <mode>                    )
+// builtin    <expr> (               <mode>                    )
+// typexpr    <expr> (               <mode>                    )
+//
+// constant   <expr> (<untyped kind> <mode>                    )
+// constant   <expr> (               <mode>       of type <typ>)
+// constant   <expr> (<untyped kind> <mode> <val>              )
+// constant   <expr> (               <mode> <val> of type <typ>)
+//
+// variable   <expr> (<untyped kind> <mode>                    )
+// variable   <expr> (               <mode>       of type <typ>)
+//
+// value      <expr> (<untyped kind> <mode>                    )
+// value      <expr> (               <mode>       of type <typ>)
+//
+// valueok    <expr> (<untyped kind> <mode>                    )
+// valueok    <expr> (               <mode>       of type <typ>)
+//
 func (x *operand) String() string {
-	if x.mode == invalid {
-		return "invalid operand"
-	}
 	var buf bytes.Buffer
+
+	var expr string
 	if x.expr != nil {
-		buf.WriteString(exprString(x.expr))
+		expr = exprString(x.expr)
+	} else {
+		switch x.mode {
+		case builtin:
+			expr = predeclaredFuncs[x.id].name
+		case typexpr:
+			expr = typeString(x.typ)
+		case constant:
+			expr = x.val.String()
+		}
+	}
+
+	// <expr> (
+	if expr != "" {
+		buf.WriteString(expr)
 		buf.WriteString(" (")
 	}
-	buf.WriteString(operandModeString[x.mode])
-	if x.mode == constant {
-		format := " %v"
-		if isString(x.typ) {
-			format = " %q"
+
+	// <untyped kind>
+	hasType := false
+	switch x.mode {
+	case invalid, novalue, builtin, typexpr:
+		// no type
+	default:
+		// has type
+		if isUntyped(x.typ) {
+			buf.WriteString(x.typ.(*Basic).name)
+			buf.WriteByte(' ')
+			break
 		}
-		fmt.Fprintf(&buf, format, x.val)
+		hasType = true
 	}
-	if x.mode != novalue && (x.mode != constant || !isUntyped(x.typ)) {
-		fmt.Fprintf(&buf, " of type %s", typeString(x.typ))
+
+	// <mode>
+	buf.WriteString(operandModeString[x.mode])
+
+	// <val>
+	if x.mode == constant {
+		if s := x.val.String(); s != expr {
+			buf.WriteByte(' ')
+			buf.WriteString(s)
+		}
 	}
-	if x.expr != nil {
+
+	// <typ>
+	if hasType {
+		if x.typ != Typ[Invalid] {
+			buf.WriteString(" of type ")
+			writeType(&buf, x.typ)
+		} else {
+			buf.WriteString(" with invalid type")
+		}
+	}
+
+	// )
+	if expr != "" {
 		buf.WriteByte(')')
 	}
+
 	return buf.String()
 }
 

go/types/testdata/errors.src

@@ -0,0 +1,38 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// testing precise operand formatting in error messages
+// (matching messages are regular expressions, hence the \'s)
+
+package errors
+
+func f(x int, m map[string]int) {
+	// no values
+	_ = f /* ERROR "f\(0, m\) \(no value\) used as value" */ (0, m)
+
+	// built-ins
+	_ = println /* ERROR "println \(built-in\) must be called" */
+
+	// types
+	_ = complex128 /* ERROR "complex128 \(type\) is not an expression" */
+
+	// constants
+	const c1 = 991
+	const c2 float32 = 0.5
+	0 /* ERROR "0 \(untyped integer constant\) is not used" */
+	c1 /* ERROR "c1 \(untyped integer constant 991\) is not used" */
+	c2 /* ERROR "c2 \(constant 1/2 of type float32\) is not used" */
+	c1 /* ERROR "c1 \+ c2 \(constant 1983/2 of type float32\) is not used" */ + c2
+
+	// variables
+	x /* ERROR "x \(variable of type int\) is not used" */
+
+	// values
+	x /* ERROR "x != x \(untyped boolean value\) is not used" */ != x
+	x /* ERROR "x \+ x \(value of type int\) is not used" */ + x
+
+	// value, ok's
+	const s = "foo"
+	m /* ERROR "m\[s\] \(value, ok of type int\) is not used" */ [s]
+}

go/types/typexpr.go

@@ -95,7 +95,7 @@ func (check *checker) ident(x *operand, e *ast.Ident, def *Named, cycleOk bool)
 	case *Builtin:
 		obj.used = true // for built-ins defined by package unsafe
 		x.mode = builtin
-		x.val = exact.MakeInt64(int64(obj.id))
+		x.id = obj.id
 
 	default:
 		unreachable()