internal/core/adt: fix trim bug
1) Ensure that imports are removed when not needed
(using astutil.Sanitize)
2) ConstraintSpan starts at the arcs of patttern constraints,
not roots. Added mechanism to mark Conjuncts as containing
roots of pattern/additional constraints so that these are
excluded from removal.
Fixes #716
Change-Id: I4a524cd2abc3e13070cab090325135db6eb133c7
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8664
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
diff --git a/internal/core/adt/closed.go b/internal/core/adt/closed.go
index 9cf590b..25e431b 100644
--- a/internal/core/adt/closed.go
+++ b/internal/core/adt/closed.go
@@ -93,7 +93,8 @@
type CloseInfo struct {
*closeInfo
- IsClosed bool
+ IsClosed bool
+ FieldTypes OptionalType
}
func (c CloseInfo) Location() Node {
diff --git a/internal/core/adt/composite.go b/internal/core/adt/composite.go
index 6f19b19..f128606 100644
--- a/internal/core/adt/composite.go
+++ b/internal/core/adt/composite.go
@@ -488,7 +488,7 @@
// OptionalType is a bit field of the type of optional constraints in use by an
// Acceptor.
-type OptionalType int
+type OptionalType int8
const (
HasField OptionalType = 1 << iota // X: T
diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go
index addd784..2ebc66f 100644
--- a/internal/core/adt/eval.go
+++ b/internal/core/adt/eval.go
@@ -385,9 +385,16 @@
// insertConjuncts inserts conjuncts previously uninserted.
func (n *nodeContext) insertConjuncts() {
for len(n.conjuncts) > 0 {
- x := n.conjuncts[0]
+ nInfos := len(n.node.Structs)
+ p := &n.conjuncts[0]
n.conjuncts = n.conjuncts[1:]
- n.addExprConjunct(x)
+ n.addExprConjunct(*p)
+
+ // Record the OptionalTypes for all structs that were inferred by this
+ // Conjunct. This information can be used by algorithms such as trim.
+ for i := nInfos; i < len(n.node.Structs); i++ {
+ p.CloseInfo.FieldTypes |= n.node.Structs[i].types
+ }
}
}
diff --git a/tools/trim/testdata/constraintroots.txtar b/tools/trim/testdata/constraintroots.txtar
new file mode 100644
index 0000000..7f47d4b
--- /dev/null
+++ b/tools/trim/testdata/constraintroots.txtar
@@ -0,0 +1,104 @@
+// Issue #716
+-- in.cue --
+keepPatternConstraintRoot: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ deployment: name: spec: []
+
+ deployment: [string]: spec: []
+}
+
+// TODO(additional): add once implemented
+// keepAdditionalRoots: {
+// if true {
+// deployment: "name": {spec: [1]}
+// }
+//
+// deployment: name: spec: [1]
+//
+// deployment: [string]: spec: [1]
+// }
+
+keepPatternConstraintRootSolo: {
+ if true {
+ deployment: "name": {spec: [1]}
+ }
+
+ deployment: [string]: spec: [1]
+}
+
+keepPatternConstraintRootIndirect: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ deployment: name: spec: []
+
+ deployment: indirect
+
+ indirect: {{[string]: spec: []}}
+}
+
+keepPatternConstraintRootDef: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ #Deployment: spec: []
+
+ deployment: name: #Deployment
+
+ deployment: [string]: spec: []
+}
+
+
+-- out/trim --
+== in.cue
+keepPatternConstraintRoot: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ deployment: [string]: spec: []
+}
+
+// TODO(additional): add once implemented
+// keepAdditionalRoots: {
+// if true {
+// deployment: "name": {spec: [1]}
+// }
+//
+// deployment: name: spec: [1]
+//
+// deployment: [string]: spec: [1]
+// }
+
+keepPatternConstraintRootSolo: {
+ if true {
+ deployment: "name": {spec: [1]}
+ }
+
+ deployment: [string]: spec: [1]
+}
+
+keepPatternConstraintRootIndirect: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ deployment: indirect
+
+ indirect: {{[string]: spec: []}}
+}
+
+keepPatternConstraintRootDef: {
+ if true {
+ deployment: "name": {spec: []}
+ }
+
+ #Deployment: spec: []
+
+ deployment: [string]: spec: []
+}
diff --git a/tools/trim/testdata/rmimport.txtar b/tools/trim/testdata/rmimport.txtar
new file mode 100644
index 0000000..f48431f
--- /dev/null
+++ b/tools/trim/testdata/rmimport.txtar
@@ -0,0 +1,38 @@
+// Issue #716
+//
+// NOTE removing the empty import decl is more a job for
+// cue/format or astutil.Sanitize.
+
+-- a/a.cue --
+package a
+
+A: 5
+-- b.cue --
+package b
+
+import (
+ "example.com/blah/a"
+)
+
+#Def: {
+ y: 5
+}
+
+x: #Def
+x: y: a.A
+-- cue.mod/module.cue --
+module: "example.com/blah"
+
+-- out/trim --
+== b.cue
+package b
+
+import (
+)
+
+#Def: {
+ y: 5
+}
+
+x: #Def
+x: {}
diff --git a/tools/trim/trim.go b/tools/trim/trim.go
index 17e164c..96a6a03 100644
--- a/tools/trim/trim.go
+++ b/tools/trim/trim.go
@@ -89,6 +89,9 @@
}
return true
}, nil)
+ if err := astutil.Sanitize(f); err != nil {
+ return err
+ }
}
return nil
@@ -109,10 +112,18 @@
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)
}
+// Removable reports whether a non-dominator conjunct can be removed. This is
+// not the case if it has pattern constraints that could turn into dominator
+// nodes.
+func removable(c adt.Conjunct) bool {
+ return c.CloseInfo.FieldTypes&(adt.HasAdditional|adt.HasPattern) == 0
+}
+
// Roots of constraints are not allowed to strip conjuncts by
// themselves as it will eliminate the reason for the trigger.
func allowRemove(v *adt.Vertex) bool {
@@ -200,7 +211,7 @@
}
for _, c := range v.Conjuncts {
- if !isDominator(c) {
+ if !isDominator(c) && removable(c) {
t.markRemove(c)
}
}
