lsp/eval: Path resolution must force eval of import specs

The LSP evaluator processes import specs super lazily so that linking
the import spec to the remote pkg evaluator is done as late as possible.
Without this, booting an evaluator would also boot every evaluator for
every imported package. And the transitive closure. So everything in the
import graph would get booted, which we don't want.

So the remote pkg evaluator only gets booted when we really pass into
it. E.g.

-- a.cue --
package a
import "b"
x: b.y
-- b.cue --
package b
y: 12

Only when we resolve the `y` of the path `b.y` do we boot pkg `b`.
But this causes a problem because it means that if you use the entire
package (e.g. changing `x: b.y` to just `x: b`) then pkg b never gets
booted and so we would also never record that pkg b is used by pkg a.

This CL fixes this by detecting when a path component resolves to an
ImportSpec and forcing the evaluation at that time. This still should
ensure that we don't fully evaluate import specs too early, but it does
ensure that the "usedBy" of packages is now correct.

The old behaviour was flakey, and for that reason it's not possible to
write a test that shows the bad behaviour. But manually backporting the
new test and running with -count 10 will show Usages being flakey.

Signed-off-by: Matthew Sackman <[e-mail]>
Change-Id: I5d02df1a5ccb17a9fd38deb22d0581c9f581e8c6
Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1228808
TryBot-Result: CUEcueckoo <[e-mail]>
Unity-Result: CUE porcuepine <[e-mail]>
Reviewed-by: Daniel Martí <[e-mail]>

internal/lsp/eval/eval.go

···
 		components[i+1].unexpanded = unexpanded
 		if pc.node != nil {
 			for _, nav := range unexpanded {
+				// We eval import specs in two stages (see [frame.eval])
+				// so that we delay booting remote pkg evals as long as
+				// possible. Because of this, if we resolve an ident to an
+				// import spec, we must eval it "early" otherwise use of
+				// the remote pkg may not be recorded correct. E.g.
+				//
+				//	package a
+				//	import "b"
+				//	c: b
+				//
+				// When resolving the path b on line 3, if we don't eval
+				// the import spec here, we would never record that pkg b
+				// is used by pkg a.
+				for _, fr := range nav.frames {
+					if _, ok := fr.node.(*ast.ImportSpec); ok {
+						nav.eval()
+						break
+					}
+				}
 				nav.recordUsage(pc.node, p.frame)
 			}
 		}

internal/lsp/eval/eval_test.go

···
 		},
 
 		{
+			name: "Resolve_Import_Embed",
+			archive: `-- a.cue --
+package a
+-- b.cue --
+package b
+
+import "a"
+
+y: z(a)
+-- c.cue --
+package a
+-- d.cue --
+package d
+
+import x "a"
+import "a"
+
+y: a / true
+`,
+			expectDefinitions: map[position][]position{
+				fln("b.cue", 3, 1, `"a"`): {fln("a.cue", 1, 3, "a"), fln("c.cue", 1, 3, "a")},
+				fln("b.cue", 5, 1, "a"):   {fln("b.cue", 3, 1, `"a"`)},
+
+				fln("d.cue", 3, 1, "x"):   {fln("a.cue", 1, 3, "a"), fln("c.cue", 1, 3, "a")},
+				fln("d.cue", 4, 1, `"a"`): {fln("a.cue", 1, 3, "a"), fln("c.cue", 1, 3, "a")},
+				fln("d.cue", 6, 1, "a"):   {fln("d.cue", 3, 1, "x"), fln("d.cue", 4, 1, `"a"`)},
+
+				fln("a.cue", 1, 3, "a"): {self, fln("c.cue", 1, 3, "a")},
+				fln("b.cue", 1, 1, "b"): {self},
+				fln("c.cue", 1, 3, "a"): {self, fln("a.cue", 1, 3, "a")},
+				fln("d.cue", 1, 1, "d"): {self},
+
+				fln("b.cue", 5, 1, "y"): {self},
+				fln("d.cue", 6, 1, "y"): {self},
+			},
+			expectCompletions: map[offsetRange]fieldEmbedCompletions{
+				orf("b.cue", 10, 18): {f: []string{"y"}},
+				orf("b.cue", 18, 22): {e: []string{"a", "y"}},
+				orf("b.cue", 22, 25): {f: []string{"y"}},
+				orf("b.cue", 25, 31): {e: []string{"a", "y"}},
+
+				orf("d.cue", 10, 18): {f: []string{"y"}},
+				orf("d.cue", 18, 24): {e: []string{"a", "x", "y"}},
+				orf("d.cue", 24, 31): {f: []string{"y"}},
+				orf("d.cue", 31, 35): {e: []string{"a", "x", "y"}},
+				orf("d.cue", 35, 38): {f: []string{"y"}},
+				orf("d.cue", 38, 43): {e: []string{"a", "x", "y"}},
+				orf("d.cue", 47, 48): {e: []string{"a", "x", "y"}},
+			},
+
+			expectUsagesExtra: map[position]map[bool][]position{
+				fln("b.cue", 3, 1, `"a"`): {true: []position{self}},
+				fln("d.cue", 3, 1, "x"):   {true: []position{self, fln("d.cue", 4, 1, `"a"`)}},
+				fln("d.cue", 4, 1, `"a"`): {true: []position{self, fln("d.cue", 3, 1, "x")}},
+			},
+			importedBy: map[string][]string{
+				"a": {"b", "d"},
+			},
+		},
+
+		{
 			name: "Resolve_Import_Repeated",
 			archive: `-- a.cue --
 package a
···
 			}
 
 			analyse := func() testCaseAnalysis {
-				evalByFilename := make(map[string]*eval.FileEvaluator)
+				evalByFilename := make(map[string]*eval.Evaluator)
 				evalByPkgName := make(map[string]*eval.Evaluator)
 				forPackage := func(importPath ast.ImportPath) *eval.Evaluator {
 					return evalByPkgName[importPath.String()]
 				}
 				importCanonicalisation := make(map[string]ast.ImportPath)
 				analysis := testCaseAnalysis{
-					evalByPkgName:  evalByPkgName,
 					evalByFilename: evalByFilename,
 				}
 
···
 					eval := eval.New(ip, importCanonicalisation, forPackage, pkgImporters, files...)
 					evalByPkgName[pkgName] = eval
 					for _, fileAst := range files {
-						evalByFilename[fileAst.Filename] = eval.ForFile(fileAst.Filename)
+						evalByFilename[fileAst.Filename] = eval
 					}
 				}
 				return analysis
···
 }
 
 type testCaseAnalysis struct {
-	evalByPkgName  map[string]*eval.Evaluator
-	evalByFilename map[string]*eval.FileEvaluator
+	evalByFilename map[string]*eval.Evaluator
 }
 
 func (tc *testCase) testDefinitions(t *testing.T, files []*ast.File, analysis testCaseAnalysis) {
···
 
 		for posFrom, positionsWant := range tc.expectDefinitions {
 			filename := posFrom.filename
-			fileEval := evalByFilename[filename]
+			fileEval := evalByFilename[filename].ForFile(filename)
 			qt.Check(t, qt.IsNotNil(fileEval))
 
 			offset := posFrom.offset
···
 		// expectations, resolve to nothing.
 		for _, fileAst := range files {
 			filename := fileAst.Filename
-			fileEval := evalByFilename[filename]
+			fileEval := evalByFilename[filename].ForFile(filename)
 			for i := range fileAst.Pos().File().Content() {
 				if ranges.Contains(filename, i) {
 					continue
···
 			if slices.Contains(posDfns, self) {
 				continue
 			} else if strings.HasSuffix(posUse.str, `"]`) {
-				// If posUse ends with "] then we assume it's a dynamic
-				// index into a struct. These can be inverted. E.g.
+				// If posUse ends with "] then we assume it's a const
+				// string dynamic index into a struct. These can be
+				// inverted. E.g.
 				//
 				//	{"g": 13}["g"]
 				//
···
 
 		declarations := make(map[position][]position)
 		for posUse, posDfns := range tc.expectDefinitions {
-			// Package-declarations are slightly special and so we have
-			// to make sure they are not treated as declarations for our
-			// purposes here.
-			if posUse.offset == 8 && strings.HasPrefix(posUse.content, "package ") {
-				continue
-			}
 			posDfns := slices.Clone(posDfns)
 			for i, posDfn := range posDfns {
 				if posDfn == self { // this must be a field declaration
···
 
 			for posUse, positionsWant := range expectUsages {
 				filename := posUse.filename
-				fe := analysis.evalByFilename[filename]
+				fe := analysis.evalByFilename[filename].ForFile(filename)
 				qt.Assert(t, qt.IsNotNil(fe))
 
 				fileOffsetsWant := make([]fileOffset, len(positionsWant))
···
 			slices.Sort(fieldCompletionWant)
 			slices.Sort(embedCompletionWant)
 			filename := curRange.filename
-			fe := evalByFilename[filename]
+			fe := evalByFilename[filename].ForFile(filename)
 			qt.Check(t, qt.IsNotNil(fe))
 
 			ranges.Add(filename, curRange.from, curRange.to)
···
 		// expectations, complete to nothing.
 		for _, fileAst := range files {
 			filename := fileAst.Filename
-			fe := evalByFilename[filename]
+			fe := evalByFilename[filename].ForFile(filename)
 
 			for i := range fileAst.Pos().File().Content() {
 				if ranges.Contains(filename, i) {
···
 	})
 }
 
-func (tc *testCase) dumpCompletions(t *testing.T, files []*ast.File, evalByFilename map[string]*eval.FileEvaluator) {
+func (tc *testCase) dumpCompletions(t *testing.T, files []*ast.File, evalByFilename map[string]*eval.Evaluator) {
 	for _, fileAst := range files {
 		filename := fileAst.Filename
-		fe := evalByFilename[filename]
+		fe := evalByFilename[filename].ForFile(filename)
 		content := fileAst.Pos().File().Content()
 
 		var strs []string