From 9ab5154f6ceb0211d8a29005c0f4d4851f0d4e4f Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Fri, 29 Jun 2018 14:11:46 -0700 Subject: [PATCH] go/types: correctly compute cycle length The existing algorithm assumed that the length of a cycle was simply the number of elements in the cycle slice starting at the start object. However, we use a special "indir" indirection object to indicate pointer and other indirections that break the inline placement of types in composite types. These indirection objects don't exist as true named type objects, so don't count them anymore. This removes an unnecessary cycle error in one of the existing tests (testdata/issues.src:100). Also: - added more tracing support (only active if tracing is enabled) - better documentation in parts - r/check.typ/check.typExpr/ in a few of places where we don't need to record a type indirection Found while investigating #26124. Change-Id: I45341743225d979a72af3fbecfa05012b32fab67 Reviewed-on: https://go-review.googlesource.com/121755 Run-TryBot: Robert Griesemer Reviewed-by: Alan Donovan --- src/go/types/check.go | 9 +------ src/go/types/decl.go | 45 ++++++++++++++++++++++++++------ src/go/types/expr.go | 2 +- src/go/types/testdata/issues.src | 2 +- 4 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/go/types/check.go b/src/go/types/check.go index 286b1f36a95..76d9c8917cb 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -162,14 +162,7 @@ func (check *Checker) pop() Object { // pathString returns a string of the form a->b-> ... ->g for an object path [a, b, ... g]. func (check *Checker) pathString() string { - var s string - for i, p := range check.objPath { - if i > 0 { - s += "->" - } - s += p.Name() - } - return s + return objPathString(check.objPath) } // NewChecker returns a new Checker instance for a given package. diff --git a/src/go/types/decl.go b/src/go/types/decl.go index e8e01541a3e..763287adbe4 100644 --- a/src/go/types/decl.go +++ b/src/go/types/decl.go @@ -38,6 +38,8 @@ func (check *Checker) declare(scope *Scope, id *ast.Ident, obj Object, pos token } // pathString returns a string of the form a->b-> ... ->g for a path [a, b, ... g]. +// TODO(gri) remove once we don't need the old cycle detection (explicitly passed +// []*TypeName path) anymore func pathString(path []*TypeName) string { var s string for i, p := range path { @@ -49,6 +51,19 @@ func pathString(path []*TypeName) string { return s } +// objPathString returns a string of the form a->b-> ... ->g for a path [a, b, ... g]. +// TODO(gri) s/objPathString/pathString/ once we got rid of pathString above +func objPathString(path []Object) string { + var s string + for i, p := range path { + if i > 0 { + s += "->" + } + s += p.Name() + } + 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 @@ -224,16 +239,18 @@ var indir = NewTypeName(token.NoPos, nil, "*", nil) // reports an error if it is not. // TODO(gri) rename s/typeCycle/cycle/ once we don't need the other // cycle method anymore. -func (check *Checker) typeCycle(obj Object) bool { +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() } - // We distinguish between cycles involving only constants and variables - // (nval = len(cycle)), cycles involving types (and functions) only - // (nval == 0), and mixed cycles (nval != 0 && nval != len(cycle)). + // Given the number of constants and variables (nval) in the cycle + // and the cycle length (ncycle = number of named objects in the cycle), + // we distinguish between cycles involving only constants and variables + // (nval = ncycle), cycles involving types (and functions) only + // (nval == 0), and mixed cycles (nval != 0 && nval != ncycle). // We ignore functions at the moment (taking them into account correctly // is complicated and it doesn't improve error reporting significantly). // @@ -242,17 +259,19 @@ func (check *Checker) typeCycle(obj Object) bool { // cannot be computed (it's either infinite or 0); if there is no type // definition, we have a sequence of alias type names which will expand // ad infinitum. - var nval int + var nval, ncycle int var hasIndir, hasTDef bool assert(obj.color() >= grey) start := obj.color() - grey // index of obj in objPath cycle := check.objPath[start:] + ncycle = len(cycle) // including indirections for _, obj := range cycle { switch obj := obj.(type) { case *Const, *Var: nval++ case *TypeName: if obj == indir { + ncycle-- // don't count (indirections are not objects) hasIndir = true } else if !check.objMap[obj].alias { hasTDef = true @@ -264,10 +283,20 @@ func (check *Checker) typeCycle(obj Object) bool { } } + if trace { + check.trace(obj.Pos(), "## cycle detected: objPath = %s->%s (len = %d)", objPathString(cycle), obj.Name(), ncycle) + check.trace(obj.Pos(), "## cycle contains: %d values, has indirection = %v, has type definition = %v", nval, hasIndir, hasTDef) + defer func() { + if isCycle { + check.trace(obj.Pos(), "=> error: cycle is invalid") + } + }() + } + // A cycle involving only constants and variables is invalid but we // ignore them here because they are reported via the initialization // cycle check. - if nval == len(cycle) { + if nval == ncycle { return false } @@ -305,7 +334,7 @@ func (check *Checker) constDecl(obj *Const, typ, init ast.Expr) { // determine type, if any if typ != nil { - t := check.typ(typ) + t := check.typExpr(typ, nil, nil) if !isConstType(t) { // don't report an error if the type is an invalid C (defined) type // (issue #22090) @@ -331,7 +360,7 @@ func (check *Checker) varDecl(obj *Var, lhs []*Var, typ, init ast.Expr) { // determine type, if any if typ != nil { - obj.typ = check.typ(typ) + obj.typ = check.typExpr(typ, nil, nil) // We cannot spread the type to all lhs variables if there // are more than one since that would mark them as checked // (see Checker.objDecl) and the assignment of init exprs, diff --git a/src/go/types/expr.go b/src/go/types/expr.go index 3f3c4f83c64..39ee6bcca35 100644 --- a/src/go/types/expr.go +++ b/src/go/types/expr.go @@ -1431,7 +1431,7 @@ func (check *Checker) exprInternal(x *operand, e ast.Expr, hint Type) exprKind { check.invalidAST(e.Pos(), "use of .(type) outside type switch") goto Error } - T := check.typ(e.Type) + T := check.typExpr(e.Type, nil, nil) if T == Typ[Invalid] { goto Error } diff --git a/src/go/types/testdata/issues.src b/src/go/types/testdata/issues.src index d727c3b3e23..9750bdc2e22 100644 --- a/src/go/types/testdata/issues.src +++ b/src/go/types/testdata/issues.src @@ -97,7 +97,7 @@ func issue10979() { // issue11347 // These should not crash. -var a1, b1 /* ERROR cycle */ /* ERROR cycle */ , c1 /* ERROR cycle */ b1 = 0 > 0<<""[""[c1]]>c1 +var a1, b1 /* ERROR cycle */ , c1 /* ERROR cycle */ b1 = 0 > 0<<""[""[c1]]>c1 var a2, b2 /* ERROR cycle */ = 0 /* ERROR cannot initialize */ /* ERROR cannot initialize */ > 0<<""[b2] var a3, b3 /* ERROR cycle */ = int /* ERROR cannot initialize */ /* ERROR cannot initialize */ (1<<""[b3])