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 <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>

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

tools/trim/trim.go

@@ -129,7 +129,7 @@ func (t *trimmer) markRemove(c adt.Conjunct) {
 }
 
 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 @@ func (t *trimmer) markKeep(c adt.Conjunct) {
 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 @@ func removable(c adt.Conjunct, v *adt.Vertex) bool {
 // 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 @@ func (t *trimmer) addDominators(d, v *adt.Vertex, hasDisjunction bool) (doms *ad
 		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 @@ func (t *trimmer) addDominators(d, v *adt.Vertex, hasDisjunction bool) (doms *ad
 		ambiguous = true
 	}
 
+	_ = hasDoms
 	return doms, hasSubs, ambiguous, strict || ambiguous
 }
 
@@ -310,7 +320,8 @@ func (t *trimmer) findSubordinates(doms, v *adt.Vertex, hasDisjunction bool) (re
 	}
 
 	for _, c := range v.Conjuncts {
-		if !isDominator(c) && removable(c, v) {
+		_, allowRemove := isDominator(c)
+		if !allowRemove && removable(c, v) {
 			t.markRemove(c)
 		}
 	}