Skip to content

Commit

Permalink
interp: improve processing of recursive types
Browse files Browse the repository at this point in the history 
Make sure to keep always a single copy of incomplete type structures.
Remove remnants of recursive types processing.

Now `import "go.uber.org/zap"` works again (see #1172), fixing regressions
introduced by #1236.
  • Loading branch information
@mvertes
mvertes committed Sep 8, 2021
1 parent 4653d87 commit 81e5a47
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 125 deletions.
16 changes: 1 addition & 15 deletions interp/cfg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ func (interp *Interpreter) cfg(root *node, importPath, pkgName string) ([]*node,
dest.gen = nop
case isFuncField(dest):
// Setting a struct field of function type requires an extra step. Do not optimize.
case isCall(src) && !isInterfaceSrc(dest.typ) && !isRecursiveField(dest) && n.kind != defineStmt:
case isCall(src) && !isInterfaceSrc(dest.typ) && n.kind != defineStmt:
// Call action may perform the assignment directly.
if dest.typ.id() != src.typ.id() {
// Skip optimitization if returned type doesn't match assigned one.
Expand Down Expand Up @@ -2403,20 +2403,6 @@ func isField(n *node) bool {
return n.kind == selectorExpr && len(n.child) > 0 && n.child[0].typ != nil && isStruct(n.child[0].typ)
}

func isRecursiveField(n *node) bool {
if !isField(n) {
return false
}
t := n.typ
for t != nil {
if t.recursive {
return true
}
t = t.val
}
return false
}

func isInConstOrTypeDecl(n *node) bool {
anc := n.anc
for anc != nil {
Expand Down
10 changes: 6 additions & 4 deletions interp/gta.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (interp *Interpreter) gta(root *node, rpath, importPath, pkgName string) ([
}
typ := atyp
if typ == nil {
if typ, err = nodeType(interp, sc, src); err != nil {
if typ, err = nodeType(interp, sc, src); err != nil || typ == nil {
return false
}
val = src.rval
Expand Down Expand Up @@ -150,7 +150,7 @@ func (interp *Interpreter) gta(root *node, rpath, importPath, pkgName string) ([
}
rcvrtype = ptrOf(elementType, withNode(rtn), withScope(sc))
rcvrtype.incomplete = elementType.incomplete
elementType.method = append(elementType.method, n)
elementType.addMethod(n)
} else {
rcvrtype = sc.getType(typeName)
if rcvrtype == nil {
Expand All @@ -159,7 +159,7 @@ func (interp *Interpreter) gta(root *node, rpath, importPath, pkgName string) ([
rcvrtype = sc.sym[typeName].typ
}
}
rcvrtype.method = append(rcvrtype.method, n)
rcvrtype.addMethod(n)
n.child[0].child[0].lastChild().typ = rcvrtype
case ident == "init":
// init functions do not get declared as per the Go spec.
Expand Down Expand Up @@ -288,7 +288,9 @@ func (interp *Interpreter) gta(root *node, rpath, importPath, pkgName string) ([
} else {
if sym.typ != nil && (len(sym.typ.method) > 0) {
// Type has already been seen as a receiver in a method function
n.typ.method = append(n.typ.method, sym.typ.method...)
for _, m := range sym.typ.method {
n.typ.addMethod(m)
}
} else {
// TODO(mpl): figure out how to detect redeclarations without breaking type aliases.
// Allow redeclarations for now.
Expand Down
Loading

4 comments on commit 81e5a47

@jptosso
Copy link

@jptosso jptosso commented on 81e5a47 Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes uber/zap but we are back to the previous issue #1227

/Users/jptosso/go/src/github.com/jptosso/coraza-waf/vendor/github.com/antchfx/xpath/func.go:23:19: panic
panic: reflect.Set: value of type func(interface {}, string, interface {}, interface {}) bool is not assignable to type *interp.node [recovered]
	panic: reflect.Set: value of type func(interface {}, string, interface {}, interface {}) bool is not assignable to type *interp.node

goroutine 1 [running]:
github.com/traefik/yaegi/interp.runCfg.func1(0xc0003e4000, 0xc001aac5a0, 0x0)
	/Users/jptosso/go/pkg/mod/github.com/traefik/[email protected]/interp/run.go:193 +0x251
panic(0x1a21020, 0xc001337bf0)

var builderPool = sync.Pool{New: func() interface{} {
        return newStringBuilder()
}}

@mvertes
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jptosso there is no visible regression in the test corresponding to issue #1227: https://github.com/traefik/yaegi/blob/master/_test/assert2.go.

Please provide a reproducible test case, with no external dependency if possible, so we can investigate and fix, thanks.

@nrwiersma
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jptosso This is an entirely different issue to #1227 coming from https://github.com/antchfx/xpath/blob/cd2afb821d43992abcefe1c70b1b41af5b640179/operator.go#L41.

@nrwiersma
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jptosso @mvertes I have opened an issue for this #1249. I will take a look at this, hopefully this weekend.

Please sign in to comment.