Skip to content

Commit 6f25808

Browse files
committed
tools/fix: classify selector, call, and default embedding flags
collectEmbedFlags missed several expression kinds that openEmbedExpr classifies as possibly resolving to a closed struct: selectors, index expressions, and and()/or() calls. Embedding a conjunction with a selector operand therefore added no __reclose wrapper to the enclosing struct, silently dropping the closing that the old semantics implied, and comprehension field values of those shapes were not opened, closing fields that the old semantics left open. Since the fixer then made no changes at all to such files, they did not even get the @experiment(explicitopen) attribute. The default marker *X was similarly misclassified as never resolving to a closed struct. It takes on the closedness of its operand, but the disjunction it appears in may resolve to another branch, so it classifies as needing a runtime check. Make collectEmbedFlags the single classifier of embedded expressions, returning just the flags, and derive the rewrites of openEmbedExpr from it: the always-ellipsis conjunction, disjunction, and parenthesis cases, the close() hoist, and a flags-driven default. Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: I39bbae3bc5ab20959930b45f68a00e7710580446 Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1242612 Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com> TryBot-Result: CUEcueckoo <cueckoo@cuelang.org> Reviewed-by: Marcel van Lohuizen <mpvl@gmail.com>
1 parent 8e2ad07 commit 6f25808

3 files changed

Lines changed: 214 additions & 95 deletions

File tree

tools/fix/fix_test.go

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,6 @@ package foo
210210
// Selectors may resolve to closed values just like plain
211211
// references, so a conjunction with a selector operand needs
212212
// a runtime __reclose check on the enclosing struct.
213-
// TODO: the enclosing struct is left unwrapped; it should be
214-
// wrapped in __reclose to preserve the old behavior.
215213
name: "reclose embeddings of selector conjunctions (fixExplicitOpen)",
216214
exps: []string{"explicitopen"},
217215
in: `package foo
@@ -231,20 +229,17 @@ package foo
231229
#A: a: int
232230
h: inner: #A
233231
234-
v: {
232+
v: __reclose({
235233
(h.inner & {a: 1})...
236234
extra: 2
237-
}
235+
})
238236
`,
239237
},
240238

241239
{
242240
// Comprehension field values that may resolve to closed
243241
// structs via a selector conjunction or an and() call must
244242
// be opened like plain references.
245-
// TODO: the field values are left untouched; they should
246-
// become "(lib.v & {hc: port: 1})..." and "and([lib.v])..."
247-
// to preserve the old behavior.
248243
name: "open selector and call field values in comprehensions (fixExplicitOpen)",
249244
exps: []string{"explicitopen"},
250245
in: `package foo
@@ -263,7 +258,9 @@ lib: v: #HC
263258
}
264259
}
265260
`,
266-
out: `package foo
261+
out: `@experiment(explicitopen)
262+
263+
package foo
267264
268265
#HC: hc: {port: 1}
269266
lib: v: #HC
@@ -272,10 +269,10 @@ lib: v: #HC
272269
enable: bool
273270
egress?: [string]: {...}
274271
if enable {
275-
egress: lib.v & {hc: port: 1}
272+
egress: (lib.v & {hc: port: 1})...
276273
}
277274
if enable {
278-
egress2: and([lib.v])
275+
egress2: and([lib.v])...
279276
}
280277
}
281278
`,
@@ -286,9 +283,6 @@ lib: v: #HC
286283
// with a defaulted definition operand needs a runtime __reclose
287284
// check when embedded, and must be opened as a comprehension
288285
// field value.
289-
// TODO: the enclosing struct is left unwrapped and the field
290-
// value untouched; they should get __reclose and "..." to
291-
// preserve the old behavior.
292286
name: "default marker embedding flags (fixExplicitOpen)",
293287
exps: []string{"explicitopen"},
294288
in: `package foo
@@ -314,16 +308,16 @@ package foo
314308
315309
#A: {a: int}
316310
317-
v: {
311+
v: __reclose({
318312
(*#A | {})...
319313
extra: 1
320-
}
314+
})
321315
322316
#S: {
323317
enable: bool
324318
x?: {...}
325319
if enable {
326-
x: *#A | {}
320+
x: (*#A | {})...
327321
}
328322
}
329323
`,

tools/fix/fixopen.go

Lines changed: 73 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -222,136 +222,133 @@ func fixExplicitOpen(f *ast.File) (result *ast.File, hasChanges bool) {
222222
// old semantics, conjuncts inserted through comprehensions were treated
223223
// like embeddings and did not close their fields.
224224
func openCompFieldValue(expr ast.Expr) (ast.Expr, bool) {
225+
// PostfixExpr is excluded: the value already has an ellipsis.
225226
switch expr.(type) {
226-
case *ast.SelectorExpr, *ast.IndexExpr:
227-
return addEllipsis(expr), true
228-
case *ast.Ident, *ast.BinaryExpr, *ast.ParenExpr, *ast.CallExpr:
229-
if _, _, f := collectEmbedFlags(expr); f.mayBeClosed() {
227+
case *ast.Ident, *ast.SelectorExpr, *ast.IndexExpr,
228+
*ast.BinaryExpr, *ast.ParenExpr, *ast.CallExpr:
229+
if collectEmbedFlags(expr).mayBeClosed() {
230230
return addEllipsis(expr), true
231231
}
232232
}
233233
return expr, false
234234
}
235235

236236
// collectEmbedFlags recurses into an expression to collect embedding flags
237-
// without modifying the expression. Used for & and | where we add ... to the
238-
// whole expression rather than individual operands.
239-
func collectEmbedFlags(expr ast.Expr) (ast.Expr, bool, embedFlags) {
237+
// without modifying the expression. It is the single classifier of what an
238+
// embedded expression may resolve to; [openEmbedExpr] derives its rewrites
239+
// from the flags it returns.
240+
func collectEmbedFlags(expr ast.Expr) embedFlags {
240241
switch x := expr.(type) {
241242
case *ast.PostfixExpr:
242243
// Already has ellipsis (e.g. rewritten by a nested pass);
243244
// still collect flags from the underlying expression.
244245
if x.Op == token.ELLIPSIS {
245-
_, _, f := collectEmbedFlags(x.X)
246-
return expr, false, f
246+
return collectEmbedFlags(x.X)
247247
}
248248
case *ast.BinaryExpr:
249249
if x.Op == token.AND || x.Op == token.OR {
250-
_, _, xf := collectEmbedFlags(x.X)
251-
_, _, yf := collectEmbedFlags(x.Y)
250+
xf := collectEmbedFlags(x.X)
251+
yf := collectEmbedFlags(x.Y)
252252
f := xf.or(yf)
253253
if x.Op == token.OR && (xf.other || yf.other) {
254254
f.forceReclose = true
255255
}
256-
return expr, false, f
256+
return f
257257
}
258+
// Other binary ops (e.g. +, *) cannot resolve to closed structs.
259+
return embedFlags{}
258260
case *ast.ParenExpr:
259261
return collectEmbedFlags(x.X)
260262
case *ast.Ident:
261263
if x.Name == "_" {
262-
return expr, false, embedFlags{}
264+
return embedFlags{}
263265
}
264266
if internal.IsDefinition(x) {
265-
return expr, false, embedFlags{def: true}
267+
return embedFlags{def: true}
266268
}
267-
return expr, false, embedFlags{other: true}
269+
return embedFlags{other: true}
268270
case *ast.CallExpr:
269271
if id, ok := x.Fun.(*ast.Ident); ok {
270272
switch id.Name {
271273
case "close":
272-
_, _, f := openCloseArg(x.Args[0])
273-
return expr, false, f.or(embedFlags{close: true})
274+
f := embedFlags{close: true}
275+
if len(x.Args) == 1 {
276+
_, _, af := openCloseArg(x.Args[0])
277+
f = f.or(af)
278+
}
279+
return f
280+
case "and", "or":
281+
return embedFlags{other: true}
274282
}
275283
}
284+
return embedFlags{}
285+
case *ast.UnaryExpr:
286+
// The default marker *X takes on X's closedness, but the
287+
// disjunction it appears in may resolve to another branch,
288+
// so the closing always needs a runtime check.
289+
if x.Op == token.MUL && collectEmbedFlags(x.X).mayBeClosed() {
290+
return embedFlags{other: true}
291+
}
292+
return embedFlags{}
293+
case *ast.ListLit, // Lists cannot be opened anyway (atm).
294+
*ast.StructLit, // Structs are open by default.
295+
*ast.BasicLit,
296+
*ast.Interpolation:
297+
return embedFlags{}
276298
}
277-
return expr, false, embedFlags{}
299+
300+
// Default: may resolve to a closed struct (SelectorExpr, IndexExpr, etc.)
301+
return embedFlags{other: true}
278302
}
279303

280-
// openEmbedExpr adds postfix ellipsis to embedded expressions. For & and |
281-
// expressions, it adds ... to the whole expression. For other expressions, it
282-
// adds ellipsis if needed based on the expression type.
304+
// openEmbedExpr adds postfix ellipsis to embedded expressions, classifying
305+
// them via [collectEmbedFlags]. Conjunctions, disjunctions, and parenthesized
306+
// expressions always get ... on the whole expression; embedded close() calls
307+
// are hoisted; any other expression gets ... exactly when its flags indicate
308+
// it may resolve to a closed value.
283309
func openEmbedExpr(expr ast.Expr) (result ast.Expr, changed bool, flags embedFlags) {
284310
switch x := expr.(type) {
285311
case *ast.PostfixExpr:
286312
// Already has ellipsis; still collect flags from the underlying
287313
// expression, as they influence the wrapping of the enclosing
288314
// struct.
289-
return collectEmbedFlags(x)
315+
return expr, false, collectEmbedFlags(x)
290316

291317
case *ast.BinaryExpr:
292-
if x.Op == token.AND || x.Op == token.OR {
293-
// Collect flags from operands, then add ... to the
294-
// entire expression rather than each operand.
295-
_, _, xFlags := collectEmbedFlags(x.X)
296-
_, _, yFlags := collectEmbedFlags(x.Y)
297-
f := xFlags.or(yFlags)
298-
if x.Op == token.OR && (xFlags.other || yFlags.other) {
299-
f.forceReclose = true
300-
}
301-
return addEllipsis(expr), true, f
318+
if x.Op != token.AND && x.Op != token.OR {
319+
// Other binary ops (e.g. +, *) don't need ellipsis.
320+
return expr, false, embedFlags{}
302321
}
303-
// Other binary ops (e.g. +, *) don't need ellipsis.
304-
return expr, false, embedFlags{}
322+
// Add ... to the entire expression rather than each operand,
323+
// even when no operand may resolve to a closed value.
324+
return addEllipsis(expr), true, collectEmbedFlags(x)
305325

306326
case *ast.ParenExpr:
307-
// Recurse through parens to collect flags, then add ...
308-
// to the whole parenthesized expression.
309-
_, _, f := collectEmbedFlags(x.X)
310-
return addEllipsis(expr), true, f
311-
312-
case *ast.Ident:
313-
if x.Name == "_" {
314-
return expr, false, embedFlags{}
315-
}
316-
if internal.IsDefinition(x) {
317-
return addEllipsis(expr), true, embedFlags{def: true}
318-
}
319-
return addEllipsis(expr), true, embedFlags{other: true}
327+
// Add ... to the whole parenthesized expression.
328+
return addEllipsis(expr), true, collectEmbedFlags(x)
320329

321330
case *ast.CallExpr:
322-
if id, ok := x.Fun.(*ast.Ident); ok {
323-
switch id.Name {
324-
case "close":
325-
// In the old semantics, embedding close() opened up
326-
// the embedding — the outer struct stayed open. Under
327-
// explicitopen, close() no longer opens up when embedded.
328-
// Hoist close() to wrapper level: return the processed
329-
// argument as the new embedding, and set the close flag
330-
// so the containing struct gets close() wrapping.
331-
if len(x.Args) == 1 {
332-
newArg, _, f := openCloseArg(x.Args[0])
333-
f.close = true
334-
astutil.CopyMeta(newArg, x)
335-
return newArg, true, f
336-
}
337-
return expr, true, embedFlags{close: true}
338-
case "and", "or":
339-
return addEllipsis(expr), true, embedFlags{other: true}
331+
if id, ok := x.Fun.(*ast.Ident); ok && id.Name == "close" {
332+
// In the old semantics, embedding close() opened up
333+
// the embedding — the outer struct stayed open. Under
334+
// explicitopen, close() no longer opens up when embedded.
335+
// Hoist close() to wrapper level: return the processed
336+
// argument as the new embedding, and set the close flag
337+
// so the containing struct gets close() wrapping.
338+
if len(x.Args) == 1 {
339+
newArg, _, f := openCloseArg(x.Args[0])
340+
f.close = true
341+
astutil.CopyMeta(newArg, x)
342+
return newArg, true, f
340343
}
344+
return expr, true, embedFlags{close: true}
341345
}
342-
return expr, false, embedFlags{}
343-
344-
case *ast.ListLit, // Lists cannot be opened anyway (atm).
345-
*ast.StructLit, // Structs are open by default
346-
*ast.BasicLit,
347-
*ast.Interpolation,
348-
*ast.UnaryExpr:
349-
350-
return expr, false, embedFlags{}
351346
}
352347

353-
// Default: needs ellipsis (SelectorExpr, IndexExpr, etc.)
354-
return addEllipsis(expr), true, embedFlags{other: true}
348+
if f := collectEmbedFlags(expr); f.mayBeClosed() {
349+
return addEllipsis(expr), true, f
350+
}
351+
return expr, false, embedFlags{}
355352
}
356353

357354
// openCloseArg processes the argument of an embedded close() call,

0 commit comments

Comments
 (0)