From c4ca0e24893ba829194ddbfe910a430e28fd06d6 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 13 Mar 2014 13:22:44 -0700 Subject: [PATCH] go.tools/go/types: don't change dot-imported object's parents With this CL, an Object.Parent() Scope is always the scope in which the object was originally declared. For dot-imported objects, that is the package scope of the package from which the objects are imported (not the file scope into which they are imported). Also: - Changed Scope.Insert to be agnostic regarding blank identifiers - blank identifiers must be handled outside. - Fixed handling of blank labels: they are never declared. Fixes golang/go#7537. LGTM=adonovan R=adonovan CC=golang-codereviews https://golang.org/cl/75570043 --- go/types/api.go | 13 +++++--- go/types/assignments.go | 3 +- go/types/decl.go | 14 +++++--- go/types/labels.go | 65 ++++++++++++++++++------------------ go/types/resolver.go | 2 -- go/types/scope.go | 19 +++-------- go/types/testdata/labels.src | 26 ++++++++++++++- 7 files changed, 84 insertions(+), 58 deletions(-) diff --git a/go/types/api.go b/go/types/api.go index 25f8e14b..37ac3d08 100644 --- a/go/types/api.go +++ b/go/types/api.go @@ -174,10 +174,15 @@ type Info struct { // Selections maps selector expressions to their corresponding selections. Selections map[*ast.SelectorExpr]*Selection - // Scopes maps ast.Nodes to the scopes they define. Note that package scopes - // are not associated with a specific node but with all files belonging to a - // package. Thus, the package scope can be found in the type-checked package - // object. + // Scopes maps ast.Nodes to the scopes they define. Package scopes are not + // associated with a specific node but with all files belonging to a package. + // Thus, the package scope can be found in the type-checked Package object. + // Scopes nest, with the Universe scope being the outermost scope, enclosing + // the package scope, which contains (one or more) files scopes, which enclose + // function scopes which in turn enclose statement and function literal scopes. + // Note that even though package-level functions are declared in the package + // scope, the function scopes are embedded in the file scope of the file + // containing the function declaration. // // The following node types may appear in Scopes: // diff --git a/go/types/assignments.go b/go/types/assignments.go index e672e0be..4d041b85 100644 --- a/go/types/assignments.go +++ b/go/types/assignments.go @@ -277,7 +277,8 @@ func (check *checker) shortVarDecl(pos token.Pos, lhs, rhs []ast.Expr) { 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 later. + // 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 diff --git a/go/types/decl.go b/go/types/decl.go index 1bb2b5f8..a81feb90 100644 --- a/go/types/decl.go +++ b/go/types/decl.go @@ -21,10 +21,16 @@ func (check *checker) reportAltDecl(obj Object) { } func (check *checker) declare(scope *Scope, id *ast.Ident, obj Object) { - if alt := scope.Insert(obj); alt != nil { - check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name()) - check.reportAltDecl(alt) - return + // spec: "The blank identifier, represented by the underscore + // character _, may be used in a declaration like any other + // identifier but the declaration does not introduce a new + // binding." + if obj.Name() != "_" { + if alt := scope.Insert(obj); alt != nil { + check.errorf(obj.Pos(), "%s redeclared in this block", obj.Name()) + check.reportAltDecl(alt) + return + } } if id != nil { check.recordDef(id, obj) diff --git a/go/types/labels.go b/go/types/labels.go index d9d3fb73..ee89993a 100644 --- a/go/types/labels.go +++ b/go/types/labels.go @@ -129,41 +129,42 @@ func (check *checker) blockBranches(all *Scope, parent *block, lstmt *ast.Labele } case *ast.LabeledStmt: - // declare label - name := s.Label.Name - lbl := NewLabel(s.Label.Pos(), name) - if alt := all.Insert(lbl); alt != nil { - check.errorf(lbl.pos, "label %s already declared", name) - check.reportAltDecl(alt) - // ok to continue - } else { - b.insert(s) - check.recordDef(s.Label, lbl) - } - // resolve matching forward jumps and remove them from fwdJumps - i := 0 - for _, jmp := range fwdJumps { - if jmp.Label.Name == name { - // match - lbl.used = true - check.recordUse(jmp.Label, lbl) - if jumpsOverVarDecl(jmp) { - check.errorf( - jmp.Label.Pos(), - "goto %s jumps over variable declaration at line %d", - name, - check.fset.Position(varDeclPos).Line, - ) - // ok to continue - } + // declare non-blank label + if name := s.Label.Name; name != "_" { + lbl := NewLabel(s.Label.Pos(), name) + if alt := all.Insert(lbl); alt != nil { + check.softErrorf(lbl.pos, "label %s already declared", name) + check.reportAltDecl(alt) + // ok to continue } else { - // no match - record new forward jump - fwdJumps[i] = jmp - i++ + b.insert(s) + check.recordDef(s.Label, lbl) } + // resolve matching forward jumps and remove them from fwdJumps + i := 0 + for _, jmp := range fwdJumps { + if jmp.Label.Name == name { + // match + lbl.used = true + check.recordUse(jmp.Label, lbl) + if jumpsOverVarDecl(jmp) { + check.softErrorf( + jmp.Label.Pos(), + "goto %s jumps over variable declaration at line %d", + name, + check.fset.Position(varDeclPos).Line, + ) + // ok to continue + } + } else { + // no match - record new forward jump + fwdJumps[i] = jmp + i++ + } + } + fwdJumps = fwdJumps[:i] + lstmt = s } - fwdJumps = fwdJumps[:i] - lstmt = s stmtBranches(s.Stmt) case *ast.BranchStmt: diff --git a/go/types/resolver.go b/go/types/resolver.go index 29ebd92c..f674befc 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -196,8 +196,6 @@ func (check *checker) collectObjects() { // A package scope may contain non-exported objects, // do not import them! if obj.Exported() { - // Note: This will change each imported object's scope! - // May be an issue for type aliases. check.declare(fileScope, nil, obj) check.recordImplicit(s, obj) } diff --git a/go/types/scope.go b/go/types/scope.go index 406bb7e5..f33cc68c 100644 --- a/go/types/scope.go +++ b/go/types/scope.go @@ -81,24 +81,13 @@ func (s *Scope) LookupParent(name string) Object { return nil } -// TODO(gri): Should Insert not be exported? - // Insert attempts to insert an object obj into scope s. // If s already contains an alternative object alt with // the same name, Insert leaves s unchanged and returns alt. -// Otherwise it inserts obj, sets the object's scope to -// s, and returns nil. Objects with blank "_" names are -// not inserted, but have their parent field set to s. +// Otherwise it inserts obj, sets the object's parent scope +// if not already set, and returns nil. func (s *Scope) Insert(obj Object) Object { name := obj.Name() - // spec: "The blank identifier, represented by the underscore - // character _, may be used in a declaration like any other - // identifier but the declaration does not introduce a new - // binding." - if name == "_" { - obj.setParent(s) - return nil - } if alt := s.elems[name]; alt != nil { return alt } @@ -106,7 +95,9 @@ func (s *Scope) Insert(obj Object) Object { s.elems = make(map[string]Object) } s.elems[name] = obj - obj.setParent(s) + if obj.Parent() == nil { + obj.setParent(s) + } return nil } diff --git a/go/types/testdata/labels.src b/go/types/testdata/labels.src index 53b8ed36..102ffc7c 100644 --- a/go/types/testdata/labels.src +++ b/go/types/testdata/labels.src @@ -180,4 +180,28 @@ L3: goto L2 goto L3 } -} \ No newline at end of file +} + +// Blank labels are never declared. + +func f4() { +_: +_: // multiple blank labels are ok + goto _ /* ERROR "label _ not declared" */ +} + +func f5() { +_: + for { + break _ /* ERROR "invalid break label _" */ + continue _ /* ERROR "invalid continue label _" */ + } +} + +func f6() { +_: + switch { + default: + break _ /* ERROR "invalid break label _" */ + } +}