Skip to content

Commit

Permalink
internal/core/eval: fix Environment linkage for Disjunctions
Browse files Browse the repository at this point in the history
The Vertex value for a disjunction is copied into another value
causing the Vertex links in the Environment to point to the wrong
node. This causes args to be missed when resolving.

Ideally the node should not be copied, but as we don't dereferenece,
the pointer value needs to be exactly what it is now.

A better solution perhaps would be to implement dereferencing.

Note that this bug is only exposed through API usage, as during normal
evaluation only DisjunctionExprs will be used in computation.

This will break OpenAPI if not done correctly.

Change-Id: I6a780dc11c7f44ca9a47c4838ea2502371bf7505
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/6647
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
  • Loading branch information
mpvl committed Jul 23, 2020
1 parent 3acdde8 commit b68adfe
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 2 deletions.
2 changes: 2 additions & 0 deletions internal/core/eval/closed.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ func (n *nodeContext) addOr(parentID uint32, c *CloseDef) { // used in eval.go
// "and" semantics of conjunctions. It generates an error if a field is not
// allowed.
func (n *acceptor) verifyArcAllowed(ctx *adt.OpContext, f adt.Feature) *adt.Bottom {
// TODO(perf): this will also generate a message for f == 0. Do something
// more clever and only generate this when it is a user error.
filter := f.IsString() || f == adt.InvalidLabel
if filter && !n.verifyArcRecursive(ctx, n.tree, f) {
label := f.SelectorString(ctx)
Expand Down
28 changes: 26 additions & 2 deletions internal/core/eval/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,15 @@ func (e *Evaluator) Evaluate(c *adt.OpContext, v *adt.Vertex) adt.Value {
// information than incorrect information.
for _, d := range d.Values {
d.Conjuncts = nil
for _, a := range d.Arcs {
for _, x := range a.Conjuncts {
// All the environments for embedded structs need to be
// dereferenced.
for env := x.Env; env != nil && env.Vertex == v; env = env.Up {
env.Vertex = d
}
}
}
}
}

Expand Down Expand Up @@ -251,13 +260,28 @@ func (e *Evaluator) Unify(c *adt.OpContext, v *adt.Vertex, state adt.VertexStatu

case d != nil && len(d.Values) > 0:
v.Value = d
v.Arcs = nil
v.Structs = nil
// The conjuncts will have too much information. Better have no
// information than incorrect information.
for _, d := range d.Values {
// We clear the conjuncts for now. As these disjuncts are for API
// use only, we will fill them out when necessary (using Defaults).
d.Conjuncts = nil

// TODO: use a more principled form of dereferencing. For instance,
// disjuncts could already be assumed to be the given Vertex, and
// the the main vertex could be dereferenced during evaluation.
for _, a := range d.Arcs {
for _, x := range a.Conjuncts {
// All the environments for embedded structs need to be
// dereferenced.
for env := x.Env; env != nil && env.Vertex == v; env = env.Up {
env.Vertex = d
}
}
}
}
v.Arcs = nil
// v.Structs = nil // TODO: should we keep or discard the Structs?

default:
if r := n.result(); r.Value != nil {
Expand Down

0 comments on commit b68adfe

Please sign in to comment.