From 6fa1336531b0d1dac2b18123d74bcec4e0defdd9 Mon Sep 17 00:00:00 2001
From: Robert Griesemer <gri@golang.org>
Date: Wed, 23 Jan 2019 18:10:54 -0800
Subject: [PATCH] go/types: permit signed integer shift count

Permit shifts by non-constant signed integer shift counts.
Share logic for constant shift counts in non-constant
shifts and improve error messages a little bit.

R=Go1.13

Updates #19113.

Change-Id: Ia01d83ca8aa60a6a3f4c49f026e0c46396f852be
Reviewed-on: https://go-review.googlesource.com/c/159317
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
---
 src/go/types/expr.go             | 37 ++++++++++++++++----------------
 src/go/types/stdlib_test.go      |  1 +
 src/go/types/testdata/decls1.src |  4 ++--
 src/go/types/testdata/expr1.src  |  8 +++----
 src/go/types/testdata/shifts.src | 26 ++++++++++++++++------
 5 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/src/go/types/expr.go b/src/go/types/expr.go
index 0dc007069f..66d62d6885 100644
--- a/src/go/types/expr.go
+++ b/src/go/types/expr.go
@@ -655,11 +655,10 @@ func (check *Checker) shift(x, y *operand, e *ast.BinaryExpr, op token.Token) {
 		return
 	}
 
-	// spec: "The right operand in a shift expression must have unsigned
-	// integer type or be an untyped constant representable by a value of
-	// type uint."
+	// spec: "The right operand in a shift expression must have integer type
+	// or be an untyped constant representable by a value of type uint."
 	switch {
-	case isUnsigned(y.typ):
+	case isInteger(y.typ):
 		// nothing to do
 	case isUntyped(y.typ):
 		check.convertUntyped(y, Typ[Uint])
@@ -668,21 +667,28 @@ func (check *Checker) shift(x, y *operand, e *ast.BinaryExpr, op token.Token) {
 			return
 		}
 	default:
-		check.invalidOp(y.pos(), "shift count %s must be unsigned integer", y)
+		check.invalidOp(y.pos(), "shift count %s must be integer", y)
 		x.mode = invalid
 		return
 	}
 
+	var yval constant.Value
+	if y.mode == constant_ {
+		// rhs must be an integer value
+		// (Either it was of an integer type already, or it was
+		// untyped and successfully converted to a uint above.)
+		yval = constant.ToInt(y.val)
+		assert(yval.Kind() == constant.Int)
+		if constant.Sign(yval) < 0 {
+			check.invalidOp(y.pos(), "negative shift count %s", y)
+			x.mode = invalid
+			return
+		}
+	}
+
 	if x.mode == constant_ {
 		if y.mode == constant_ {
-			// rhs must be an integer value
-			yval := constant.ToInt(y.val)
-			if yval.Kind() != constant.Int {
-				check.invalidOp(y.pos(), "shift count %s must be unsigned integer", y)
-				x.mode = invalid
-				return
-			}
-			// rhs must be within reasonable bounds
+			// rhs must be within reasonable bounds in constant shifts
 			const shiftBound = 1023 - 1 + 52 // so we can express smallestFloat64
 			s, ok := constant.Uint64Val(yval)
 			if !ok || s > shiftBound {
@@ -741,11 +747,6 @@ func (check *Checker) shift(x, y *operand, e *ast.BinaryExpr, op token.Token) {
 		}
 	}
 
-	// constant rhs must be >= 0
-	if y.mode == constant_ && constant.Sign(y.val) < 0 {
-		check.invalidOp(y.pos(), "shift count %s must not be negative", y)
-	}
-
 	// non-constant shift - lhs must be an integer
 	if !isInteger(x.typ) {
 		check.invalidOp(x.pos(), "shifted operand %s must be integer", x)
diff --git a/src/go/types/stdlib_test.go b/src/go/types/stdlib_test.go
index 84908fd190..b254b29bdf 100644
--- a/src/go/types/stdlib_test.go
+++ b/src/go/types/stdlib_test.go
@@ -167,6 +167,7 @@ func TestStdFixed(t *testing.T) {
 	}
 
 	testTestDir(t, filepath.Join(runtime.GOROOT(), "test", "fixedbugs"),
+		"bug073.go",                           // checks for unsigned integer shift - disabled for now
 		"bug248.go", "bug302.go", "bug369.go", // complex test instructions - ignore
 		"issue6889.go",   // gc-specific test
 		"issue7746.go",   // large constants - consumes too much memory
diff --git a/src/go/types/testdata/decls1.src b/src/go/types/testdata/decls1.src
index 07405469a4..e6beb78358 100644
--- a/src/go/types/testdata/decls1.src
+++ b/src/go/types/testdata/decls1.src
@@ -43,7 +43,7 @@ var (
 	s11 = &v
 	s12 = -(u + *t11) / *&v
 	s13 = a /* ERROR "shifted operand" */ << d
-	s14 = i << j /* ERROR "must be unsigned" */
+	s14 = i << j
 	s18 = math.Pi * 10.0
 	s19 = s1 /* ERROR "cannot call" */ ()
  	s20 = f0 /* ERROR "no value" */ ()
@@ -62,7 +62,7 @@ var (
 	t11 *complex64 = &v
 	t12 complex64 = -(u + *t11) / *&v
 	t13 int = a /* ERROR "shifted operand" */ << d
-	t14 int = i << j /* ERROR "must be unsigned" */
+	t14 int = i << j
 	t15 math /* ERROR "not in selector" */
 	t16 math.xxx /* ERROR "not declared" */
 	t17 math /* ERROR "not a type" */ .Pi
diff --git a/src/go/types/testdata/expr1.src b/src/go/types/testdata/expr1.src
index eaaf610b03..4ead815158 100644
--- a/src/go/types/testdata/expr1.src
+++ b/src/go/types/testdata/expr1.src
@@ -35,8 +35,8 @@ func _(x, y int, z myint) {
 	x = x * y
 	x = x / y
 	x = x % y
-	x = x << y // ERROR must be unsigned integer
-	x = x >> y // ERROR must be unsigned integer
+	x = x << y
+	x = x >> y
 
 	z = z + 1
 	z = z + 1.0
@@ -46,8 +46,8 @@ func _(x, y int, z myint) {
 	z = z /* ERROR mismatched types */ * y
 	z = z /* ERROR mismatched types */ / y
 	z = z /* ERROR mismatched types */ % y
-	z = z << y // ERROR must be unsigned integer
-	z = z >> y // ERROR must be unsigned integer
+	z = z << y
+	z = z >> y
 }
 
 type myuint uint
diff --git a/src/go/types/testdata/shifts.src b/src/go/types/testdata/shifts.src
index 52e340ec65..ebc95ba4d7 100644
--- a/src/go/types/testdata/shifts.src
+++ b/src/go/types/testdata/shifts.src
@@ -10,9 +10,21 @@ func shifts0() {
 		s = 10
 		_ = 0<<0
 		_ = 1<<s
-		_ = 1<<- /* ERROR "overflows uint" */ 1
+		_ = 1<<- /* ERROR "negative shift count" */ 1
+		// For the test below we may decide to convert to int
+		// rather than uint and then report a negative shift
+		// count instead, which might be a better error. The
+		// (minor) difference is that this would restrict the
+		// shift count range by half (from all uint values to
+		// the positive int values).
+		// This depends on the exact spec wording which is not
+		// done yet.
+		// TODO(gri) revisit and adjust when spec change is done
+		_ = 1<<- /* ERROR "truncated to uint" */ 1.0
 		_ = 1<<1075 /* ERROR "invalid shift" */
 		_ = 2.0<<1
+		_ = 1<<1.0
+		_ = 1<<(1+0i)
 
 		_ int = 2<<s
 		_ float32 = 2<<s
@@ -35,11 +47,13 @@ func shifts1() {
 		u uint
 
 		_ = 1<<0
-		_ = 1<<i /* ERROR "must be unsigned" */
+		_ = 1<<i
 		_ = 1<<u
 		_ = 1<<"foo" /* ERROR "cannot convert" */
 		_ = i<<0
-		_ = i<<- /* ERROR "overflows uint" */ 1
+		_ = i<<- /* ERROR "negative shift count" */ 1
+		_ = i<<1.0
+		_ = 1<<(1+0i)
 		_ = 1 /* ERROR "overflows" */ <<100
 
 		_ uint = 1 << 0
@@ -47,10 +61,10 @@ func shifts1() {
 		_ float32 = 1 /* ERROR "must be integer" */ << u
 
 		// for issue 14822
-		_ = 1<<( /* ERROR "invalid shift count" */ 1<<63)
-		_ = 1<<( /* ERROR "overflows uint" */ 1<<64)
+		_ = 1<<( /* ERROR "invalid shift count" */ 1<<64-1)
+		_ = 1<<( /* ERROR "invalid shift count" */ 1<<64)
 		_ = u<<(1<<63) // valid
-		_ = u<<( /* ERROR "overflows uint" */ 1<<64)
+		_ = u<<(1<<64) // valid
 	)
 }