From a68deb25ffcbdbc5757f63c850a47c1d6bf0cb14 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 18 Sep 2013 11:31:46 -0700 Subject: [PATCH] go.tools/go/types: "declared but not used" error for type switches Also: - added more tests - removed Var.Used accessor: it's not meaningful for clients since it does not reflect actual use/def information - fixed position for short variable declaration errors R=adonovan CC=golang-dev https://golang.org/cl/13240051 --- go/types/assignments.go | 2 +- go/types/check.go | 1 + go/types/objects.go | 1 - go/types/resolver.go | 20 +++++++++++++++++++- go/types/stmt.go | 28 +++++++++++++++++----------- go/types/testdata/stmt0.src | 2 +- go/types/testdata/vardecl.src | 31 +++++++++++++++++++++++++++++-- 7 files changed, 68 insertions(+), 17 deletions(-) diff --git a/go/types/assignments.go b/go/types/assignments.go index 06fb2abf..4f0db273 100644 --- a/go/types/assignments.go +++ b/go/types/assignments.go @@ -355,7 +355,7 @@ func (check *checker) shortVarDecl(lhs, rhs []ast.Expr) { scope.Insert(obj) } if n == scope.Len() { - check.errorf(vars[0].Pos(), "no new variables on left side of :=") + check.errorf(lhs[0].Pos(), "no new variables on left side of :=") } } diff --git a/go/types/check.go b/go/types/check.go index 9a2f257a..a1bbbcc3 100644 --- a/go/types/check.go +++ b/go/types/check.go @@ -37,6 +37,7 @@ type checker struct { methods map[string][]*Func // maps type names to associated methods conversions map[*ast.CallExpr]bool // set of type-checked conversions (to distinguish from calls) untyped map[ast.Expr]exprInfo // map of expressions without final type + lhsVarsList [][]*Var // type switch lhs variable sets, for 'declared but not used' errors firstErr error // first error encountered Info // collected type info diff --git a/go/types/objects.go b/go/types/objects.go index 117e8c27..a8bb549d 100644 --- a/go/types/objects.go +++ b/go/types/objects.go @@ -179,7 +179,6 @@ func NewField(pos token.Pos, pkg *Package, name string, typ Type, anonymous bool } func (obj *Var) Anonymous() bool { return obj.anonymous } -func (obj *Var) Used() bool { return obj.used } func (obj *Var) String() string { return obj.toString("var", obj.typ) } // A Func represents a declared function, concrete method, or abstract diff --git a/go/types/resolver.go b/go/types/resolver.go index 20a764de..b2c54044 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -391,8 +391,26 @@ func (check *checker) resolveFiles(files []*ast.File) { } // Phase 5: Check for declared but not used packages and variables. - // Note: must happen after checking all functions because closures may affect outer scopes + + // Each set of implicitly declared lhs variables of a type switch acts collectively + // as a single lhs variable. If any one was 'used', all of them are 'used'. Handle + // them before the general analysis. + for _, vars := range check.lhsVarsList { + // len(vars) > 0 + var used bool + for _, v := range vars { + if v.used { + used = true + } + v.used = true // avoid later error + } + if !used { + v := vars[0] + check.errorf(v.pos, "%s declared but not used", v.name) + } + } + for _, f := range check.funcList { // spec: "Implementation restriction: A compiler may make it illegal to // declare a variable inside a function body if the variable is never used." diff --git a/go/types/stmt.go b/go/types/stmt.go index 8e8f5242..0315f866 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -209,12 +209,12 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { case *ast.GoStmt: var x operand check.call(&x, s.Call) - // TODO(gri) If a builtin is called, the builtin must be valid this context. + // TODO(gri) If a builtin is called, the builtin must be valid in this context. case *ast.DeferStmt: var x operand check.call(&x, s.Call) - // TODO(gri) If a builtin is called, the builtin must be valid this context. + // TODO(gri) If a builtin is called, the builtin must be valid in this context. case *ast.ReturnStmt: sig := check.funcSig @@ -371,7 +371,7 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { check.invalidAST(s.Pos(), "incorrect form of type switch guard") return } - check.recordObject(lhs, nil) // lhs is implicitly declared in each cause clause + check.recordObject(lhs, nil) // lhs variable is implicitly declared in each cause clause rhs = guard.Rhs[0] @@ -398,6 +398,7 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { } check.multipleDefaults(s.Body.List) + var lhsVars []*Var // set of implicitly declared lhs variables for _, s := range s.Body.List { clause, _ := s.(*ast.CaseClause) if clause == nil { @@ -423,20 +424,25 @@ func (check *checker) stmt(s ast.Stmt, fallthroughOk bool) { T = x.typ } obj := NewVar(lhs.Pos(), check.pkg, lhs.Name, T) - // For now we mark all implicitly declared variables as used. If we don't, - // we will get an error for each implicitly declared but unused variable, - // even if there are others belonging to the same type switch which are used. - // The right solution will count any use of an implicit variable in this - // switch as a use for all of them, but we cannot make that decision until - // we have seen all code, including possibly nested closures. - // TODO(gri) Fix this! - obj.used = true + // For the "declared but not used" error, all lhs variables act as + // one; i.e., if any one of them is 'used', all of them are 'used'. + // Collect them for later analysis. + lhsVars = append(lhsVars, obj) check.declareObj(check.topScope, nil, obj) check.recordImplicit(clause, obj) } check.stmtList(clause.Body, false) check.closeScope() } + // If a lhs variable was declared but there were no case clauses, make sure + // we have at least one (dummy) 'unused' variable to force an error message. + if len(lhsVars) == 0 && lhs != nil { + lhsVars = []*Var{NewVar(lhs.Pos(), check.pkg, lhs.Name, x.typ)} + } + // Record lhs variables for this type switch, if any. + if len(lhsVars) > 0 { + check.lhsVarsList = append(check.lhsVarsList, lhsVars) + } case *ast.SelectStmt: check.multipleDefaults(s.Body.List) diff --git a/go/types/testdata/stmt0.src b/go/types/testdata/stmt0.src index 355192a7..4856eebc 100644 --- a/go/types/testdata/stmt0.src +++ b/go/types/testdata/stmt0.src @@ -275,7 +275,7 @@ func typeswitches() { default /* ERROR "multiple defaults" */ : } - switch x := x.(type) {} + switch x /* ERROR "declared but not used" */ := x.(type) {} switch x := x.(type) { case int: diff --git a/go/types/testdata/vardecl.src b/go/types/testdata/vardecl.src index b09b81f5..5b7c806b 100644 --- a/go/types/testdata/vardecl.src +++ b/go/types/testdata/vardecl.src @@ -94,8 +94,35 @@ func (r T) _(a, b, c int) (u, v, w int) { x /* ERROR "declared but not used" */ := 0 } - // TODO(gri) Add test cases for type switches once they work - // correctly with implicitly declared variables. + var t interface{} + switch t /* ERROR "declared but not used" */ := t.(type) {} + + switch t /* ERROR "declared but not used" */ := t.(type) { + case int: + } + + switch t /* ERROR "declared but not used" */ := t.(type) { + case int: + case float32, complex64: + t = nil + } + + switch t := t.(type) { + case int: + case float32, complex64: + _ = t + } + + switch t := t.(type) { + case int: + case float32: + case string: + _ = func() string { + return t + } + } + + switch t := t; t /* ERROR "declared but not used" */ := t.(type) {} var z1 /* ERROR "declared but not used" */ int var z2 int