tools/fix: open field values inside comprehensions

Under the old semantics, conjuncts inserted through comprehensions are
treated like embeddings and do not close their fields, recursively.
Under explicitopen this opening is gone, so a comprehension conjunct
such as "if c { egress: #HC }" closes egress and rejects sibling entries
added elsewhere. This broke the greymatter tenant-examples unity
project, whose #Service inserts a health-check egress listener through a
comprehension.

Append a postfix ellipsis to field values inside comprehension bodies
when they may resolve to a closed struct: references, selectors, index
expressions, and conjunctions or disjunctions involving them. The
postfix ellipsis opens recursively, matching the old comprehension
behavior. Values that cannot carry closedness, such as literals, are
left alone, as are close() calls, which explicitly opt in to closing.

The new lexical comprehension counter subsumes the field-scoped
closeInfo.inComprehension, whose only use was the TODO comment on
embeddings; that comment now also applies to embeddings nested under
fields within a comprehension.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Ia1498de6ed63412cee0266edd6094d37aa4bff47
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1242590
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>

Author: mvdan

tools/fix/fix_test.go

@@ -176,8 +176,6 @@ v: __closeAll({
 			// comprehensions are treated like embeddings and do not close
 			// their fields. Field values that may resolve to closed
 			// structs must be opened to preserve that behavior.
-			// TODO: the egress value is left untouched; it should become
-			// "#HC..." so that sibling entries of egress remain allowed.
 			name: "open field values in comprehensions (fixExplicitOpen)",
 			exps: []string{"explicitopen"},
 			in: `package foo
@@ -192,15 +190,17 @@ v: __closeAll({
 	}
 }
 `,
-			out: `package foo
+			out: `@experiment(explicitopen)
+
+package foo
 
 #HC: hc: {port: 1}
 
 #Service: {
 	enable: bool
 	egress?: [string]: {...}
 	if enable {
-		egress: #HC
+		egress: #HC...
 	}
 }
 `,

tools/fix/fixopen.go

@@ -45,14 +45,17 @@ func (a embedFlags) or(b embedFlags) embedFlags {
 	}
 }
 
+// mayBeClosed reports whether the expression the flags were collected
+// from may resolve to a closed value.
+func (f embedFlags) mayBeClosed() bool {
+	return f.def || f.other || f.close
+}
+
 type closeInfo struct {
 	// Do not close enclosing structs if non-zero. This may be the case
 	// for comprehensions, nested structs, etc.
 	suspendReclose int
 
-	// inComprehension tracks whether we are inside a comprehension value.
-	inComprehension int
-
 	embedFlags
 }
 
@@ -64,6 +67,10 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 
 	var info closeInfo
 	recloseStack := []closeInfo{}
+	// comprehensionDepth tracks whether we are lexically inside a
+	// comprehension value. Unlike the closeInfo state, it is not reset
+	// at field or conjunction boundaries.
+	comprehensionDepth := 0
 	result = astutil.Apply(f, func(c astutil.Cursor) bool {
 		n := c.Node()
 		switch n := n.(type) {
@@ -82,7 +89,7 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 
 		case *ast.Comprehension:
 			info.suspendReclose++
-			info.inComprehension++
+			comprehensionDepth++
 
 		case *ast.EmbedDecl:
 			info.suspendReclose++
@@ -95,6 +102,15 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 			recloseStack = recloseStack[:len(recloseStack)-1]
 			c.ClearEnclosingModified()
 
+			// See openCompFieldValue: comprehension conjuncts did not
+			// close their fields under the old semantics.
+			if comprehensionDepth > 0 {
+				if newValue, changed := openCompFieldValue(n.Value); changed {
+					n.Value = newValue
+					hasChanges = true
+				}
+			}
+
 		case *ast.BinaryExpr:
 			if n.Op == token.AND || n.Op == token.OR {
 				info = recloseStack[len(recloseStack)-1]
@@ -104,7 +120,7 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 
 		case *ast.Comprehension:
 			info.suspendReclose--
-			info.inComprehension--
+			comprehensionDepth--
 
 		case *ast.EmbedDecl:
 			info.suspendReclose--
@@ -116,7 +132,7 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 			newExpr, exprChanged, flags := openEmbedExpr(n.Expr)
 			info.embedFlags = info.embedFlags.or(flags)
 			if exprChanged {
-				if info.inComprehension > 0 {
+				if comprehensionDepth > 0 {
 					ast.AddComment(newExpr, todoComment(
 						"... may not be intended inside a comprehension value; consider removing it."))
 				}
@@ -185,6 +201,22 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
 	return result, hasChanges
 }
 
+// openCompFieldValue adds a postfix ellipsis to a field value inside a
+// comprehension when the value may resolve to a closed struct. Under the
+// old semantics, conjuncts inserted through comprehensions were treated
+// like embeddings and did not close their fields.
+func openCompFieldValue(expr ast.Expr) (ast.Expr, bool) {
+	switch expr.(type) {
+	case *ast.SelectorExpr, *ast.IndexExpr:
+		return addEllipsis(expr), true
+	case *ast.Ident, *ast.BinaryExpr, *ast.ParenExpr:
+		if _, _, f := collectEmbedFlags(expr); f.mayBeClosed() {
+			return addEllipsis(expr), true
+		}
+	}
+	return expr, false
+}
+
 // collectEmbedFlags recurses into an expression to collect embedding flags
 // without modifying the expression. Used for & and | where we add ... to the
 // whole expression rather than individual operands.
@@ -316,7 +348,7 @@ func openCloseArg(expr ast.Expr) (ast.Expr, bool, embedFlags) {
 		// Non-struct argument: process like a regular embedding so that
 		// e.g. close(#A) → #A... when hoisted.
 		newExpr, _, f := openEmbedExpr(expr)
-		return newExpr, f.def || f.other || f.close, f
+		return newExpr, f.mayBeClosed(), f
 	}
 	var f embedFlags
 	var changed bool