core/toposort: Stop using RawString

Use of RawString in debug output is unsafe because the Feature could be
an IntLabel for a list which exceeds the string index. So for debugging,
switch to using SelectorString.

In general, the graph can contain nodes which have an IntLabel Feature,
and we must be able to sort those correctly. So adjust the comparison
function in order to detect the type of the Feature and make sensible
choices for comparison.

Fixes #3698.

Signed-off-by: Matthew Sackman <[email protected]>
Change-Id: I9eb692d6c722ea9c098d7ca380fd8a71c8cb040c
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207715
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>

Author: cuematthew

internal/core/toposort/graph.go

@@ -49,10 +49,13 @@ func (n *Node) IsSorted() bool {
 	return n.position >= 0
 }
 
-func (n *Node) Name(index adt.StringIndexer) string {
-	// TODO: two different fields like "#foo" and #foo, can have the same raw
-	// string
-	return n.Feature.RawString(index)
+// SafeName returns a string useful for debugging, regardless of the
+// type of the feature. So for IntLabels, you'll get back `1`, `10`
+// etc; for identifiers, you may get back a string with quotes in it,
+// eg `"runs-on"`. So this is not useful for comparisons, but it is
+// useful (and safe) for debugging.
+func (n *Node) SafeName(index adt.StringIndexer) string {
+	return n.Feature.SelectorString(index)
 }
 
 type Nodes []*Node
@@ -136,7 +139,19 @@ func (builder *GraphBuilder) Build() *Graph {
 type indexComparison struct{ adt.StringIndexer }
 
 func (index *indexComparison) compareNodeByName(a, b *Node) int {
-	return cmp.Compare(a.Name(index), b.Name(index))
+	aFeature, bFeature := a.Feature, b.Feature
+	aIsInt, bIsInt := aFeature.Typ() == adt.IntLabel, bFeature.Typ() == adt.IntLabel
+
+	switch {
+	case aIsInt && bIsInt:
+		return cmp.Compare(aFeature.Index(), bFeature.Index())
+	case aIsInt:
+		return -1
+	case bIsInt:
+		return 1
+	default:
+		return cmp.Compare(aFeature.RawString(index), bFeature.RawString(index))
+	}
 }
 
 func (index *indexComparison) compareCyclesByNames(a, b *Cycle) int {
@@ -202,7 +217,7 @@ func chooseCycle(indexCmp *indexComparison, unusedCycles []*Cycle) *Cycle {
 
 		debug("cycle %v; edgeCount %v; enabledSince %v; entryNode %v\n",
 			cycle, brokenEdgeCount, enabledSince,
-			entryNode.Name(indexCmp))
+			entryNode.SafeName(indexCmp))
 
 		cycleIsBetter := chosenCycleIdx == -1
 		// this is written out long-form for ease of readability
@@ -219,7 +234,7 @@ func chooseCycle(indexCmp *indexComparison, unusedCycles []*Cycle) *Cycle {
 		case enabledSince > chosenCycleEnabledSince:
 			// noop - only continue if ==
 
-		case entryNode.Name(indexCmp) < chosenCycleEntryNode.Name(indexCmp):
+		case indexCmp.compareNodeByName(entryNode, chosenCycleEntryNode) < 0:
 			cycleIsBetter = true
 		case entryNode == chosenCycleEntryNode:
 			cycleIsBetter =
@@ -239,7 +254,7 @@ func chooseCycle(indexCmp *indexComparison, unusedCycles []*Cycle) *Cycle {
 	}
 
 	debug("Chose cycle: %v; entering at node: %s\n",
-		unusedCycles[chosenCycleIdx], chosenCycleEntryNode.Name(indexCmp))
+		unusedCycles[chosenCycleIdx], chosenCycleEntryNode.SafeName(indexCmp))
 	cycle := unusedCycles[chosenCycleIdx]
 	unusedCycles[chosenCycleIdx] = nil
 	cycle.RotateToStartAt(chosenCycleEntryNode)
@@ -361,7 +376,7 @@ func appendNodes(indexCmp *indexComparison, nodesSorted, nodesReady, nodesEnable
 				}
 			}
 			debug("After %v, found new ready: %s\n",
-				nodesSorted, next.Name(indexCmp))
+				nodesSorted, next.SafeName(indexCmp))
 			nodesEnabled = append(nodesEnabled, next)
 			nodesReadyNeedsSorting = true
 		}

internal/core/toposort/sort_test.go

@@ -18,11 +18,16 @@ import (
 	"fmt"
 	"testing"
 
+	"cuelang.org/go/cue"
+	"cuelang.org/go/cue/cuecontext"
 	"cuelang.org/go/cue/format"
+	"cuelang.org/go/internal/core/adt"
 	"cuelang.org/go/internal/core/eval"
 	"cuelang.org/go/internal/core/export"
+	"cuelang.org/go/internal/core/toposort"
 	"cuelang.org/go/internal/cuetdtest"
 	"cuelang.org/go/internal/cuetxtar"
+	"cuelang.org/go/internal/value"
 )
 
 func TestTopologicalSort(t *testing.T) {
@@ -73,3 +78,18 @@ func TestTopologicalSort(t *testing.T) {
 		})
 	}
 }
+
+func TestIssue3698(t *testing.T) {
+	str := `import "list"
+list.Repeat([{x: 5}], 1000)
+`
+	ctx := cuecontext.New()
+	val := ctx.CompileString(str, cue.Filename("issue3698"))
+	if err := val.Err(); err != nil {
+		t.Fatal(err)
+	}
+	run, v := value.ToInternal(val)
+	// If toposort does not correctly handle feature names,
+	// specifically IntLabels, then this will panic:
+	toposort.VertexFeatures(adt.NewContext(run, v), v)
+}

internal/core/toposort/vertex.go

@@ -393,7 +393,7 @@ func (vf *vertexFeatures) compareStructMeta(a, b *structMeta) int {
 }
 
 func VertexFeatures(ctx *adt.OpContext, v *adt.Vertex) []adt.Feature {
-	debug("\n*** V (%s %v %p) ***\n", v.Label.RawString(ctx), v.Label, v)
+	debug("\n*** V (%s %v %p) ***\n", v.Label.SelectorString(ctx), v.Label, v)
 
 	builder := NewGraphBuilder(!ctx.Config.SortFields)
 	dynFieldsMap := dynamicFieldsFeatures(v)

tools/flow/testdata/slice.txtar

@@ -24,21 +24,21 @@ root: [
 -- out/run/errors --
 -- out/run/t0 --
 graph TD
-  t0("root[2] [Waiting]")
-  t0-->t2
-  t0-->t1
+  t0("root[0] [Ready]")
   t1("root[1] [Waiting]")
-  t1-->t2
-  t2("root[0] [Ready]")
+  t1-->t0
+  t2("root[2] [Waiting]")
+  t2-->t0
+  t2-->t1
 
 -- out/run/t1 --
 graph TD
-  t0("root[2] [Waiting]")
-  t0-->t2
-  t0-->t1
+  t0("root[0] [Terminated]")
   t1("root[1] [Ready]")
-  t1-->t2
-  t2("root[0] [Terminated]")
+  t1-->t0
+  t2("root[2] [Waiting]")
+  t2-->t0
+  t2-->t1
 
 -- out/run/t1/value --
 {
@@ -58,12 +58,12 @@ Conjuncts:    30
 Disjuncts:    17
 -- out/run/t2 --
 graph TD
-  t0("root[2] [Ready]")
-  t0-->t2
-  t0-->t1
+  t0("root[0] [Terminated]")
   t1("root[1] [Terminated]")
-  t1-->t2
-  t2("root[0] [Terminated]")
+  t1-->t0
+  t2("root[2] [Ready]")
+  t2-->t0
+  t2-->t1
 
 -- out/run/t2/value --
 {
@@ -88,12 +88,12 @@ Conjuncts:    33
 Disjuncts:    17
 -- out/run/t3 --
 graph TD
-  t0("root[2] [Terminated]")
-  t0-->t2
-  t0-->t1
+  t0("root[0] [Terminated]")
   t1("root[1] [Terminated]")
-  t1-->t2
-  t2("root[0] [Terminated]")
+  t1-->t0
+  t2("root[2] [Terminated]")
+  t2-->t0
+  t2-->t1
 
 -- out/run/t3/value --
 {