tools/trim: don't trim shared nodes
This one is a bit tricky. We accomplish this by explicitly marking
all nodes that cannot be removed. This seems like a good general
safety measure to make anyway, albeit a bit expensive.
Fixes #760
Change-Id: Ie46e398dd5295cedf150c11f38cdf82d68306180
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8702
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
diff --git a/tools/trim/testdata/shared.txtar b/tools/trim/testdata/shared.txtar
new file mode 100644
index 0000000..1aef0f1
--- /dev/null
+++ b/tools/trim/testdata/shared.txtar
@@ -0,0 +1,76 @@
+// Removing fields from shared nodes not supported.
+-- in.cue --
+import "strings"
+
+// TODO: this would be okay to normalize. Consider whether
+// this makes sense, though.
+a: {
+ service: [ID=_]: name: "a"
+
+ service: foo: shared
+ service: bar: shared
+
+ shared: name: "a"
+}
+
+issue760: {
+
+ service: [ID=_]: {
+ name: ID
+ }
+ service: foo: _shared
+ service: bar: _shared
+ _shared: {
+ name: "foo" // Do not remove!
+ }
+}
+
+
+issue760: {
+ service: [ID=_]: {
+ _service_name: *strings.TrimSuffix(ID, "-suffix") | string
+ }
+ service: "a-suffix": _shared
+ service: "b-suffix": _shared
+ _shared: {
+ _service_name: "a" // Do not remove!
+ }
+}
+
+-- out/trim --
+== in.cue
+import "strings"
+
+// TODO: this would be okay to normalize. Consider whether
+// this makes sense, though.
+a: {
+ service: [ID=_]: name: "a"
+
+ service: foo: shared
+ service: bar: shared
+
+ shared: name: "a"
+}
+
+issue760: {
+
+ service: [ID=_]: {
+ name: ID
+ }
+ service: foo: _shared
+ service: bar: _shared
+ _shared: {
+ name: "foo" // Do not remove!
+ }
+}
+
+issue760: {
+ service: [ID=_]: {
+ _service_name: *strings.TrimSuffix(ID, "-suffix") | string
+ }
+ service: "a-suffix": _shared
+ service: "b-suffix": _shared
+ _shared: {
+ _service_name: "a" // Do not remove!
+ }
+}
diff --git a/tools/trim/trim.go b/tools/trim/trim.go
index 5c4100f..7058259 100644
--- a/tools/trim/trim.go
+++ b/tools/trim/trim.go
@@ -78,11 +78,12 @@
v := vx.(*adt.Vertex)
t := &trimmer{
- Config: *cfg,
- ctx: adt.NewContext(r, v),
- remove: map[ast.Node]bool{},
- debug: Debug,
- w: os.Stderr,
+ Config: *cfg,
+ ctx: adt.NewContext(r, v),
+ remove: map[ast.Node]bool{},
+ exclude: map[ast.Node]bool{},
+ debug: Debug,
+ w: os.Stderr,
}
t.findSubordinates(v)
@@ -90,7 +91,7 @@
// Remove subordinate values from files.
for _, f := range files {
astutil.Apply(f, func(c astutil.Cursor) bool {
- if f, ok := c.Node().(*ast.Field); ok && t.remove[f.Value] {
+ if f, ok := c.Node().(*ast.Field); ok && t.remove[f.Value] && !t.exclude[f.Value] {
c.Delete()
}
return true
@@ -106,8 +107,9 @@
type trimmer struct {
Config
- ctx *adt.OpContext
- remove map[ast.Node]bool
+ ctx *adt.OpContext
+ remove map[ast.Node]bool
+ exclude map[ast.Node]bool
debug bool
indent int
@@ -125,6 +127,18 @@
}
}
+func (t *trimmer) markKeep(c adt.Conjunct) {
+ if isDominator(c) {
+ return
+ }
+ if src := c.Expr().Source(); src != nil {
+ t.exclude[src] = true
+ if t.debug {
+ t.logf("keeping")
+ }
+ }
+}
+
const dominatorNode = adt.ComprehensionSpan | adt.DefinitionSpan | adt.ConstraintSpan
// isDominator reports whether a node can remove other nodes.
@@ -135,17 +149,18 @@
// 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 {
+func removable(c adt.Conjunct, v *adt.Vertex) 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 {
+func (t *trimmer) allowRemove(v *adt.Vertex) bool {
for _, c := range v.Conjuncts {
- if isDominator(c) &&
- (c.CloseInfo.Location() != c.Expr() ||
- c.CloseInfo.RootSpanType() != adt.ConstraintSpan) {
+ isDom := isDominator(c)
+ loc := c.CloseInfo.Location() != c.Expr()
+ isSpan := c.CloseInfo.RootSpanType() != adt.ConstraintSpan
+ if isDom && (loc || isSpan) {
return true
}
}
@@ -162,8 +177,15 @@
yes
)
-func (t *trimmer) findSubordinates(v *adt.Vertex) int {
+func (t *trimmer) findSubordinates(v *adt.Vertex) (result int) {
defer un(t.trace(v))
+ defer func() {
+ if result == no {
+ for _, c := range v.Conjuncts {
+ t.markKeep(c)
+ }
+ }
+ }()
// 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
@@ -184,7 +206,7 @@
}
}
- if !allowRemove(v) {
+ if !t.allowRemove(v) {
return no
}
@@ -228,7 +250,7 @@
}
for _, c := range v.Conjuncts {
- if !isDominator(c) && removable(c) {
+ if !isDominator(c) && removable(c, v) {
t.markRemove(c)
}
}
