Fix linter hanging in certain situations.

We had some accidental aliasing due to shallow copy instead of deep copy.

Fixes google#541.

Author: sbarzowski

linter/internal/types/build_graph.go

@@ -192,6 +192,9 @@ func calcTP(node ast.Node, varAt map[ast.Node]*common.Variable, g *typeGraph) ty
 	case *ast.Error:
 		return concreteTP(voidTypeDesc())
 	case *ast.Index:
+		// TODO(sbarzowski) It appears we create a separate index node each time.
+		// Perhaps we could de-duplicate (cache) index placeholders to save processing
+		// later?
 		switch index := node.Index.(type) {
 		case *ast.LiteralString:
 			return tpIndex(knownObjectIndex(g.getExprPlaceholder(node.Target), index.Value))

linter/internal/types/check.go

@@ -140,6 +140,8 @@ func Check(mainNode ast.Node, roots map[string]ast.Node, vars map[string]map[ast
 	g.prepareTypes(mainNode, et)
 
 	// TODO(sbarzowski) Useful for debugging – expose it in CLI?
+	// spew.Dump(g.timeStats)
+	// spew.Dump(g.counters)
 	// t := et[node.node]
 	// fmt.Fprintf(os.Stderr, "%v\n", types.Describe(&t))
 

linter/internal/types/desc.go

@@ -20,6 +20,17 @@ type arrayDesc struct {
 	elementContains [][]placeholderID
 }
 
+func (a *arrayDesc) copy() *arrayDesc {
+	elementContains := make([][]placeholderID, len(a.elementContains))
+	for i, placeholders := range a.elementContains {
+		elementContains[i] = append([]placeholderID{}, placeholders...)
+	}
+	return &arrayDesc{
+		furtherContain:  append([]placeholderID{}, a.furtherContain...),
+		elementContains: elementContains,
+	}
+}
+
 func (a *arrayDesc) widen(other *arrayDesc) {
 	if other == nil {
 		return
@@ -49,6 +60,18 @@ type objectDesc struct {
 	allFieldsKnown bool
 }
 
+func (o *objectDesc) copy() *objectDesc {
+	fieldContains := make(map[string][]placeholderID)
+	for k, v := range o.fieldContains {
+		fieldContains[k] = append([]placeholderID(nil), v...)
+	}
+	return &objectDesc{
+		unknownContain: append([]placeholderID(nil), o.unknownContain...),
+		fieldContains:  fieldContains,
+		allFieldsKnown: o.allFieldsKnown,
+	}
+}
+
 func (o *objectDesc) widen(other *objectDesc) {
 	if other == nil {
 		return
@@ -77,6 +100,7 @@ func (o *objectDesc) normalize() {
 type functionDesc struct {
 	resultContains []placeholderID
 
+	// Read-only
 	// TODO(sbarzowski) instead of keeping "real" parameters here,
 	// maybe keep only what we care about in the linter desc
 	// (names and required-or-not).
@@ -85,6 +109,15 @@ type functionDesc struct {
 	minArity, maxArity int
 }
 
+func (f *functionDesc) copy() *functionDesc {
+	return &functionDesc{
+		resultContains: append([]placeholderID{}, f.resultContains...),
+		params:         f.params, // this is read-only, so we can just copy the pointer
+		minArity:       f.minArity,
+		maxArity:       f.maxArity,
+	}
+}
+
 func sameParameters(a, b []ast.Parameter) bool {
 	if a == nil || b == nil {
 		return false
@@ -255,7 +288,44 @@ func Describe(t *TypeDesc) string {
 	return strings.Join(parts, " or ")
 }
 
+// An intuitive notion of the size of type description. For performance tuning.
+func (t *TypeDesc) size() int64 {
+	res := 0
+	if t.Bool {
+		res += 1
+	}
+	if t.Number {
+		res += 1
+	}
+	if t.String {
+		res += 1
+	}
+	if t.Null {
+		res += 1
+	}
+	if t.Function() {
+		res += len(t.FunctionDesc.resultContains)
+		res += len(t.FunctionDesc.params)
+	}
+	if t.Array() {
+		res += len(t.ArrayDesc.furtherContain)
+		for _, elem := range t.ArrayDesc.elementContains {
+			res += len(elem)
+		}
+	}
+	if t.Object() {
+		res += len(t.ObjectDesc.unknownContain)
+		for _, elem := range t.ObjectDesc.fieldContains {
+			res += len(elem)
+		}
+	}
+	return int64(res)
+}
+
 func (t *TypeDesc) widen(b *TypeDesc) {
+	if t == b {
+		panic("BUG: widening the type with itself")
+	}
 	t.Bool = t.Bool || b.Bool
 	t.Number = t.Number || b.Number
 	t.String = t.String || b.String
@@ -264,22 +334,24 @@ func (t *TypeDesc) widen(b *TypeDesc) {
 	if t.FunctionDesc != nil {
 		t.FunctionDesc.widen(b.FunctionDesc)
 	} else if t.FunctionDesc == nil && b.FunctionDesc != nil {
-		copy := *b.FunctionDesc
-		t.FunctionDesc = &copy
+		// copy := *b.FunctionDesc
+		// t.FunctionDesc = &copy
+		t.FunctionDesc = b.FunctionDesc.copy()
 	}
 
 	if t.ObjectDesc != nil {
 		t.ObjectDesc.widen(b.ObjectDesc)
 	} else if t.ObjectDesc == nil && b.ObjectDesc != nil {
-		copy := *b.ObjectDesc
-		t.ObjectDesc = &copy
+		// TODO(sbarzowski) Maybe we want copy on write?
+		// So that it actually aliases underneath (with a bool marker)
+		// which is resolved when widened.
+		t.ObjectDesc = b.ObjectDesc.copy()
 	}
 
 	if t.ArrayDesc != nil {
 		t.ArrayDesc.widen(b.ArrayDesc)
 	} else if t.ArrayDesc == nil && b.ArrayDesc != nil {
-		copy := *b.ArrayDesc
-		t.ArrayDesc = &copy
+		t.ArrayDesc = b.ArrayDesc.copy()
 	}
 }
 

linter/internal/types/graph.go

@@ -1,6 +1,10 @@
 package types
 
 import (
+	"fmt"
+	"io"
+	"time"
+
 	"github.com/google/go-jsonnet/ast"
 )
 
@@ -20,6 +24,32 @@ type typeGraph struct {
 
 	// TODO(sbarzowski) what was this for?
 	importFunc ImportFunc
+
+	// For performance tuning
+	timeStats timeStats
+
+	counters counters
+}
+
+// Subject to change at any point. For fine-tuning only.
+type timeStats struct {
+	simplifyRef       time.Duration
+	separateElemTypes time.Duration
+	topoOrder         time.Duration
+	findTypes         time.Duration
+}
+
+type counters struct {
+	sccCount                   int
+	containWidenCount          int
+	builtinWidenConcreteCount  int
+	builtinWidenContainedCount int
+	preNormalizationSumSize    int64
+	postNormalizationSumSize   int64
+}
+
+func (g *typeGraph) debugStats(w io.Writer) {
+	fmt.Fprintf(w, "placeholders no: %d\n", len(g._placeholders))
 }
 
 func (g *typeGraph) placeholder(id placeholderID) *typePlaceholder {
@@ -150,10 +180,21 @@ func newTypeGraph(importFunc ImportFunc) *typeGraph {
 // prepareTypes produces a final type for each expression in the graph.
 // No further operations on the graph are valid after this is called.
 func (g *typeGraph) prepareTypes(node ast.Node, typeOf exprTypes) {
+	tStart := time.Now()
 	g.simplifyReferences()
+	tSimplify := time.Now()
 	g.separateElementTypes()
+	tSeparate := time.Now()
 	g.makeTopoOrder()
+	tTopo := time.Now()
 	g.findTypes()
+	tTypes := time.Now()
+	g.timeStats = timeStats{
+		simplifyRef:       tSimplify.Sub(tStart),
+		separateElemTypes: tSeparate.Sub(tSimplify),
+		topoOrder:         tTopo.Sub(tSeparate),
+		findTypes:         tTypes.Sub(tTopo),
+	}
 	for e, p := range g.exprPlaceholder {
 		typeOf[e] = g.upperBound[p]
 	}

linter/internal/types/process_graph.go

@@ -1,6 +1,8 @@
 package types
 
-import "fmt"
+import (
+	"fmt"
+)
 
 type stronglyConnectedComponentID int
 
@@ -288,6 +290,7 @@ func (g *typeGraph) findTypes() {
 			sccID++
 		}
 	}
+	g.counters.sccCount = len(stronglyConnectedComponents)
 
 	for i := len(stronglyConnectedComponents) - 1; i >= 0; i-- {
 		scc := stronglyConnectedComponents[i]
@@ -301,11 +304,18 @@ func (g *typeGraph) resolveTypesInSCC(scc []placeholderID) {
 	common := voidTypeDesc()
 
 	for _, p := range scc {
+		// Add types from contained SCCs. These are fully calculated
+		// because we are going in topo order
 		for _, contained := range g.placeholder(p).contains {
+			// TODO(sbarzowski) Is it possible that we are adding multiple times from the same placeholder?
+			// Each `p` is normalized, but different placeholders in scc may still contain the same thing.
 			if g.sccOf[contained] != sccID {
 				common.widen(&g.upperBound[contained])
+				g.counters.containWidenCount += 1
 			}
 		}
+
+		// Handle builtins (e.g)
 		builtinOp := g.placeholder(p).builtinOp
 		if builtinOp != nil {
 			concreteArgs := []*TypeDesc{}
@@ -318,9 +328,11 @@ func (g *typeGraph) resolveTypesInSCC(scc []placeholderID) {
 			}
 			res := builtinOp.f(concreteArgs, builtinOp.args)
 			common.widen(&res.concrete)
+			g.counters.builtinWidenConcreteCount += 1
 			for _, contained := range res.contained {
 				if g.sccOf[contained] != sccID {
 					common.widen(&g.upperBound[contained])
+					g.counters.builtinWidenContainedCount += 1
 				}
 			}
 		}
@@ -333,8 +345,12 @@ func (g *typeGraph) resolveTypesInSCC(scc []placeholderID) {
 		}
 	}
 
+	g.counters.preNormalizationSumSize += common.size()
+
 	common.normalize()
 
+	g.counters.postNormalizationSumSize += common.size()
+
 	for _, p := range scc {
 		g.upperBound[p] = common
 	}

linter/internal/types/stdlib.go

@@ -9,7 +9,7 @@ func prepareStdlib(g *typeGraph) {
 	arrayOfNumber := anyArrayType
 	stringOrArray := anyType
 	stringOrNumber := anyType
-	jsonType := anyType // It actually cannot functions anywhere
+	jsonType := anyType // It actually cannot have functions anywhere
 
 	required := func(name string) ast.Parameter {
 		return ast.Parameter{Name: ast.Identifier(name)}