tools/trim: optional fields should not remove non-optional
The solution is rather brute-force, we need to find something
more fine-grained eventually. But for now this avoids an
erroneous removal.
Fixing this will be easier when optional fields are represented
as normal fields with an optional flag, as they were in v0.2.
Fixes #855
Change-Id: I65829858b1856fcd09aa121f1e1c53fd49864c87
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/9221
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/tools/trim/testdata/optional.txtar b/tools/trim/testdata/optional.txtar
new file mode 100644
index 0000000..160b9ae
--- /dev/null
+++ b/tools/trim/testdata/optional.txtar
@@ -0,0 +1,37 @@
+// Optional fields should retain status after removal of unified
+// content.
+
+// Issue #855
+
+-- cue.mod/module.cue --
+module: "mod.com"
+-- a.cue --
+package pkg
+
+a: [...#A]
+
+a: [{
+ annotations: {}
+}]
+
+#A: annotations?: [string]: string
+
+b: #B
+b: bb: c: 2 // c can be removed, bb not.
+#B: bb?: c: 2
+
+-- out/trim --
+== a.cue
+package pkg
+
+a: [...#A]
+
+a: [{
+ annotations: {}
+}]
+
+#A: annotations?: [string]: string
+
+b: #B
+b: bb: {} // c can be removed, bb not.
+#B: bb?: c: 2
diff --git a/tools/trim/trim.go b/tools/trim/trim.go
index 5b20295..0199689 100644
--- a/tools/trim/trim.go
+++ b/tools/trim/trim.go
@@ -129,7 +129,7 @@
}
func (t *trimmer) markKeep(c adt.Conjunct) {
- if isDominator(c) {
+ if isDom, _ := isDominator(c); isDom {
return
}
if src := c.Expr().Source(); src != nil {
@@ -143,8 +143,15 @@
const dominatorNode = adt.ComprehensionSpan | adt.DefinitionSpan | adt.ConstraintSpan
// isDominator reports whether a node can remove other nodes.
-func isDominator(c adt.Conjunct) bool {
- return c.CloseInfo.IsInOneOf(dominatorNode)
+func isDominator(c adt.Conjunct) (ok, mayRemove bool) {
+ if !c.CloseInfo.IsInOneOf(dominatorNode) {
+ return false, false
+ }
+ switch c.Field().(type) {
+ case *adt.OptionalField: // bulk constraints handled elsewhere.
+ return true, false
+ }
+ return true, true
}
// Removable reports whether a non-dominator conjunct can be removed. This is
@@ -158,10 +165,10 @@
// themselves as it will eliminate the reason for the trigger.
func (t *trimmer) allowRemove(v *adt.Vertex) bool {
for _, c := range v.Conjuncts {
- isDom := isDominator(c)
+ _, allowRemove := isDominator(c)
loc := c.CloseInfo.Location() != c.Expr()
isSpan := c.CloseInfo.RootSpanType() != adt.ConstraintSpan
- if isDom && (loc || isSpan) {
+ if allowRemove && (loc || isSpan) {
return true
}
}
@@ -202,9 +209,11 @@
doms.Conjuncts = append(doms.Conjuncts, d.Conjuncts...)
}
+ hasDoms := false
for _, c := range v.Conjuncts {
+ isDom, _ := isDominator(c)
switch {
- case isDominator(c):
+ case isDom:
doms.AddConjunct(c)
default:
if r, ok := c.Expr().(adt.Resolver); ok {
@@ -236,6 +245,7 @@
ambiguous = true
}
+ _ = hasDoms
return doms, hasSubs, ambiguous, strict || ambiguous
}
@@ -310,7 +320,8 @@
}
for _, c := range v.Conjuncts {
- if !isDominator(c) && removable(c, v) {
+ _, allowRemove := isDominator(c)
+ if !allowRemove && removable(c, v) {
t.markRemove(c)
}
}
