]> Cypherpunks repositories - gostls13.git/commitdiff
go/types: track local cycles using same mechanism as for global objects
authorRobert Griesemer <gri@golang.org>
Tue, 21 Aug 2018 16:50:58 +0000 (09:50 -0700)
committerRobert Griesemer <gri@golang.org>
Thu, 30 Aug 2018 22:54:29 +0000 (22:54 +0000)
For Go 1.11, cycle tracking of global (package-level) objects was changed
to use a Checker-level object path rather than relying on the explicit
path parameter that is passed around to some (but not all) type-checker
functions.

This change now uses the same mechanism for the detection of local
type cycles (local non-type objects cannot create cycles by definition
of the spec).

As a result, local alias cycles are now correctly detected as well
(issue #27106).

The path parameter that is explicitly passed around to some type-checker
methods is still present and will be removed in a follow-up CL.

Also:
- removed useCycleMarking flag and respective dead code
- added a couple more tests
- improved documentation

Fixes #27106.
Updates #25773.

Change-Id: I7cbf304bceb43a8d52e6483dcd0fa9ef7e1ea71c
Reviewed-on: https://go-review.googlesource.com/130455
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
src/go/types/check.go
src/go/types/decl.go
src/go/types/resolver.go
src/go/types/testdata/cycles.src
src/go/types/typexpr.go

index 76d9c8917cb76c66358381b46b8408b54d081be3..5b796be40d4c1da44972d5dfa0bd52e0495eafdb 100644 (file)
@@ -76,7 +76,7 @@ type Checker struct {
        fset *token.FileSet
        pkg  *Package
        *Info
-       objMap map[Object]*declInfo   // maps package-level object to declaration info
+       objMap map[Object]*declInfo   // maps package-level objects and (non-interface) methods to declaration info
        impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package
 
        // information collected during type-checking of a set of package files
index 11b68583e38c08202bfd3d4774c74f95821a6f1d..d845789143305388310e8b5074e970d08f4987ea 100644 (file)
@@ -64,13 +64,6 @@ func objPathString(path []Object) string {
        return s
 }
 
-// useCycleMarking enables the new coloring-based cycle marking scheme
-// for package-level objects. Set this flag to false to disable this
-// code quickly and revert to the existing mechanism (and comment out
-// some of the new tests in cycles5.src that will fail again).
-// TODO(gri) remove this for Go 1.12
-const useCycleMarking = true
-
 // objDecl type-checks the declaration of obj in its respective (file) context.
 // See check.typ for the details on def and path.
 func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
@@ -147,7 +140,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
                // order code.
                switch obj := obj.(type) {
                case *Const:
-                       if useCycleMarking && check.typeCycle(obj) {
+                       if check.typeCycle(obj) {
                                obj.typ = Typ[Invalid]
                                break
                        }
@@ -156,7 +149,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
                        }
 
                case *Var:
-                       if useCycleMarking && check.typeCycle(obj) {
+                       if check.typeCycle(obj) {
                                obj.typ = Typ[Invalid]
                                break
                        }
@@ -190,7 +183,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
                                }
                        }
 
-                       if useCycleMarking && check.typeCycle(obj) {
+                       if check.typeCycle(obj) {
                                // break cycle
                                // (without this, calling underlying()
                                // below may lead to an endless loop
@@ -200,7 +193,7 @@ func (check *Checker) objDecl(obj Object, def *Named, path []*TypeName) {
                        }
 
                case *Func:
-                       if useCycleMarking && check.typeCycle(obj) {
+                       if check.typeCycle(obj) {
                                // Don't set obj.typ to Typ[Invalid] here
                                // because plenty of code type-asserts that
                                // functions have a *Signature type. Grey
@@ -273,10 +266,15 @@ var cutCycle = NewTypeName(token.NoPos, nil, "!", nil)
 // TODO(gri) rename s/typeCycle/cycle/ once we don't need the other
 // cycle method anymore.
 func (check *Checker) typeCycle(obj Object) (isCycle bool) {
-       d := check.objMap[obj]
-       if d == nil {
-               check.dump("%v: %s should have been declared", obj.Pos(), obj)
-               unreachable()
+       // The object map contains the package scope objects and the non-interface methods.
+       if debug {
+               info := check.objMap[obj]
+               inObjMap := info != nil && (info.fdecl == nil || info.fdecl.Recv == nil) // exclude methods
+               isPkgObj := obj.Parent() == check.pkg.scope
+               if isPkgObj != inObjMap {
+                       check.dump("%v: inconsistent object map for %s (isPkgObj = %v, inObjMap = %v)", obj.Pos(), obj, isPkgObj, inObjMap)
+                       unreachable()
+               }
        }
 
        // Given the number of constants and variables (nval) in the cycle
@@ -312,8 +310,25 @@ func (check *Checker) typeCycle(obj Object) (isCycle bool) {
                                // that we type-check methods when we type-check their
                                // receiver base types.
                                return false
-                       case !check.objMap[obj].alias:
-                               hasTDef = true
+                       default:
+                               // Determine if the type name is an alias or not. For
+                               // package-level objects, use the object map which
+                               // provides syntactic information (which doesn't rely
+                               // on the order in which the objects are set up). For
+                               // local objects, we can rely on the order, so use
+                               // the object's predicate.
+                               // TODO(gri) It would be less fragile to always access
+                               // the syntactic information. We should consider storing
+                               // this information explicitly in the object.
+                               var alias bool
+                               if d := check.objMap[obj]; d != nil {
+                                       alias = d.alias // package-level object
+                               } else {
+                                       alias = obj.IsAlias() // function local object
+                               }
+                               if !alias {
+                                       hasTDef = true
+                               }
                        }
                case *Func:
                        // ignored for now
@@ -552,15 +567,13 @@ func (check *Checker) addMethodDecls(obj *TypeName) {
                }
        }
 
-       if useCycleMarking {
-               // Suppress detection of type cycles occurring through method
-               // declarations - they wouldn't exist if methods were type-
-               // checked separately from their receiver base types. See also
-               // comment at the end of Checker.typeDecl.
-               // TODO(gri) Remove this once methods are type-checked separately.
-               check.push(cutCycle)
-               defer check.pop()
-       }
+       // Suppress detection of type cycles occurring through method
+       // declarations - they wouldn't exist if methods were type-
+       // checked separately from their receiver base types. See also
+       // comment at the end of Checker.typeDecl.
+       // TODO(gri) Remove this once methods are type-checked separately.
+       check.push(cutCycle)
+       defer check.pop()
 
        // type-check methods
        for _, m := range methods {
@@ -730,8 +743,10 @@ func (check *Checker) declStmt(decl ast.Decl) {
                                // the innermost containing block."
                                scopePos := s.Name.Pos()
                                check.declare(check.scope, s.Name, obj, scopePos)
+                               // mark and unmark type before calling typeDecl; its type is still nil (see Checker.objDecl)
+                               obj.setColor(grey + color(check.push(obj)))
                                check.typeDecl(obj, s.Type, nil, nil, s.Assign.IsValid())
-
+                               check.pop().setColor(black)
                        default:
                                check.invalidAST(s.Pos(), "const, type, or var declaration expected")
                        }
index 5cbaba187b964b4b56d6424de63adeaf068ad666..a462912cd149caed272ddf60bdb448eaf7e1b6d1 100644 (file)
@@ -420,6 +420,10 @@ func (check *Checker) collectObjects() {
                                        check.recordDef(d.Name, obj)
                                }
                                info := &declInfo{file: fileScope, fdecl: d}
+                               // Methods are not package-level objects but we still track them in the
+                               // object map so that we can handle them like regular functions (if the
+                               // receiver is invalid); also we need their fdecl info when associating
+                               // them with their receiver base type, below.
                                check.objMap[obj] = info
                                obj.setOrder(uint32(len(check.objMap)))
 
index 59f112dba1db7f7a808d0579278e5ec7707a0f60..a9af46a9337b1b62be9c70a8e2230453d673a4d8 100644 (file)
@@ -158,6 +158,8 @@ type T20 chan [unsafe.Sizeof(func(ch T20){ _ = <-ch })]byte
 type T22 = chan [unsafe.Sizeof(func(ch T20){ _ = <-ch })]byte
 
 func _() {
-       type T1 chan [unsafe.Sizeof(func(ch T1){ _ = <-ch })]byte
-       type T2 = chan [unsafe.Sizeof(func(ch T2){ _ = <-ch })]byte
+       type T0 func(T0)
+       type T1 /* ERROR cycle */ = func(T1)
+       type T2 chan [unsafe.Sizeof(func(ch T2){ _ = <-ch })]byte
+       type T3 /* ERROR cycle */ = chan [unsafe.Sizeof(func(ch T3){ _ = <-ch })]byte
 }
index 45ada5874bc193adbcd3ae37c69ca7a0bfad154e..1da1f01956f8600f09fe413d11d3f75d80a0b5fe 100644 (file)
@@ -71,17 +71,6 @@ func (check *Checker) ident(x *operand, e *ast.Ident, def *Named, path []*TypeNa
 
        case *TypeName:
                x.mode = typexpr
-               // package-level alias cycles are now checked by Checker.objDecl
-               if useCycleMarking {
-                       if check.objMap[obj] != nil {
-                               break
-                       }
-               }
-               if check.cycle(obj, path, true) {
-                       // maintain x.mode == typexpr despite error
-                       typ = Typ[Invalid]
-                       break
-               }
 
        case *Var:
                // It's ok to mark non-local variables, but ignore variables
@@ -169,10 +158,8 @@ func (check *Checker) typExpr(e ast.Expr, def *Named, path []*TypeName) (T Type)
 func (check *Checker) typ(e ast.Expr) Type {
        // typExpr is called with a nil path indicating an indirection:
        // push indir sentinel on object path
-       if useCycleMarking {
-               check.push(indir)
-               defer check.pop()
-       }
+       check.push(indir)
+       defer check.pop()
        return check.typExpr(e, nil, nil)
 }