]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: better errors for invalid short var decls
authorRob Findley <rfindley@google.com>
Wed, 28 Apr 2021 14:35:53 +0000 (10:35 -0400)
committerRobert Findley <rfindley@google.com>
Wed, 28 Apr 2021 20:22:15 +0000 (20:22 +0000)
This is a port of CL 312170 to go/types, adjusted to use go/ast and to
add error codes. go/parser already emits errors for non-identifiers on
the LHS of a short var decl, so a TODO is added to reconsider this
redundancy.

A new error code is added for repeated identifiers in short var decls.
This is a bit specific, but I considered it to be a unique kind of
error.

The x/tools tests for this port turned up a bug: the new logic failed to
call recordDef for blank identifiers. Patchset #2 contains the fix for
this bug, both in go/types and cmd/compile/internal/types2.

Change-Id: Ibdc40b8b4ad0e0696111d431682e1f1056fd5eeb
Reviewed-on: https://go-review.googlesource.com/c/go/+/314629
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
src/cmd/compile/internal/types2/api_test.go
src/cmd/compile/internal/types2/assignments.go
src/go/types/api_test.go
src/go/types/assignments.go
src/go/types/errorcodes.go
src/go/types/fixedbugs/issue43087.src [new file with mode: 0644]
src/go/types/testdata/stmt0.src

index c90f2e7510ecd53918808208ffcb86e27d42e4ae..873390c1e9280d6b961ad02592e52e16f39096c3 100644 (file)
@@ -555,6 +555,7 @@ func TestDefsInfo(t *testing.T) {
                {`package p2; var x int`, `x`, `var p2.x int`},
                {`package p3; type x int`, `x`, `type p3.x int`},
                {`package p4; func f()`, `f`, `func p4.f()`},
+               {`package p5; func f() int { x, _ := 1, 2; return x }`, `_`, `var _ int`},
 
                // generic types must be sanitized
                // (need to use sufficiently nested types to provoke unexpanded types)
index ec9fdbba629c83bc864f8d586b94c1a04b406c79..583118c8b235c5e6ac6a419e97add3e402c1cd47 100644 (file)
@@ -344,16 +344,14 @@ func (check *Checker) shortVarDecl(pos syntax.Pos, lhs, rhs []syntax.Expr) {
                }
 
                name := ident.Value
-               if name == "_" {
-                       continue
-               }
-
-               if seen[name] {
-                       check.errorf(lhs, "%s repeated on left side of :=", lhs)
-                       hasErr = true
-                       continue
+               if name != "_" {
+                       if seen[name] {
+                               check.errorf(lhs, "%s repeated on left side of :=", lhs)
+                               hasErr = true
+                               continue
+                       }
+                       seen[name] = true
                }
-               seen[name] = true
 
                // Use the correct obj if the ident is redeclared. The
                // variable's scope starts after the declaration; so we
@@ -374,7 +372,9 @@ func (check *Checker) shortVarDecl(pos syntax.Pos, lhs, rhs []syntax.Expr) {
                // declare new variable
                obj := NewVar(ident.Pos(), check.pkg, name, nil)
                lhsVars[i] = obj
-               newVars = append(newVars, obj)
+               if name != "_" {
+                       newVars = append(newVars, obj)
+               }
                check.recordDef(ident, obj)
        }
 
index 5ac91bedd2b057652ce57e2acda844bb0ad3a3e2..f37b91d5a4eba5f6fae7ebe26b5bb3415e4fa9d7 100644 (file)
@@ -401,6 +401,7 @@ func TestDefsInfo(t *testing.T) {
                {`package p2; var x int`, `x`, `var p2.x int`},
                {`package p3; type x int`, `x`, `type p3.x int`},
                {`package p4; func f()`, `f`, `func p4.f()`},
+               {`package p5; func f() int { x, _ := 1, 2; return x }`, `_`, `var _ int`},
 
                // generic types must be sanitized
                // (need to use sufficiently nested types to provoke unexpanded types)
index 3aa06e8939fca76084b0217202c4f2eb8c4b4509..18eae62184028ade83978e76483ebc20ba8c1102 100644 (file)
@@ -308,40 +308,60 @@ func (check *Checker) shortVarDecl(pos positioner, lhs, rhs []ast.Expr) {
        scope := check.scope
 
        // collect lhs variables
-       var newVars []*Var
-       var lhsVars = make([]*Var, len(lhs))
+       seen := make(map[string]bool, len(lhs))
+       lhsVars := make([]*Var, len(lhs))
+       newVars := make([]*Var, 0, len(lhs))
+       hasErr := false
        for i, lhs := range lhs {
-               var obj *Var
-               if ident, _ := lhs.(*ast.Ident); ident != nil {
-                       // Use the correct obj if the ident is redeclared. The
-                       // variable's scope starts after the declaration; so we
-                       // must use Scope.Lookup here and call Scope.Insert
-                       // (via check.declare) later.
-                       name := ident.Name
-                       if alt := scope.Lookup(name); alt != nil {
-                               // redeclared object must be a variable
-                               if alt, _ := alt.(*Var); alt != nil {
-                                       obj = alt
-                               } else {
-                                       check.errorf(lhs, _UnassignableOperand, "cannot assign to %s", lhs)
-                               }
-                               check.recordUse(ident, alt)
+               ident, _ := lhs.(*ast.Ident)
+               if ident == nil {
+                       check.useLHS(lhs)
+                       // TODO(rFindley) this is redundant with a parser error. Consider omitting?
+                       check.errorf(lhs, _BadDecl, "non-name %s on left side of :=", lhs)
+                       hasErr = true
+                       continue
+               }
+
+               name := ident.Name
+               if name != "_" {
+                       if seen[name] {
+                               check.errorf(lhs, _RepeatedDecl, "%s repeated on left side of :=", lhs)
+                               hasErr = true
+                               continue
+                       }
+                       seen[name] = true
+               }
+
+               // Use the correct obj if the ident is redeclared. The
+               // variable's scope starts after the declaration; so we
+               // must use Scope.Lookup here and call Scope.Insert
+               // (via check.declare) later.
+               if alt := scope.Lookup(name); alt != nil {
+                       check.recordUse(ident, alt)
+                       // redeclared object must be a variable
+                       if obj, _ := alt.(*Var); obj != nil {
+                               lhsVars[i] = obj
                        } else {
-                               // declare new variable, possibly a blank (_) variable
-                               obj = NewVar(ident.Pos(), check.pkg, name, nil)
-                               if name != "_" {
-                                       newVars = append(newVars, obj)
-                               }
-                               check.recordDef(ident, obj)
+                               check.errorf(lhs, _UnassignableOperand, "cannot assign to %s", lhs)
+                               hasErr = true
                        }
-               } else {
-                       check.useLHS(lhs)
-                       check.invalidAST(lhs, "cannot declare %s", lhs)
+                       continue
+               }
+
+               // declare new variable
+               obj := NewVar(ident.Pos(), check.pkg, name, nil)
+               lhsVars[i] = obj
+               if name != "_" {
+                       newVars = append(newVars, obj)
                }
+               check.recordDef(ident, obj)
+       }
+
+       // create dummy variables where the lhs is invalid
+       for i, obj := range lhsVars {
                if obj == nil {
-                       obj = NewVar(lhs.Pos(), check.pkg, "_", nil) // dummy variable
+                       lhsVars[i] = NewVar(lhs[i].Pos(), check.pkg, "_", nil)
                }
-               lhsVars[i] = obj
        }
 
        check.initVars(lhsVars, rhs, token.NoPos)
@@ -349,17 +369,18 @@ func (check *Checker) shortVarDecl(pos positioner, lhs, rhs []ast.Expr) {
        // process function literals in rhs expressions before scope changes
        check.processDelayed(top)
 
-       // declare new variables
-       if len(newVars) > 0 {
-               // spec: "The scope of a constant or variable identifier declared inside
-               // a function begins at the end of the ConstSpec or VarSpec (ShortVarDecl
-               // for short variable declarations) and ends at the end of the innermost
-               // containing block."
-               scopePos := rhs[len(rhs)-1].End()
-               for _, obj := range newVars {
-                       check.declare(scope, nil, obj, scopePos) // recordObject already called
-               }
-       } else {
+       if len(newVars) == 0 && !hasErr {
                check.softErrorf(pos, _NoNewVar, "no new variables on left side of :=")
+               return
+       }
+
+       // declare new variables
+       // spec: "The scope of a constant or variable identifier declared inside
+       // a function begins at the end of the ConstSpec or VarSpec (ShortVarDecl
+       // for short variable declarations) and ends at the end of the innermost
+       // containing block."
+       scopePos := rhs[len(rhs)-1].End()
+       for _, obj := range newVars {
+               check.declare(scope, nil, obj, scopePos) // id = nil: recordDef already called
        }
 }
index 1106cd986fea41f47c54ab53713258693d26359e..a33a4e7dcea134f2f3eba6474a6c1e8354a06fe9 100644 (file)
@@ -1357,6 +1357,15 @@ const (
        // _BadDecl occurs when a declaration has invalid syntax.
        _BadDecl
 
+       // _RepeatedDecl occurs when an identifier occurs more than once on the left
+       // hand side of a short variable declaration.
+       //
+       // Example:
+       //  func _() {
+       //      x, y, y := 1, 2, 3
+       //  }
+       _RepeatedDecl
+
        // _Todo is a placeholder for error codes that have not been decided.
        // TODO(rFindley) remove this error code after deciding on errors for generics code.
        _Todo
diff --git a/src/go/types/fixedbugs/issue43087.src b/src/go/types/fixedbugs/issue43087.src
new file mode 100644 (file)
index 0000000..ef37b4a
--- /dev/null
@@ -0,0 +1,43 @@
+// Copyright 2021 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.
+
+package p
+
+func _() {
+       a, b, b /* ERROR b repeated on left side of := */ := 1, 2, 3
+       _ = a
+       _ = b
+}
+
+func _() {
+       a, _, _ := 1, 2, 3 // multiple _'s ok
+       _ = a
+}
+
+func _() {
+       var b int
+       a, b, b /* ERROR b repeated on left side of := */ := 1, 2, 3
+       _ = a
+       _ = b
+}
+
+func _() {
+       var a []int
+       a /* ERROR expected identifier */ /* ERROR non-name .* on left side of := */ [0], b := 1, 2
+       _ = a
+       _ = b
+}
+
+func _() {
+       var a int
+       a, a /* ERROR a repeated on left side of := */ := 1, 2
+       _ = a
+}
+
+func _() {
+       var a, b int
+       a, b := /* ERROR no new variables on left side of := */ 1, 2
+       _ = a
+       _ = b
+}
index 2602d7dacf4daa450a67c235a07041b701d78ad9..76b6e70d63bacd703871aedfc02778196a7b5e6a 100644 (file)
@@ -143,11 +143,11 @@ func issue6487() {
 }
 
 func issue6766a() {
-       a, a /* ERROR redeclared */ := 1, 2
+       a, a /* ERROR a repeated on left side of := */ := 1, 2
        _ = a
-       a, b, b /* ERROR redeclared */ := 1, 2, 3
+       a, b, b /* ERROR b repeated on left side of := */ := 1, 2, 3
        _ = b
-       c, c /* ERROR redeclared */, b := 1, 2, 3
+       c, c /* ERROR c repeated on left side of := */, b := 1, 2, 3
        _ = c
        a, b := /* ERROR no new variables */ 1, 2
 }