Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Commit

Permalink
internal/core/adt: fix regressions in disjunct elimination
Browse files Browse the repository at this point in the history
Fix finalization of disjunctions. This bug was introduced
with the latest (critical) performance optimization for
disjunctions and was miraculously not caught by tests.

This also fixes a regression in v0.3 from v0.2 where
cyclic disjunctons were sometimes not fully resolved.

TODO: it probably makes sense to resolve defaults and
disambiguate not considering optionals during the
finalization step of disjunctions. A more concrete
proposal (no pun intended) is in the works.

Fixes #702
Fixes #700

Change-Id: Iae2a1c2a609dfb3a7e9f5ed3d7c6ee5ee17b3e55
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8462
Reviewed-by: CUE cueckoo <cueckoo@gmail.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
  • Loading branch information
mpvl committed Feb 2, 2021
1 parent bedb047 commit 8d537aa
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ xa2: xa3 + 2
xa3: 6 & xa1-2
xa4: xa2 + 2

// The second disjunct in xb4 can be eliminated as both disjuncts
// The second disjunct of xb1 can be eliminated as both disjuncts
// of xb3 result in an incompatible sum when substituted.
xb1: (xb2 & 8) | (xb4 & 9)
xb2: xb3 + 2
Expand Down Expand Up @@ -174,16 +174,10 @@ Result:
xa2: (int){ 8 }
xa3: (int){ 6 }
xa4: (int){ 10 }
xb1: (int){ |((int){ 8 }, (int){ 9 }) }
xb2: (_|_){
// [incomplete] xb2: unresolved disjunction 6 | 9 (type int):
// ./in.cue:18:6
}
xb3: (int){ |((int){ 6 }, (int){ 9 }) }
xb4: (_|_){
// [incomplete] xb2: unresolved disjunction 6 | 9 (type int):
// ./in.cue:18:6
}
xb1: (int){ 8 }
xb2: (int){ 8 }
xb3: (int){ 6 }
xb4: (int){ 10 }
xc1: (int){ |((int){ 8 }, (int){ 9 }) }
xc2: (int){ 8 }
xc3: (_|_){
Expand Down Expand Up @@ -226,16 +220,10 @@ Result:
// ./in.cue:45:6
// ./in.cue:45:10
}
xf1: (int){ |((int){ 8 }, (int){ 9 }) }
xf2: (_|_){
// [incomplete] xf2: unresolved disjunction 6 | 9 (type int):
// ./in.cue:52:6
}
xf3: (int){ |((int){ 6 }, (int){ 9 }) }
xf4: (_|_){
// [incomplete] xf2: unresolved disjunction 6 | 9 (type int):
// ./in.cue:52:6
}
xf1: (int){ 8 }
xf2: (int){ 8 }
xf3: (int){ 6 }
xf4: (int){ 10 }
z1: (int){ |((int){ 11 }, (int){ 13 }) }
z2: (int){ 10 }
z3: (_|_){
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ xa2: xa3 + 2
xa3: 6 & xa1-2
xa4: xa2 + 2

// As xb3 is a disjunction, xb2 cannot be resolved and evaluating
// the cycle completely is broken. However, it is not an error
// as the user might still resolve the disjunction.
xb1: *(xb2 & 8) | (xb4 & 9)
xb2: xb3 + 2
xb3: *(6 & (xb1 - 2)) | (xb4 & 9)
Expand Down Expand Up @@ -131,11 +128,11 @@ z3: 8
Errors:
xe1: 2 errors in empty disjunction:
xe1: conflicting values 8 and 9:
./in.cue:36:14
./in.cue:41:6
xe3: conflicting values 7 and 6:
./in.cue:33:14
./in.cue:38:6
./in.cue:38:10
xe3: conflicting values 7 and 6:
./in.cue:35:6
./in.cue:35:10

Result:
(_|_){
Expand All @@ -146,7 +143,7 @@ Result:
xa4: (int){ 10 }
xb1: (int){ 8 }
xb2: (int){ 8 }
xb3: (int){ |(*(int){ 6 }, (int){ 9 }) }
xb3: (int){ 6 }
xb4: (int){ 10 }
xc1: (int){ |(*(int){ 8 }, (int){ 9 }) }
xc2: (int){ 8 }
Expand All @@ -161,31 +158,31 @@ Result:
xe1: (_|_){
// [eval] xe1: 2 errors in empty disjunction:
// xe1: conflicting values 8 and 9:
// ./in.cue:36:14
// ./in.cue:41:6
// xe3: conflicting values 7 and 6:
// ./in.cue:33:14
// ./in.cue:38:6
// ./in.cue:38:10
// xe3: conflicting values 7 and 6:
// ./in.cue:35:6
// ./in.cue:35:10
}
xe2: (_|_){
// [eval] xe3: conflicting values 7 and 6:
// ./in.cue:38:6
// ./in.cue:38:10
// ./in.cue:35:6
// ./in.cue:35:10
}
xe3: (_|_){
// [eval] xe3: conflicting values 7 and 6:
// ./in.cue:38:6
// ./in.cue:38:10
// ./in.cue:35:6
// ./in.cue:35:10
}
xe4: (_|_){
// [eval] xe3: conflicting values 7 and 6:
// ./in.cue:38:6
// ./in.cue:38:10
// ./in.cue:35:6
// ./in.cue:35:10
}
xe5: (_|_){
// [eval] xe3: conflicting values 7 and 6:
// ./in.cue:38:6
// ./in.cue:38:10
// ./in.cue:35:6
// ./in.cue:35:10
}
z1: (int){ |(*(int){ 11 }, (int){ 13 }) }
z2: (int){ 10 }
Expand Down
110 changes: 110 additions & 0 deletions cue/testdata/disjunctions/incomplete.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
-- in.cue --
import "encoding/yaml"

issue700: {
a:y: "y"
test1: *a.x | 1
test2: *a.y | 1
}

lookup: {
x: {
name: "Hello"
}
y: x.a
ok1: *x.a | x
ok2: x | *x.a
ok3: x.a | *x
ok4: *x | x.a
ok5: x | x.a
ok5: x.a | x
allFail1: x.a | x.b
allFail2: x.a | x.b
}

func: {
s: string
ok1: yaml.MarshalStream(s) | yaml.Marshal(s)
}

-- out/eval --
(struct){
issue700: (struct){
a: (struct){
y: (string){ "y" }
}
test1: (int){ 1 }
test2: ((int|string)){ |(*(string){ "y" }, (int){ 1 }) }
}
lookup: (struct){
x: (struct){
name: (string){ "Hello" }
}
y: (_|_){
// [incomplete] lookup.y: undefined field a:
// ./in.cue:13:10
}
ok1: (struct){
name: (string){ "Hello" }
}
ok2: (struct){
name: (string){ "Hello" }
}
ok3: (struct){
name: (string){ "Hello" }
}
ok4: (struct){
name: (string){ "Hello" }
}
ok5: (struct){
name: (string){ "Hello" }
}
allFail1: (_|_){
// [incomplete] lookup.allFail1: 1 errors in empty disjunction:
// lookup.allFail1: undefined field a:
// ./in.cue:20:18
}
allFail2: (_|_){
// [incomplete] lookup.allFail2: 1 errors in empty disjunction:
// lookup.allFail2: undefined field a:
// ./in.cue:21:18
}
}
func: (struct){
s: (string){ string }
ok1: (_|_){
// [incomplete] func.ok1: 1 errors in empty disjunction:
// func.ok1: non-concrete argument 0:
// ./in.cue:26:10
}
}
}
-- out/compile --
--- in.cue
{
issue700: {
a: {
y: "y"
}
test1: (*〈0;a〉.x|1)
test2: (*〈0;a〉.y|1)
}
lookup: {
x: {
name: "Hello"
}
y: 〈0;x〉.a
ok1: (*〈0;x〉.a|〈0;x〉)
ok2: (〈0;x〉|*〈0;x〉.a)
ok3: (〈0;x〉.a|*〈0;x〉)
ok4: (*〈0;x〉|〈0;x〉.a)
ok5: (〈0;x〉|〈0;x〉.a)
ok5: (〈0;x〉.a|〈0;x〉)
allFail1: (〈0;x〉.a|〈0;x〉.b)
allFail2: (〈0;x〉.a|〈0;x〉.b)
}
func: {
s: string
ok1: (〈import;"encoding/yaml"〉.MarshalStream(〈0;s〉)|〈import;"encoding/yaml"〉.Marshal(〈0;s〉))
}
}
62 changes: 50 additions & 12 deletions internal/core/adt/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,7 @@ func (c *OpContext) Unify(v *Vertex, state VertexStatus) {
}
n.expandDisjuncts(disState, n, maybeDefault, false)

for _, d := range n.disjuncts {
d.finalizeIncomplete()
d.free()
}
n.finalizeDisjuncts()

switch len(n.disjuncts) {
case 0:
Expand Down Expand Up @@ -354,7 +351,11 @@ func (c *OpContext) Unify(v *Vertex, state VertexStatus) {

// We don't do this in postDisjuncts, as it should only be done after
// completing all disjunctions.
n.finalizeIncomplete()
if !n.done() {
if err := n.incompleteErrors(); err != nil {
n.node.BaseValue = err
}
}

if state != Finalized {
return
Expand Down Expand Up @@ -385,15 +386,34 @@ func (n *nodeContext) insertConjuncts() {
}
}

// finalizeIncomplete collects all uncompleted expressions and adds them as
// errors. As disjuncts are always evaluated with Finalized, care should be
// taken to only call this after all disjunctions in a path have been completed.
func (n *nodeContext) finalizeIncomplete() {
if !n.done() {
if err := n.incompleteErrors(); err != nil {
n.node.BaseValue = err
// finalizeDisjuncts: incomplete errors are kept around and not removed early.
// This call filters the incomplete errors and removes them
//
// This also collects all errors of empty disjunctions. These cannot be
// collected during the finalization state of individual disjuncts. Care should
// be taken to only call this after all disjuncts have been finalized.
func (n *nodeContext) finalizeDisjuncts() {
a := n.disjuncts
if len(a) == 0 {
return
}
k := 0
for i, d := range a {
switch d.finalDone() {
case true:
a[k], a[i] = d, a[k]
k++
default:
if err := d.incompleteErrors(); err != nil {
n.disjunctErrs = append(n.disjunctErrs, err)
}
}
d.free()
}
if k == 0 {
n.makeError()
}
n.disjuncts = a[:k]
}

func (n *nodeContext) doNotify() {
Expand Down Expand Up @@ -668,6 +688,11 @@ func (n *nodeContext) createDisjunct() *Disjunction {
a[i] = v
}
}
// TODO: disambiguate based on concrete values.
// TODO: consider not storing defaults.
// if p > 0 {
// a = a[:p]
// }
return &Disjunction{
Values: a,
NumDefaults: p,
Expand Down Expand Up @@ -952,6 +977,19 @@ func (n *nodeContext) done() bool {
len(n.exprs) == 0
}

// finalDone is like done, but allows for cycle errors, which can be ignored
// as they essentially indicate a = a & _.
func (n *nodeContext) finalDone() bool {
for _, x := range n.exprs {
if x.err.Code != CycleError {
return false
}
}
return len(n.dynamicFields) == 0 &&
len(n.ifClauses) == 0 &&
len(n.forClauses) == 0
}

// hasErr is used to determine if an evaluation path, for instance a single
// path after expanding all disjunctions, has an error.
func (n *nodeContext) hasErr() bool {
Expand Down

0 comments on commit 8d537aa

Please sign in to comment.