Skip to content

Commit

Permalink
internal/core/adt: impove arcType update logic
Browse files Browse the repository at this point in the history 
This makes closedness checking with arcType updates
more robust:
- it asserts that "allows" is only called on
  closeContexts that are done processing
- it consolidates recursive logic in a single place
  instead of sprinkling it throughout comprehension
  code
- it now actively marks disallowed pending nodes
  as ArcNotPresent
- decisions now mostly rely on the arcType in the
  closeContext only, instead of on both the arc type
  in the Vertex and closeContext.

It also simplifies some of the calling signatures
and cleans up some of the logic in the process.

Fixes #3533

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: I5f2663fa2167ba2a4bcaadcf9a8f8196c0d3600d
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1203000
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
  • Loading branch information
@mpvl
mpvl committed Oct 24, 2024
1 parent 7e82983 commit d28f285
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 163 deletions.
132 changes: 22 additions & 110 deletions cue/testdata/comprehensions/pushdown.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,11 +803,6 @@ embed.fail1.p: field not allowed:
./in.cue:46:7
./in.cue:46:4
./in.cue:49:9
embed.success4.x.y: field not allowed:
./in.cue:76:9
./in.cue:70:7
./in.cue:71:6
./in.cue:76:6
embed.fail4.p: field not allowed:
./in.cue:87:10
./in.cue:87:4
Expand All @@ -817,11 +812,6 @@ fieldMismatch.a: cannot combine regular field "x" with 2:
structShare.err1.x.d.e: field not allowed:
./in.cue:591:12
./in.cue:591:9
issue3533.out.metadata.namespace: field not allowed:
./issue3533.cue:7:26
./issue3533.cue:2:15
./issue3533.cue:7:15
./issue3533.cue:11:17

Result:
(_|_){
Expand Down Expand Up @@ -890,20 +880,12 @@ Result:
}
#C3: (#struct){
}
success4: (_|_){
// [eval]
success4: (struct){
#X: (#struct){
y: (string){ string }
}
x: (_|_){
// [eval]
y: (_|_){
// [eval] embed.success4.x.y: field not allowed:
// ./in.cue:76:9
// ./in.cue:70:7
// ./in.cue:71:6
// ./in.cue:76:6
}
x: (#struct){
y: (string){ string }
}
}
fail4: (_|_){
Expand Down Expand Up @@ -1432,22 +1414,13 @@ Result:
root: (#struct){
}
}
issue3533: (_|_){
// [eval]
issue3533: (struct){
#ObjectMeta: (#struct){
namespace?: (string){ string }
}
out: (_|_){
// [eval]
metadata: (_|_){
// [eval]
namespace: (_|_){
// [eval] issue3533.out.metadata.namespace: field not allowed:
// ./issue3533.cue:7:26
// ./issue3533.cue:2:15
// ./issue3533.cue:7:15
// ./issue3533.cue:11:17
}
out: (struct){
metadata: (#struct){
namespace: (string){ |(*(string){ "default" }, (string){ string }) }
}
}
}
Expand Down Expand Up @@ -1475,7 +1448,7 @@ Result:
diff old new
--- old
+++ new
@@ -1,32 +1,29 @@
@@ -1,32 +1,19 @@
Errors:
+noStackOverflowStructCycle.#list.tail: structural cycle
+noStackOverflowStructCycle.list.tail: structural cycle
Expand All @@ -1487,11 +1460,6 @@ diff old new
+ ./in.cue:46:7
./in.cue:46:4
./in.cue:49:9
+embed.success4.x.y: field not allowed:
+ ./in.cue:76:9
+ ./in.cue:70:7
+ ./in.cue:71:6
+ ./in.cue:76:6
embed.fail4.p: field not allowed:
- ./in.cue:82:9
- ./in.cue:83:7
Expand All @@ -1517,15 +1485,10 @@ diff old new
+structShare.err1.x.d.e: field not allowed:
+ ./in.cue:591:12
+ ./in.cue:591:9
+issue3533.out.metadata.namespace: field not allowed:
+ ./issue3533.cue:7:26
+ ./issue3533.cue:2:15
+ ./issue3533.cue:7:15
+ ./issue3533.cue:11:17

Result:
(_|_){
@@ -68,8 +65,8 @@
@@ -68,8 +55,8 @@
}
fail: (struct){
a: (_|_){
Expand All @@ -1536,7 +1499,7 @@ diff old new
}
}
embed: (_|_){
@@ -78,10 +75,7 @@
@@ -78,10 +65,7 @@
// [eval]
p: (_|_){
// [eval] embed.fail1.p: field not allowed:
Expand All @@ -1548,31 +1511,7 @@ diff old new
// ./in.cue:46:4
// ./in.cue:49:9
}
@@ -98,12 +92,20 @@
}
#C3: (#struct){
}
- success4: (struct){
+ success4: (_|_){
+ // [eval]
#X: (#struct){
y: (string){ string }
}
- x: (#struct){
- y: (string){ string }
+ x: (_|_){
+ // [eval]
+ y: (_|_){
+ // [eval] embed.success4.x.y: field not allowed:
+ // ./in.cue:76:9
+ // ./in.cue:70:7
+ // ./in.cue:71:6
+ // ./in.cue:76:6
+ }
}
}
fail4: (_|_){
@@ -110,10 +112,7 @@
@@ -110,10 +94,7 @@
// [eval]
p: (_|_){
// [eval] embed.fail4.p: field not allowed:
Expand All @@ -1584,7 +1523,7 @@ diff old new
// ./in.cue:87:4
q: (int){ 1 }
}
@@ -187,8 +186,7 @@
@@ -187,8 +168,7 @@
// [structural cycle] noStackOverflowStructCycle.list.tail: structural cycle
}
}
Expand All @@ -1594,7 +1533,7 @@ diff old new
t1: (struct){
#a: (_|_){
// [incomplete] provideIncompleteSuccess.t1.#a: incomplete bool: bool:
@@ -196,16 +194,12 @@
@@ -196,16 +176,12 @@
b: (bool){ bool }
}
x: (#struct){
Expand All @@ -1617,7 +1556,7 @@ diff old new
#a: (#struct){
c: (int){ 4 }
b: (bool){ true }
@@ -212,17 +206,8 @@
@@ -212,17 +188,8 @@
}
#c: (#struct){
}
Expand All @@ -1637,7 +1576,7 @@ diff old new
}
b: (bool){ true }
}
@@ -252,9 +237,22 @@
@@ -252,9 +219,22 @@
}
cyclicError: (struct){
a: (_|_){
Expand All @@ -1663,7 +1602,7 @@ diff old new
}
}
midwayReferences: (struct){
@@ -268,24 +266,9 @@
@@ -268,24 +248,9 @@
}
}
}
Expand Down Expand Up @@ -1691,7 +1630,7 @@ diff old new
}
closedCheck: (struct){
success1: (struct){
@@ -388,13 +371,7 @@
@@ -388,13 +353,7 @@
}
}
}
Expand All @@ -1706,7 +1645,7 @@ diff old new
#F: (#struct){
e: (bool){ bool }
f: (_|_){
@@ -411,17 +388,10 @@
@@ -411,17 +370,10 @@
}
}
E: (_|_){
Expand All @@ -1725,7 +1664,7 @@ diff old new
}
}
derefDisj2: (struct){
@@ -432,17 +402,10 @@
@@ -432,17 +384,10 @@
}
}
E: (_|_){
Expand All @@ -1744,7 +1683,7 @@ diff old new
}
}
bulk1: (struct){
@@ -565,9 +528,7 @@
@@ -565,9 +510,7 @@
// [eval]
e: (_|_){
// [eval] structShare.err1.x.d.e: field not allowed:
Expand All @@ -1755,7 +1694,7 @@ diff old new
// ./in.cue:591:9
}
}
@@ -591,13 +552,13 @@
@@ -591,13 +534,13 @@
}
envs: (struct){
e1: (#struct){
Expand All @@ -1773,7 +1712,7 @@ diff old new
}
}
}
@@ -634,9 +595,8 @@
@@ -634,9 +577,8 @@
_c: (struct){
y: (int){ 1 }
}
Expand All @@ -1785,33 +1724,6 @@ diff old new
}
}
errorPropagation: (_|_){
@@ -674,13 +634,22 @@
root: (#struct){
}
}
- issue3533: (struct){
+ issue3533: (_|_){
+ // [eval]
#ObjectMeta: (#struct){
namespace?: (string){ string }
}
- out: (struct){
- metadata: (#struct){
- namespace: (string){ |(*(string){ "default" }, (string){ string }) }
+ out: (_|_){
+ // [eval]
+ metadata: (_|_){
+ // [eval]
+ namespace: (_|_){
+ // [eval] issue3533.out.metadata.namespace: field not allowed:
+ // ./issue3533.cue:7:26
+ // ./issue3533.cue:2:15
+ // ./issue3533.cue:7:15
+ // ./issue3533.cue:11:17
+ }
}
}
}
-- diff/-out/evalalpha/stats<==>+out/eval/stats --
diff old new
--- old
Expand Down
4 changes: 2 additions & 2 deletions cue/testdata/cycle/issue990.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ Allocs: 1392
Retain: 0

Unifications: 183
Conjuncts: 4545
Conjuncts: 4543
Disjuncts: 284
-- out/evalalpha --
Errors:
Expand Down Expand Up @@ -268,7 +268,7 @@ diff old new
-Conjuncts: 12017
-Disjuncts: 3244
+Unifications: 183
+Conjuncts: 4545
+Conjuncts: 4543
+Disjuncts: 284
-- diff/-out/evalalpha<==>+out/eval --
diff old new
Expand Down
24 changes: 7 additions & 17 deletions internal/core/adt/comprehension.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,30 +463,20 @@ func (n *nodeContext) processComprehensionInner(d *envYield, state vertexStatus)
d.inserted = true

if len(d.envs) == 0 {
c := d.leaf
for p := c.arcCC; p != nil; p = p.parent {
// because the parent referrer will reach a zero count before this
// node will reach a zero count, we need to propagate the arcType.
p.updateArcType(ArcNotPresent)
}
c := d.leaf.arcCC
// because the parent referrer will reach a zero count before this
// node will reach a zero count, we need to propagate the arcType.
c.updateArcType(ctx, ArcNotPresent)
return nil
}

v := n.node
f := v.Label
for c := d.leaf; c.parent != nil; c = c.parent {
// because the parent referrer will reach a zero count before this
// node will reach a zero count, we need to propagate the arcType.
for arc, p := c.arcCC, c.cc; p != nil; arc, p = arc.parent, p.parent {

t := arc.arcType
if p.isClosed && t >= ArcPending && !p.allows(ctx, f, arc) {
ctx.notAllowedError(p.src, arc.src)
}
// TODO: remove this line once we use the arcType of the
// closeContext in notAllowedError.
arc.src.updateArcType(c.arcType)
arc.updateArcType(c.arcType)
if p := c.arcCC; p != nil {
p.src.updateArcType(c.arcType)
p.updateArcType(ctx, c.arcType)
}
v.updateArcType(c.arcType)
if v.ArcType == ArcNotPresent {
Expand Down
6 changes: 5 additions & 1 deletion internal/core/adt/conjunct.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,14 @@ loop2:
case *Comprehension, Expr:
// No need to increment and decrement, as there will be at least
// one entry.
if _, ok := s.Src.(*ast.File); !ok {
if _, ok := s.Src.(*ast.File); !ok && s.Src != nil {
// If this is not a file, the struct indicates the scope/
// boundary at which closedness should apply. This is not true
// for files.
// We should also not spawn if this is a nested Comprehension,
// where the spawn is already done as it may lead to spurious
// field not allowed errors. We can detect this with a nil s.Src.
// TODO(evalv3): use a more principled detection mechanism.
// TODO: set this as a flag in StructLit so as to not have to
// do the somewhat dangerous cast here.
ci, _ = ci.spawnCloseContext(n.ctx, 0)
Expand Down
Loading

0 comments on commit d28f285

Please sign in to comment.