From 45395b5ad6d59f418cbe8b93950c9bae6e6f2196 Mon Sep 17 00:00:00 2001
From: griesemer <gri@golang.org>
Date: Fri, 22 Sep 2017 16:24:25 +0200
Subject: [PATCH] go/types: don't accept incorrect shift expression arguments

Under certain circumstances involving shifts, go/types didn't verify
that untyped constant values were representable by the relevant type,
leading to the acceptance of incorrect programs (see the issue).

Fixing this code exposed another problem with int-to-string conversions
which suddenly failed because now the type-checker complained that a
(constant) integer argument wasn't representable as a string. Fixed that
as well.

Added many additional tests covering the various scenarious.

Found two cmd/compile bugs in the process (#21979, #21981) and filed
a go/types TODO (#21982).

Fixes #21727.

Change-Id: If443ee0230979cd7d45d2fc669e623648caa70da
Reviewed-on: https://go-review.googlesource.com/65370
Reviewed-by: Alan Donovan <adonovan@google.com>
---
 src/go/types/api_test.go         | 18 ++++++++++++++----
 src/go/types/conversions.go      | 17 +++++++++++++++--
 src/go/types/expr.go             | 28 +++++++++++++++++++---------
 src/go/types/testdata/shifts.src |  9 +++++++++
 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/src/go/types/api_test.go b/src/go/types/api_test.go
index ab08a2669d..4f54f684b8 100644
--- a/src/go/types/api_test.go
+++ b/src/go/types/api_test.go
@@ -87,6 +87,10 @@ func TestValuesInfo(t *testing.T) {
 		{`package c5a; var _ = string("foo")`, `"foo"`, `string`, `"foo"`},
 		{`package c5b; var _ = string("foo")`, `string("foo")`, `string`, `"foo"`},
 		{`package c5c; type T string; var _ = T("foo")`, `T("foo")`, `c5c.T`, `"foo"`},
+		{`package c5d; var _ = string(65)`, `65`, `untyped int`, `65`},
+		{`package c5e; var _ = string('A')`, `'A'`, `untyped rune`, `65`},
+		{`package c5f; type T string; var _ = T('A')`, `'A'`, `untyped rune`, `65`},
+		{`package c5g; var s uint; var _ = string(1 << s)`, `1 << s`, `untyped int`, ``},
 
 		{`package d0; var _ = []byte("foo")`, `"foo"`, `string`, `"foo"`},
 		{`package d1; var _ = []byte(string("foo"))`, `"foo"`, `string`, `"foo"`},
@@ -122,7 +126,7 @@ func TestValuesInfo(t *testing.T) {
 		}
 		name := mustTypecheck(t, "ValuesInfo", test.src, &info)
 
-		// look for constant expression
+		// look for expression
 		var expr ast.Expr
 		for e := range info.Types {
 			if ExprString(e) == test.expr {
@@ -142,9 +146,15 @@ func TestValuesInfo(t *testing.T) {
 			continue
 		}
 
-		// check that value is correct
-		if got := tv.Value.ExactString(); got != test.val {
-			t.Errorf("package %s: got value %s; want %s", name, got, test.val)
+		// if we have a constant, check that value is correct
+		if tv.Value != nil {
+			if got := tv.Value.ExactString(); got != test.val {
+				t.Errorf("package %s: got value %s; want %s", name, got, test.val)
+			}
+		} else {
+			if test.val != "" {
+				t.Errorf("package %s: no constant found; want %s", name, test.val)
+			}
 		}
 	}
 }
diff --git a/src/go/types/conversions.go b/src/go/types/conversions.go
index 2bf1e2d5e3..81a65838fe 100644
--- a/src/go/types/conversions.go
+++ b/src/go/types/conversions.go
@@ -46,16 +46,19 @@ func (check *Checker) conversion(x *operand, T Type) {
 	// The conversion argument types are final. For untyped values the
 	// conversion provides the type, per the spec: "A constant may be
 	// given a type explicitly by a constant declaration or conversion,...".
-	final := x.typ
 	if isUntyped(x.typ) {
-		final = T
+		final := T
 		// - For conversions to interfaces, use the argument's default type.
 		// - For conversions of untyped constants to non-constant types, also
 		//   use the default type (e.g., []byte("foo") should report string
 		//   not []byte as type for the constant "foo").
 		// - Keep untyped nil for untyped nil arguments.
+		// - For integer to string conversions, keep the argument type.
+		//   (See also the TODO below.)
 		if IsInterface(T) || constArg && !isConstType(T) {
 			final = Default(x.typ)
+		} else if isInteger(x.typ) && isString(T) {
+			final = x.typ
 		}
 		check.updateExprType(x.expr, final, true)
 	}
@@ -63,6 +66,16 @@ func (check *Checker) conversion(x *operand, T Type) {
 	x.typ = T
 }
 
+// TODO(gri) convertibleTo checks if T(x) is valid. It assumes that the type
+// of x is fully known, but that's not the case for say string(1<<s + 1.0):
+// Here, the type of 1<<s + 1.0 will be UntypedFloat which will lead to the
+// (correct!) refusal of the conversion. But the reported error is essentially
+// "cannot convert untyped float value to string", yet the correct error (per
+// the spec) is that we cannot shift a floating-point value: 1 in 1<<s should
+// be converted to UntypedFloat because of the addition of 1.0. Fixing this
+// is tricky because we'd have to run updateExprType on the argument first.
+// (Issue #21982.)
+
 func (x *operand) convertibleTo(conf *Config, T Type) bool {
 	// "x is assignable to T"
 	if x.assignableTo(conf, T, nil) {
diff --git a/src/go/types/expr.go b/src/go/types/expr.go
index 461f0a525b..01c97afc07 100644
--- a/src/go/types/expr.go
+++ b/src/go/types/expr.go
@@ -402,9 +402,10 @@ func (check *Checker) updateExprType(x ast.Expr, typ Type, final bool) {
 
 	case *ast.UnaryExpr:
 		// If x is a constant, the operands were constants.
-		// They don't need to be updated since they never
-		// get "materialized" into a typed value; and they
-		// will be processed at the end of the type check.
+		// The operands don't need to be updated since they
+		// never get "materialized" into a typed value. If
+		// left in the untyped map, they will be processed
+		// at the end of the type check.
 		if old.val != nil {
 			break
 		}
@@ -443,12 +444,21 @@ func (check *Checker) updateExprType(x ast.Expr, typ Type, final bool) {
 	// Remove it from the map of yet untyped expressions.
 	delete(check.untyped, x)
 
-	// If x is the lhs of a shift, its final type must be integer.
-	// We already know from the shift check that it is representable
-	// as an integer if it is a constant.
-	if old.isLhs && !isInteger(typ) {
-		check.invalidOp(x.Pos(), "shifted operand %s (type %s) must be integer", x, typ)
-		return
+	if old.isLhs {
+		// If x is the lhs of a shift, its final type must be integer.
+		// We already know from the shift check that it is representable
+		// as an integer if it is a constant.
+		if !isInteger(typ) {
+			check.invalidOp(x.Pos(), "shifted operand %s (type %s) must be integer", x, typ)
+			return
+		}
+	} else if old.val != nil {
+		// If x is a constant, it must be representable as a value of typ.
+		c := operand{old.mode, x, old.typ, old.val, 0}
+		check.convertUntyped(&c, typ)
+		if c.mode == invalid {
+			return
+		}
 	}
 
 	// Everything's fine, record final type and value for x.
diff --git a/src/go/types/testdata/shifts.src b/src/go/types/testdata/shifts.src
index dc029fc647..ca288290d6 100644
--- a/src/go/types/testdata/shifts.src
+++ b/src/go/types/testdata/shifts.src
@@ -345,3 +345,12 @@ func issue11594() {
 	_ = complex64 /* ERROR "must be integer" */ (0) << 3
 	_ = complex64 /* ERROR "must be integer" */ (0) >> 4
 }
+
+func issue21727() {
+	var s uint
+	var a = make([]int, 1<<s + 1.2 /* ERROR "truncated to int" */ )
+	var _ = a[1<<s - 2.3 /* ERROR "truncated to int" */ ]
+	var _ int = 1<<s + 3.4 /* ERROR "truncated to int" */
+	var _ = string(1 << s)
+	var _ = string(1.0 /* ERROR "cannot convert" */ << s)
+}
-- 
2.48.1