Skip to content

Commit

Permalink
internal/core/adt: fix disjunct dedupping regression
Browse files Browse the repository at this point in the history
Equal was called on unfinalized values, which could lead
to premuture elimination of values.

Fixes #641

Change-Id: I56343ecee2c8bb6332903b5deed7906fd00925b3
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8224
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
  • Loading branch information
mpvl committed Jan 16, 2021
1 parent ed7d254 commit a086f74
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 12 deletions.
77 changes: 77 additions & 0 deletions cue/testdata/eval/disjunctions.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,31 @@ d100: {
j: i.bar
}

issue641: {
#A: {
type: "a"
x: ""
}

#B: {
type: "b"
x: string
}

#C: {
b: #A | #B
}

e: [string]: #C & {
b: #A | #B
}

e: foobar: #C & {
b: #B & {
x: "foobar"
}
}
}
-- out/eval --
Errors:
f: 2 errors in empty disjunction:
Expand Down Expand Up @@ -251,6 +276,33 @@ Result:
// ./in.cue:107:6
}
}
issue641: (struct){
#A: (#struct){
type: (string){ "a" }
x: (string){ "" }
}
#B: (#struct){
type: (string){ "b" }
x: (string){ string }
}
#C: (#struct){
b: (struct){ |((#struct){
type: (string){ "a" }
x: (string){ "" }
}, (#struct){
type: (string){ "b" }
x: (string){ string }
}) }
}
e: (struct){
foobar: (#struct){
b: (#struct){
type: (string){ "b" }
x: (string){ "foobar" }
}
}
}
}
}
-- out/compile --
--- in.cue
Expand Down Expand Up @@ -411,4 +463,29 @@ Result:
})
j: 〈0;i〉.bar
}
issue641: {
#A: {
type: "a"
x: ""
}
#B: {
type: "b"
x: string
}
#C: {
b: (〈1;#A〉|〈1;#B〉)
}
e: {
[string]: (〈1;#C〉 & {
b: (〈2;#A〉|〈2;#B〉)
})
}
e: {
foobar: (〈1;#C〉 & {
b: (〈2;#B〉 & {
x: "foobar"
})
})
}
}
}
26 changes: 14 additions & 12 deletions internal/core/adt/disjunct.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,22 +165,16 @@ func (n *nodeContext) expandDisjuncts(
}
return
}
if n.node.BaseValue == nil {
n.node.BaseValue = n.getValidators()
}

// TODO: clean up this mess:
result := *n.node // XXX: n.result = snapshotVertex(n.node)?

if recursive && state < Finalized {
*n = m
}
n.result = result
n.node = &n.result

if recursive {
*n = m
n.result = *n.node // XXX: n.result = snapshotVertex(n.node)?
n.node = &n.result
n.disjuncts = append(n.disjuncts, n)
}
if n.node.BaseValue == nil {
n.node.BaseValue = n.getValidators()
}

case len(n.disjunctions) > 0:
// Process full disjuncts to ensure that erroneous disjuncts are
Expand All @@ -201,6 +195,8 @@ func (n *nodeContext) expandDisjuncts(
// not be done unless there is complete information.
state = Partial
}
// TODO(perf): ideally always finalize. See comment below
// state = Finalized

for _, dn := range a {
switch {
Expand Down Expand Up @@ -272,6 +268,12 @@ func (n *nodeContext) expandDisjuncts(
outer:
for _, d := range n.disjuncts {
for _, v := range p.disjuncts {
// TODO(perf): not checking equality here may lead to polynomial
// blowup. Doesn't seem to affect large configurations, though,
// where this could matter at the moment.
if state != Finalized {
break
}
if Equal(n.ctx, &v.result, &d.result) {
if d.defaultMode == isDefault {
v.defaultMode = isDefault
Expand Down

0 comments on commit a086f74

Please sign in to comment.