tools/trim: fix too aggressive elimination for disjunctions

See comments in trim.go

Fixes #801
Fixes #781

Change-Id: Ia86ed8530b5206e7b3196b9f5bc848bfbe373548
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8922
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>

cmd/cue/cmd/testdata/script/trim.txt

@@ -56,7 +56,7 @@ group: {
 		aa: 8 // new value
 	}
 
-	comp: {} // TODO: remove: implied by comprehension above
+	comp: baz: {} // TODO: remove: implied by comprehension above
 }
 -- trim/trim.cue --
 package trim

tools/trim/testdata/defaults.txtar

@@ -10,6 +10,62 @@ subToDom: {
     something: ip: *"default" | string
     something: #maybeString
 }
+
+// references to definitions of a disjunction should be resolved and counted
+// as dominator nodes.
+resolveDefaults: {
+    #monitor: {
+        kind: "a"
+    } | {
+        kind: "b"
+    }
+
+    monitor: #monitor
+
+    monitor: kind: "a"
+}
+
+issue781: {
+    #monitor_check: {
+        check_name:      string
+        check_interval?: string
+    }
+
+    #monitor_check: {
+        check_type: "nginx_config"
+        } | {
+        check_type: "docker_running"
+        vars: {
+            container_name: string
+        }
+    }
+
+
+    monitor: {
+        checks: [...#monitor_check]
+    }
+
+    monitor: {
+        checks: [{
+            check_type: "nginx_config"
+            check_name: "nginx_config"
+        }]
+    }
+}
+
+// Issue #801
+// Here the concrete value selects the default from a dominator, after which
+// the dominator becomes an exact match. The exact match should not allow it
+// to be erased, as the exact match is only there because subordinate value
+// was first used to select the default.
+dontEraseDefaultSelection: {
+    rule: _#Rule & {
+        verbs: [ "c" ]
+    }
+    _#Rule: {
+        verbs: *["a", "b"] | ["c"]
+    }
+}
 -- out/trim --
 == in.cue
 domToSub: {
@@ -23,3 +79,58 @@ subToDom: {
 	something: ip: *"default" | string
 	something: #maybeString
 }
+
+// references to definitions of a disjunction should be resolved and counted
+// as dominator nodes.
+resolveDefaults: {
+	#monitor: {
+		kind: "a"
+	} | {
+		kind: "b"
+	}
+
+	monitor: #monitor
+
+	monitor: kind: "a"
+}
+
+issue781: {
+	#monitor_check: {
+		check_name:      string
+		check_interval?: string
+	}
+
+	#monitor_check: {
+		check_type: "nginx_config"
+	} | {
+		check_type: "docker_running"
+		vars: {
+			container_name: string
+		}
+	}
+
+	monitor: {
+		checks: [...#monitor_check]
+	}
+
+	monitor: {
+		checks: [{
+			check_type: "nginx_config"
+			check_name: "nginx_config"
+		}]
+	}
+}
+
+// Issue #801
+// Here the concrete value selects the default from a dominator, after which
+// the dominator becomes an exact match. The exact match should not allow it
+// to be erased, as the exact match is only there because subordinate value
+// was first used to select the default.
+dontEraseDefaultSelection: {
+	rule: _#Rule & {
+		verbs: [ "c"]
+	}
+	_#Rule: {
+		verbs: *["a", "b"] | ["c"]
+	}
+}

tools/trim/trim.go

@@ -86,7 +86,8 @@ func Files(files []*ast.File, inst *cue.Instance, cfg *Config) error {
 		w:       os.Stderr,
 	}
 
-	t.findSubordinates(v)
+	d, _, _, pickedDefault := t.addDominators(nil, v, false)
+	t.findSubordinates(d, v, pickedDefault)
 
 	// Remove subordinate values from files.
 	for _, f := range files {
@@ -177,7 +178,68 @@ const (
 	yes
 )
 
-func (t *trimmer) findSubordinates(v *adt.Vertex) (result int) {
+// addDominators injects dominator values from v into d. If no default has
+// been selected from dominators so far, the values are erased. Otherwise,
+// both default and new values are merged.
+//
+// Erasing the previous values when there has been no default so far allows
+// interpolations, for instance, to be evaluated in the new context and
+// eliminated.
+//
+// Values are kept when there has been a default (current or ancestor) because
+// the current value may contain information that caused that default to be
+// selected and thus erasing it would cause that information to be lost.
+//
+// TODO:
+// In principle, information only needs to be kept for discriminator values, or
+// any value that was instrumental in selecting the default. This is currently
+// hard to do, however, so we just fall back to a stricter mode in the presence
+// of defaults.
+func (t *trimmer) addDominators(d, v *adt.Vertex, hasDisjunction bool) (doms *adt.Vertex, ambiguous, hasSubs, strict bool) {
+	strict = hasDisjunction
+	doms = &adt.Vertex{}
+	if d != nil && hasDisjunction {
+		doms.Conjuncts = append(doms.Conjuncts, d.Conjuncts...)
+	}
+
+	for _, c := range v.Conjuncts {
+		switch {
+		case isDominator(c):
+			doms.AddConjunct(c)
+		default:
+			if r, ok := c.Expr().(adt.Resolver); ok {
+				x, _ := t.ctx.Resolve(c.Env, r)
+				// Even if this is not a dominator now, descendants will be.
+				if x.Label.IsDef() {
+					for _, c := range x.Conjuncts {
+						doms.AddConjunct(c)
+					}
+					break
+				}
+			}
+			hasSubs = true
+		}
+	}
+	doms.Finalize(t.ctx)
+
+	switch x := doms.Value().(type) {
+	case *adt.Disjunction:
+		switch x.NumDefaults {
+		case 1:
+			strict = true
+		default:
+			ambiguous = true
+		}
+	}
+
+	if doms = doms.Default(); doms.IsErr() {
+		ambiguous = true
+	}
+
+	return doms, hasSubs, ambiguous, strict || ambiguous
+}
+
+func (t *trimmer) findSubordinates(doms, v *adt.Vertex, hasDisjunction bool) (result int) {
 	defer un(t.trace(v))
 	defer func() {
 		if result == no {
@@ -187,14 +249,21 @@ func (t *trimmer) findSubordinates(v *adt.Vertex) (result int) {
 		}
 	}()
 
+	doms, hasSubs, ambiguous, pickedDefault := t.addDominators(doms, v, hasDisjunction)
+
+	if ambiguous {
+		return no
+	}
+
 	// TODO(structure sharing): do not descend into vertices whose parent is not
 	// equal to the parent. This is not relevant at this time, but may be so in
 	// the future.
 
 	if len(v.Arcs) > 0 {
 		var match int
 		for _, a := range v.Arcs {
-			match |= t.findSubordinates(a)
+			d := doms.Lookup(a.Label)
+			match |= t.findSubordinates(d, a, pickedDefault)
 		}
 
 		// This also skips embedded scalars if not all fields are removed. In
@@ -217,32 +286,18 @@ func (t *trimmer) findSubordinates(v *adt.Vertex) (result int) {
 		// have to check additional optional constraints to pass subsumption.
 
 	default:
-		doms := &adt.Vertex{}
-		hasSubs := false
-
-		for _, c := range v.Conjuncts {
-			if isDominator(c) {
-				doms.AddConjunct(c)
-			} else {
-				hasSubs = true
-			}
-		}
-
 		if !hasSubs {
-			return maybe // only if there are siblings to be removed.
+			return maybe
 		}
 
-		doms.Finalize(t.ctx)
-		doms = doms.Default()
-
 		// This should normally not be necessary, as subsume should catch this.
 		// But as we already take the default value for doms, it doesn't hurt to
 		// do it.
 		v = v.Default()
 
 		// This is not necessary, but seems like it may result in more
 		// user-friendly semantics.
-		if doms.IsErr() || v.IsErr() {
+		if v.IsErr() {
 			return no
 		}