internal/core/dep: distinghish inline structs

With an upcoming change, inline structs can be shared
and reappropriated as regular structs in some cases.
This means that the current algorithm, which assumes
that inline structs are always local to the current
field, is no longer correct.

We modify the algorithm to prepare for this change.
One aspect of this is isLocal, which checks whether
the field references resolve to within the current
scope of dependency analysis (by comparing to empty).

Another change is that we now need to be more
precise when it comes to checking whether a Vertex
is rooted or not. We use IsDetached and MayAttach
instead of Rooted for V3 in some cases.

Finally, we are now more aggressive with taking
the top reference, instead of an interstitial
reference, as the representative reference.
This results in better output even for V2

Changes:
issue2247.txtar: appropriate simplication
import.txtar: fixes an issue in v2
let2.txtar: the v3 output seems more correct, as
the original reference is more important than
the interstitial one.

Issue #2854

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

internal/core/dep/dep.go

@@ -17,6 +17,7 @@ package dep
 
 import (
 	"cuelang.org/go/cue/errors"
+	"cuelang.org/go/internal"
 	"cuelang.org/go/internal/core/adt"
 )
 
@@ -413,15 +414,37 @@ func (c *visitor) reportDependency(env *adt.Environment, ref adt.Resolver, v *ad
 		reference = c.topRef
 	}
 
-	if !v.Rooted() {
+	inspect := false
+
+	if c.ctxt.Version == internal.DevVersion {
+		inspect = v.IsDetached() || !v.MayAttach()
+	} else {
+		inspect = !v.Rooted()
+	}
+
+	if inspect {
+		// TODO: there is currently no way to inspect where a non-rooted node
+		// originated from. As of EvalV3, we allow non-rooted nodes to be
+		// structure shared. This makes them effectively rooted, with the
+		// difference that there is an indirection in BaseValue for the
+		// structure sharing. Nonetheless, this information is lost in the
+		// internal API when traversing.
+
+		// As an alternative we now do not skip processing the node if we
+		// an inlined, non-rooted node is associated with another node than
+		// the one we are currently processing.
+
+		// If a node is internal, we need to further investigate any references.
+		// If there are any, reference, even if it is otherwise not reported,
+		// we report this reference.
 		before := c.numRefs
 		c.markInternalResolvers(env, ref, v)
 		// TODO: this logic could probably be simplified if we let clients
 		// explicitly mark whether to visit rootless nodes. Visiting these
 		// may be necessary when substituting values.
 		switch _, ok := ref.(*adt.FieldReference); {
-		case !ok:
-			// Do not report rootless nodes for selectors.
+		case !ok && c.isLocal(env, ref):
+			// 	Do not report rootless nodes for selectors.
 			return
 		case c.numRefs > before:
 			// For FieldReferences that resolve to something we do not need
@@ -453,6 +476,9 @@ func (c *visitor) reportDependency(env *adt.Environment, ref adt.Resolver, v *ad
 		}
 		v = w
 	}
+	if len(c.pathStack) == 0 && c.topRef != nil {
+		altRef = c.topRef
+	}
 
 	// All resolvers are expressions.
 	if p := importRef(ref.(adt.Expr)); p != nil {
@@ -476,6 +502,26 @@ func (c *visitor) reportDependency(env *adt.Environment, ref adt.Resolver, v *ad
 	}
 }
 
+// isLocal reports whether a non-rooted struct is an internal node or not.
+// If it is not, we need to further investigate any references.
+func (c *visitor) isLocal(env *adt.Environment, r adt.Resolver) bool {
+	for {
+		switch x := r.(type) {
+		case *adt.FieldReference:
+			for i := 0; i < int(x.UpCount); i++ {
+				env = env.Up
+			}
+			return env.Vertex == empty
+		case *adt.SelectorExpr:
+			r, _ = x.X.(adt.Resolver)
+		case *adt.IndexExpr:
+			r, _ = x.X.(adt.Resolver)
+		default:
+			return env.Vertex == empty
+		}
+	}
+}
+
 // TODO(perf): make this available as a property of vertices to avoid doing
 // work repeatedly.
 func hasLetParent(v *adt.Vertex) bool {

internal/core/dep/dep_test.go

@@ -116,7 +116,7 @@ func testVisit(t *testing.T, w io.Writer, ctxt *adt.OpContext, v *adt.Vertex, cf
 		if i := d.Import(); i != nil {
 			path := i.ImportPath.StringValue(ctxt)
 			str = fmt.Sprintf("%q.%s", path, str)
-		} else if !d.Node.Rooted() {
+		} else if d.Node.IsDetached() {
 			str = "**non-rooted**"
 		}
 
@@ -129,7 +129,7 @@ func testVisit(t *testing.T, w io.Writer, ctxt *adt.OpContext, v *adt.Vertex, cf
 // DO NOT REMOVE: for Testing purposes.
 func TestX(t *testing.T) {
 	version := internal.DefaultVersion
-	version = internal.DevVersion
+	// version = internal.DevVersion // Uncomment for eval V3
 	flags := cuedebug.Config{
 		Sharing: true,
 		LogEval: 1,

internal/core/dep/testdata/let2.txtar

@@ -23,6 +23,25 @@ a: b: {
 }
 -- out/dependencies/field --
 line  reference    path of resulting vertex
+-- out/dependencies-v3/all --
+line  reference    path of resulting vertex
+12:   X1        => v.w
+13:   X2        => v.w
+14:   X3.w      => v.w
+20:   Y1        => v
+21:   Y2        => v
+-- diff/-out/dependencies-v3/all<==>+out/dependencies/all --
+diff old new
+--- old
++++ new
+@@ -2,5 +2,5 @@
+ 12:   X1        => v.w
+ 13:   X2        => v.w
+ 14:   X3.w      => v.w
+-7:    v         => v
+-8:    v         => v
++20:   Y1        => v
++21:   Y2        => v
 -- out/dependencies/all --
 line  reference    path of resulting vertex
 12:   X1        => v.w

internal/core/export/testdata/selfcontained/import.txtar

@@ -160,15 +160,6 @@ diff old new
  //cue:path: "mod.test/a/pkg".a.b
  let B = {
  	c: {
-@@ -42,7 +45,7 @@
- let W = {
- 	a: _hidden_567475F3
- 	_hidden_567475F3: {
--		a: 1
-+		a: b
- 	}
- 	b: 1
- 	x: {
 -- diff/self/todo/p2 --
 Investigate differences.
 We assign p2, because the differences only appear with sharing off.
@@ -217,7 +208,7 @@ let F = {
 let W = {
 	a: _hidden_567475F3
 	_hidden_567475F3: {
-		a: 1
+		a: b
 	}
 	b: 1
 	x: {

internal/core/export/testdata/selfcontained/issue2247.txtar

@@ -46,12 +46,8 @@ let P = {
 	e: {
 		out: c
 	}.out
-	f: {
-		out: Q
-	}.out
-	g: {
-		out: Q
-	}.out
+	f: Q
+	g: Q
 	h: {
 		out: {
 			r: {
@@ -98,12 +94,8 @@ f: {
 	e: {
 		out: c
 	}.out
-	f: {
-		out: Q
-	}.out
-	g: {
-		out: Q
-	}.out
+	f: Q
+	g: Q
 	h: {
 		out: {
 			r: {