diff --git a/cue/testdata/cycle/builtins.txtar b/cue/testdata/cycle/builtins.txtar index c6537a1d21f..3bb10839023 100644 --- a/cue/testdata/cycle/builtins.txtar +++ b/cue/testdata/cycle/builtins.txtar @@ -90,42 +90,96 @@ issue3443: { // This is not a structural cycle, and should not hang, as n? is optional. #S: matchN(1, [{n?: #S & (int | {})}]) - // TODO: hang! This hangs with v2 and v3 - // s: #S - // s: n: n: _ + noHang: { + s: #S + s: n: n: _ + } } noCycle2: { #S: matchN(1, [{n?: (int | #S)}]) } - cycleButOkay: { + cycle1: { // This correct CUE, as matchN allows for schema to be errors. We should // probably have a vet rule to catch this,though. #S: matchN(1, [{n: #S}]) // This is not an error as the result is structure shared. Not sure if // this should be accepted. - s: #S - s: n: n: _ + ok: { + s: #S + s: _ + } + + // This unifies deep enough to cause a structural cycle. It probably + // should not. + cycle: { + s: #S + s: n: n: _ + } + } + + cycle2: { + // TODO: this should probably fail, or at least be consistent with + // cycle1.cycle. + fail: #S: matchN(1, [{n: #S}]) & {n: n: n: _} } } -- todo/p1 -- issue3443.noCycle: fix hang +-- out/evalalpha/stats -- +Leaks: 222 +Freed: 17 +Reused: 17 +Allocs: 222 +Retain: 0 + +Unifications: 194 +Conjuncts: 760 +Disjuncts: 28 +-- diff/-out/evalalpha/stats<==>+out/eval/stats -- +diff old new +--- old ++++ new +@@ -1,9 +1,9 @@ +-Leaks: 12 +-Freed: 235 +-Reused: 218 +-Allocs: 29 +-Retain: 65 ++Leaks: 222 ++Freed: 17 ++Reused: 17 ++Allocs: 222 ++Retain: 0 + +-Unifications: 211 +-Conjuncts: 389 +-Disjuncts: 290 ++Unifications: 194 ++Conjuncts: 760 ++Disjuncts: 28 -- out/eval/stats -- -Leaks: 8 -Freed: 70 -Reused: 62 -Allocs: 16 -Retain: 45 +Leaks: 12 +Freed: 235 +Reused: 218 +Allocs: 29 +Retain: 65 -Unifications: 78 -Conjuncts: 111 -Disjuncts: 105 +Unifications: 211 +Conjuncts: 389 +Disjuncts: 290 -- out/evalalpha -- Errors: issue3443.matchIf.#S: cannot call non-function matchIf (type struct): ./matchn.cue:16:7 +issue3443.cycle1.cycle.s: invalid value {n:{n:_}} (does not satisfy matchN(1, [{n:matchN(1, [{n:#S}])}])): 0 matched, expected 1: + ./matchn.cue:35:7 + ./matchn.cue:35:14 +issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: structural cycle))): 0.n: 0 matched, expected 1))): 0.n: 0 matched, expected 1))): 0 matched, expected 1: + ./matchn.cue:55:13 + ./matchn.cue:55:20 Result: (_|_){ @@ -231,6 +285,13 @@ Result: // }] } }) } + noHang: (struct){ + s: (#struct){ + n: (struct){ + n: (_){ _ } + } + } + } } noCycle2: (struct){ #S: (_){ matchN(1, (#list){ @@ -240,16 +301,47 @@ Result: } }) } } - cycleButOkay: (struct){ + cycle1: (_|_){ + // [eval] #S: (_){ matchN(1, (#list){ 0: (_|_){// &[{ // n: 〈2;#S〉 // }] } }) } - s: (#struct){ - n: (struct){ - n: (_){ _ } + ok: (struct){ + s: (_){ matchN(1, (#list){ + 0: (_|_){// &[{ + // n: 〈2;#S〉 + // }] + } + }) } + } + cycle: (_|_){ + // [eval] + s: (_|_){ + // [eval] issue3443.cycle1.cycle.s: invalid value {n:{n:_}} (does not satisfy matchN(1, [{n:matchN(1, [{n:#S}])}])): 0 matched, expected 1: + // ./matchn.cue:35:7 + // ./matchn.cue:35:14 + n: (struct){ + n: (_){ _ } + } + } + } + } + cycle2: (_|_){ + // [eval] + fail: (_|_){ + // [eval] + #S: (_|_){ + // [eval] issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: structural cycle))): 0.n: 0 matched, expected 1))): 0.n: 0 matched, expected 1))): 0 matched, expected 1: + // ./matchn.cue:55:13 + // ./matchn.cue:55:20 + n: (#struct){ + n: (#struct){ + n: (_){ _ } + } + } } } } @@ -259,7 +351,19 @@ Result: diff old new --- old +++ new -@@ -62,20 +62,20 @@ +@@ -1,7 +1,10 @@ + Errors: + issue3443.matchIf.#S: cannot call non-function matchIf (type struct): + ./matchn.cue:16:7 +-issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: structural cycle (and 1 more errors)))): issue3443.cycle2.fail.0.n: 0 matched, expected 1))): 0 matched, expected 1: ++issue3443.cycle1.cycle.s: invalid value {n:{n:_}} (does not satisfy matchN(1, [{n:matchN(1, [{n:#S}])}])): 0 matched, expected 1: ++ ./matchn.cue:35:7 ++ ./matchn.cue:35:14 ++issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: structural cycle))): 0.n: 0 matched, expected 1))): 0.n: 0 matched, expected 1))): 0 matched, expected 1: + ./matchn.cue:55:13 + ./matchn.cue:55:20 + +@@ -65,20 +68,20 @@ issue3410: (struct){ _s: (struct){ #x: (_){ matchN(1, (#list){ @@ -288,7 +392,7 @@ diff old new } }) } } -@@ -82,9 +82,9 @@ +@@ -85,9 +88,9 @@ issue3420: (struct){ matches1: (struct){ #S: (_){ matchN(1, (#list){ @@ -301,7 +405,7 @@ diff old new } }) } s: (int){ 2 } -@@ -101,28 +101,28 @@ +@@ -104,13 +107,13 @@ } noCycle: (struct){ #S: (_){ matchN(1, (#list){ @@ -312,6 +416,13 @@ diff old new + // }] } }) } + noHang: (struct){ +- s: (struct){ ++ s: (#struct){ + n: (struct){ + n: (_){ _ } + } +@@ -119,29 +122,34 @@ } noCycle2: (struct){ #S: (_){ matchN(1, (#list){ @@ -319,30 +430,64 @@ diff old new + 0: (_|_){// &[{ // n?: (int|〈2;#S〉) - // } -+ // }] - } - }) } - } - cycleButOkay: (struct){ - #S: (_){ matchN(1, (#list){ -- 0: (_|_){// { -+ 0: (_|_){// &[{ - // n: 〈2;#S〉 -- // } - } - }) } -- s: (struct){ +- } +- cycle1: (struct){ +- #S: (_){ matchN(1, (#list){ +- 0: (_|_){// { + // }] + } + }) } -+ s: (#struct){ - n: (struct){ - n: (_){ _ } - } ++ } ++ cycle1: (_|_){ ++ // [eval] ++ #S: (_){ matchN(1, (#list){ ++ 0: (_|_){// &[{ + // n: 〈2;#S〉 +- // } ++ // }] + } + }) } + ok: (struct){ + s: (_){ matchN(1, (#list){ +- 0: (_|_){// { ++ 0: (_|_){// &[{ + // n: 〈2;#S〉 +- // } ++ // }] + } + }) } + } +- cycle: (struct){ +- s: (struct){ ++ cycle: (_|_){ ++ // [eval] ++ s: (_|_){ ++ // [eval] issue3443.cycle1.cycle.s: invalid value {n:{n:_}} (does not satisfy matchN(1, [{n:matchN(1, [{n:#S}])}])): 0 matched, expected 1: ++ // ./matchn.cue:35:7 ++ // ./matchn.cue:35:14 + n: (struct){ + n: (_){ _ } + } +@@ -153,7 +161,7 @@ + fail: (_|_){ + // [eval] + #S: (_|_){ +- // [eval] issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: structural cycle (and 1 more errors)))): issue3443.cycle2.fail.0.n: 0 matched, expected 1))): 0 matched, expected 1: ++ // [eval] issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(0.n: structural cycle))): 0.n: 0 matched, expected 1))): 0.n: 0 matched, expected 1))): 0 matched, expected 1: + // ./matchn.cue:55:13 + // ./matchn.cue:55:20 + n: (#struct){ +-- diff/todo/p2 -- +issue3443: Sort out differences in reporting of cycles. -- out/eval -- Errors: issue3443.matchIf.#S: cannot call non-function matchIf (type struct): ./matchn.cue:16:7 +issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: structural cycle (and 1 more errors)))): issue3443.cycle2.fail.0.n: 0 matched, expected 1))): 0 matched, expected 1: + ./matchn.cue:55:13 + ./matchn.cue:55:20 Result: (_|_){ @@ -448,6 +593,13 @@ Result: // } } }) } + noHang: (struct){ + s: (struct){ + n: (struct){ + n: (_){ _ } + } + } + } } noCycle2: (struct){ #S: (_){ matchN(1, (#list){ @@ -457,16 +609,42 @@ Result: } }) } } - cycleButOkay: (struct){ + cycle1: (struct){ #S: (_){ matchN(1, (#list){ 0: (_|_){// { // n: 〈2;#S〉 // } } }) } - s: (struct){ - n: (struct){ - n: (_){ _ } + ok: (struct){ + s: (_){ matchN(1, (#list){ + 0: (_|_){// { + // n: 〈2;#S〉 + // } + } + }) } + } + cycle: (struct){ + s: (struct){ + n: (struct){ + n: (_){ _ } + } + } + } + } + cycle2: (_|_){ + // [eval] + fail: (_|_){ + // [eval] + #S: (_|_){ + // [eval] issue3443.cycle2.fail.#S: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: invalid value {n:{n:{n:_}}} (does not satisfy matchN(1, _|_(issue3443.cycle2.fail.0.n: structural cycle (and 1 more errors)))): issue3443.cycle2.fail.0.n: 0 matched, expected 1))): 0 matched, expected 1: + // ./matchn.cue:55:13 + // ./matchn.cue:55:20 + n: (#struct){ + n: (#struct){ + n: (_){ _ } + } + } } } } @@ -560,6 +738,14 @@ Result: n?: (〈2;#S〉 & (int|{})) }, ]) + noHang: { + s: 〈1;#S〉 + s: { + n: { + n: _ + } + } + } } noCycle2: { #S: matchN(1, [ @@ -568,18 +754,39 @@ Result: }, ]) } - cycleButOkay: { + cycle1: { #S: matchN(1, [ { n: 〈2;#S〉 }, ]) - s: 〈0;#S〉 - s: { - n: { - n: _ + ok: { + s: 〈1;#S〉 + s: _ + } + cycle: { + s: 〈1;#S〉 + s: { + n: { + n: _ + } } } } + cycle2: { + fail: { + #S: (matchN(1, [ + { + n: 〈2;#S〉 + }, + ]) & { + n: { + n: { + n: _ + } + } + }) + } + } } } diff --git a/internal/core/adt/conjunct.go b/internal/core/adt/conjunct.go index 1bf34e1ab33..c1c9e044c95 100644 --- a/internal/core/adt/conjunct.go +++ b/internal/core/adt/conjunct.go @@ -693,7 +693,7 @@ func (n *nodeContext) insertValueConjunct(env *Environment, v Value, id CloseInf k := 0 match := false for _, c := range n.checks { - if y, ok := c.(*BoundValue); ok { + if y, ok := c.x.(*BoundValue); ok { switch z := SimplifyBounds(ctx, n.kind, x, y); { case z == y: match = true @@ -706,15 +706,16 @@ func (n *nodeContext) insertValueConjunct(env *Environment, v Value, id CloseInf } n.checks = n.checks[:k] if !match { - n.checks = append(n.checks, x) + n.checks = append(n.checks, MakeConjunct(env, x, id)) } return } case Validator: // This check serves as simplifier, but also to remove duplicates. + cx := MakeConjunct(env, x, id) for i, y := range n.checks { - if b := SimplifyValidator(ctx, x, y); b != nil { + if b, ok := SimplifyValidator(ctx, cx, y); ok { n.checks[i] = b return } @@ -729,7 +730,7 @@ func (n *nodeContext) insertValueConjunct(env *Environment, v Value, id CloseInf if kind&(ListKind|StructKind) != 0 { id.cc.hasTop = true } - n.checks = append(n.checks, x) + n.checks = append(n.checks, cx) // We use set the type of the validator argument here to ensure that // validation considers the ultimate value of embedded validators, diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go index 812dbb275b3..5443c91de34 100644 --- a/internal/core/adt/context.go +++ b/internal/core/adt/context.go @@ -542,14 +542,23 @@ func (c *OpContext) Lookup(env *Environment, r Resolver) (*Vertex, *Bottom) { // // TODO(errors): return boolean instead: only the caller has enough information // to generate a proper error message. -func (c *OpContext) Validate(check Validator, value Value) *Bottom { +func (c *OpContext) Validate(check Conjunct, value Value) *Bottom { // TODO: use a position stack to push both values. - saved := c.src + + // TODO(evalv3): move to PushConjunct once the migration is complete. + // Using PushConjunct also saves and restores the error, which may be + // impactful, so we want to do this in a separate commit. + // saved := c.PushConjunct(check) + + src := c.src + ci := c.ci c.src = check.Source() + c.ci = check.CloseInfo - err := check.validate(c, value) + err := check.x.(Validator).validate(c, value) - c.src = saved + c.src = src + c.ci = ci return err } diff --git a/internal/core/adt/disjunct2.go b/internal/core/adt/disjunct2.go index 0dc356b64fe..ff414bcba25 100644 --- a/internal/core/adt/disjunct2.go +++ b/internal/core/adt/disjunct2.go @@ -717,7 +717,7 @@ func isEqualNodeValue(x, y *nodeContext) bool { // Assume that checks are added in the same order. for i, c := range x.checks { d := y.checks[i] - if !Equal(x.ctx, c, d, CheckStructural) { + if !Equal(x.ctx, c.x.(Value), d.x.(Value), CheckStructural) { return false } } diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go index 64935b2215f..4b637d7a3e1 100644 --- a/internal/core/adt/eval.go +++ b/internal/core/adt/eval.go @@ -657,7 +657,8 @@ func (n *nodeContext) validateValue(state vertexStatus) { // serious errors and would like to know about all errors anyway. if n.lowerBound != nil { - if b := ctx.Validate(n.lowerBound, v); b != nil { + c := MakeRootConjunct(nil, n.lowerBound) + if b := ctx.Validate(c, v); b != nil { // TODO(errors): make Validate return boolean and generate // optimized conflict message. Also track and inject IDs // to determine origin location.s @@ -669,7 +670,8 @@ func (n *nodeContext) validateValue(state vertexStatus) { } } if n.upperBound != nil { - if b := ctx.Validate(n.upperBound, v); b != nil { + c := MakeRootConjunct(nil, n.upperBound) + if b := ctx.Validate(c, v); b != nil { // TODO(errors): make Validate return boolean and generate // optimized conflict message. Also track and inject IDs // to determine origin location.s @@ -1051,8 +1053,14 @@ type nodeContext struct { vLists []*Vertex exprs []envExpr - checks []Validator // BuiltinValidator, other bound values. - postChecks []envCheck // Check non-monotonic constraints, among other things. + // Checks is a list of conjuncts, as we need to preserve the context in + // which it was evaluated. The conjunct is always a validator (and thus + // a Value). We need to keep track of the CloseInfo, however, to be able + // to catch cycles when evaluating BuiltinValidators. + // TODO: introduce ValueConjunct to get better compile time type checking. + checks []Conjunct + + postChecks []envCheck // Check non-monotonic constraints, among other things. // Disjunction handling disjunctions []envDisjunct @@ -1510,7 +1518,7 @@ func (n *nodeContext) getValidators(state vertexStatus) BaseValue { } for _, c := range n.checks { // Drop !=x if x is out of bounds with another bound. - if b, _ := c.(*BoundValue); b != nil && b.Op == NotEqualOp { + if b, _ := c.x.(*BoundValue); b != nil && b.Op == NotEqualOp { if n.upperBound != nil && SimplifyBounds(ctx, n.kind, n.upperBound, b) != nil { continue @@ -1520,8 +1528,9 @@ func (n *nodeContext) getValidators(state vertexStatus) BaseValue { continue } } - a = append(a, c) - kind &= c.Kind() + v := c.x.(Value) + a = append(a, v) + kind &= v.Kind() } if kind&^n.kind != 0 { @@ -1987,8 +1996,9 @@ func (n *nodeContext) addValueConjunct(env *Environment, v Value, id CloseInfo) // This check serves as simplifier, but also to remove duplicates. k := 0 match := false + cx := MakeConjunct(env, x, id) for _, c := range n.checks { - if y, ok := c.(*BoundValue); ok { + if y, ok := c.x.(*BoundValue); ok { switch z := SimplifyBounds(ctx, n.kind, x, y); { case z == y: match = true @@ -2001,21 +2011,22 @@ func (n *nodeContext) addValueConjunct(env *Environment, v Value, id CloseInfo) } n.checks = n.checks[:k] if !match { - n.checks = append(n.checks, x) + n.checks = append(n.checks, cx) } return } case Validator: // This check serves as simplifier, but also to remove duplicates. + cx := MakeConjunct(env, x, id) for i, y := range n.checks { - if b := SimplifyValidator(ctx, x, y); b != nil { + if b, ok := SimplifyValidator(ctx, cx, y); ok { n.checks[i] = b return } } n.updateNodeType(x.Kind(), x, id) - n.checks = append(n.checks, x) + n.checks = append(n.checks, cx) // TODO(validatorType): see namesake TODO in conjunct.go. k := x.Kind() if k == TopKind { diff --git a/internal/core/adt/simplify.go b/internal/core/adt/simplify.go index da9d95a49ad..5ba9b64e2c5 100644 --- a/internal/core/adt/simplify.go +++ b/internal/core/adt/simplify.go @@ -202,16 +202,16 @@ func test(ctx *OpContext, op Op, a, b Value) bool { // Currently this only checks for pure equality. In the future this can be used // to simplify certain builtin validators analogously to how we simplify bounds // now. -func SimplifyValidator(ctx *OpContext, v, w Validator) Validator { - switch x := v.(type) { +func SimplifyValidator(ctx *OpContext, v, w Conjunct) (c Conjunct, ok bool) { + switch x := v.x.(type) { case *BuiltinValidator: - switch y := w.(type) { + switch y := w.x.(type) { case *BuiltinValidator: if x == y { - return x + return v, true } if x.Builtin != y.Builtin || len(x.Args) != len(y.Args) { - return nil + return c, false } for i, a := range x.Args { b := y.Args[i] @@ -222,11 +222,11 @@ func SimplifyValidator(ctx *OpContext, v, w Validator) Validator { v.Finalize(ctx) } if !Equal(ctx, a, b, CheckStructural) { - return nil + return c, false } } - return x + return v, true } } - return nil + return c, false } diff --git a/internal/core/subsume/value.go b/internal/core/subsume/value.go index cdb5fdfcd80..8edbd6ad8b7 100644 --- a/internal/core/subsume/value.go +++ b/internal/core/subsume/value.go @@ -103,7 +103,9 @@ func (s *subsumer) values(a, b adt.Value) (result bool) { case *adt.BuiltinValidator: state := s.ctx.PushState(s.ctx.Env(0), b.Source()) - b1 := s.ctx.Validate(x, b) + // TODO: is this always correct? + cx := adt.MakeRootConjunct(s.ctx.Env(0), x) + b1 := s.ctx.Validate(cx, b) if b1 != nil { s.errs = errors.Append(s.errs, b1.Err) }