From b1730b6f8bf3f3c82dc4b2d83141e9b051a89b98 Mon Sep 17 00:00:00 2001
From: Marcel van Lohuizen <mpvl@golang.org>
Date: Tue, 30 Mar 2021 08:41:23 +0200
Subject: [PATCH] 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>
---
 tools/trim/testdata/optional.txtar | 37 ++++++++++++++++++++++++++++++
 tools/trim/trim.go                 | 25 ++++++++++++++------
 2 files changed, 55 insertions(+), 7 deletions(-)
 create mode 100644 tools/trim/testdata/optional.txtar

diff --git a/tools/trim/testdata/optional.txtar b/tools/trim/testdata/optional.txtar
new file mode 100644
index 00000000000..160b9ae7a96
--- /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 5b202959fe9..01996898c35 100644
--- a/tools/trim/trim.go
+++ b/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)
 		}
 	}