Skip to content

Commit

Permalink
internal/core/(adt|subsume): pattern constraints are optional
Browse files Browse the repository at this point in the history 
Currently, the new evaluator adds pattern constraints as
member fields, whereas they are equivalent to optional
constraints according to the spec. This results in subsumption
being incorrect.

Similarly, there are various points where constraints are
collected where the resulting Vertex must be marked as
optional. We now use a wrapper matchAndInsert in the
subsumption package to handle these cases. We could not
set ArcOptional unconditionally in MatchAndInsert, as this
is not always correct.

Another issue was that a pattern constraint of the form
["foo"]: value is equivalent to foo?: value. The code
now catches these "bounded" cases of constraints and
treats them as optional fields within subsumption. Note that
there remains an element of approximation. This is fine.

Tests in value_test were added to check the subsumption
behavior reguarding pattern constraints.

Note that subsumption was much less precise for the old
evaluator. For this reason the tests are sometimes
disabled for the old evaluator.

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I223d0dfc4b0893c1bbf08b1ef9ad2dc651cf9cc8
Dispatch-Trailer: {"type":"trybot","CL":1196978,"patchset":1,"ref":"refs/changes/78/1196978/1","targetBranch":"master"}
  • Loading branch information
@mpvl @cueckoo
mpvl authored and cueckoo committed Jul 2, 2024
1 parent 5264a9c commit 1fd209e
Show file tree
Hide file tree
Showing 7 changed files with 147 additions and 92 deletions.
106 changes: 51 additions & 55 deletions cue/testdata/comprehensions/issue837.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ _params.hsize.$_instances: conflicting values 1 and {$_instances:(>=0|*1)} (mism
_params.hsize.$_instances: conflicting values >=0 and {$_instances:(>=0|*1)} (mismatched types number and struct):
./in.cue:55:12
./in.cue:56:15
#DoDeploy.deployment.description.dep.service.description: undefined field: service:
./in.cue:5:28
_params.hsize.$_instances.$_instances: field not allowed:
./in.cue:33:20
./in.cue:56:2
Expand Down Expand Up @@ -176,26 +174,37 @@ Result:
// [eval]
description: (_|_){
// [eval]
let dep#1 = (_|_){
// [eval]
service: (_|_){
// [eval]
let dep#1 = (#struct){
service: (#struct){
ref: (#struct){
kind: (string){ "service" }
}
description: (_|_){
// [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
// [incomplete] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
// ./in.cue:5:28
}
}
hsize: (_|_){
// [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
// [incomplete] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
// ./in.cue:5:28
}
}
service: (_|_){
// [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
// ./in.cue:5:28
// [eval] _params.hsize.$_instances: conflicting values 1 and {$_instances:(>=0|*1)} (mismatched types int and struct):
// ./in.cue:55:12
// ./in.cue:56:22
// _params.hsize.$_instances: conflicting values >=0 and {$_instances:(>=0|*1)} (mismatched types number and struct):
// ./in.cue:55:12
// ./in.cue:56:15
// _params.hsize.$_instances.$_instances: field not allowed:
// ./in.cue:33:20
// ./in.cue:56:2
// _params.hsize.$_instances: invalid operands {$_instances:_|_(_params.hsize.$_instances.$_instances: field not allowed)} and 0 to '>=' (type _|_ and int):
// ./in.cue:56:15
// ./in.cue:56:17
// _params.hsize.$_instances.$_instances: field not allowed:
// ./in.cue:56:15
// ./in.cue:56:2
}
}
}
Expand Down Expand Up @@ -294,7 +303,7 @@ Result:
diff old new
--- old
+++ new
@@ -1,10 +1,23 @@
@@ -1,10 +1,21 @@
Errors:
-#Configure.service.description.role: undefined field: role:
- ./in.cue:40:19
Expand All @@ -306,8 +315,6 @@ diff old new
+_params.hsize.$_instances: conflicting values >=0 and {$_instances:(>=0|*1)} (mismatched types number and struct):
+ ./in.cue:55:12
+ ./in.cue:56:15
+#DoDeploy.deployment.description.dep.service.description: undefined field: service:
+ ./in.cue:5:28
+_params.hsize.$_instances.$_instances: field not allowed:
+ ./in.cue:33:20
+ ./in.cue:56:2
Expand All @@ -322,7 +329,7 @@ diff old new

Result:
(_|_){
@@ -19,11 +32,13 @@
@@ -19,11 +30,13 @@
kind: (string){ "service" }
}
description: (_|_){
Expand All @@ -341,7 +348,7 @@ diff old new
}
}
}
@@ -47,11 +62,13 @@
@@ -47,11 +60,13 @@
kind: (string){ "service" }
}
description: (_|_){
Expand All @@ -360,7 +367,7 @@ diff old new
}
}
}
@@ -68,11 +85,13 @@
@@ -68,11 +83,13 @@
kind: (string){ "service" }
}
description: (_|_){
Expand All @@ -379,30 +386,20 @@ diff old new
}
}
}
@@ -81,35 +100,26 @@
// [eval]
description: (_|_){
// [eval]
- let dep#1 = (#struct){
- service: (#struct){
- ref: (#struct){
- kind: (string){ "service" }
- }
- description: (_|_){
- // [incomplete] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
- // ./in.cue:5:28
- }
- }
@@ -91,25 +108,27 @@
// ./in.cue:5:28
}
}
- hsize: (#struct){
- }
- }
- service: (_|_){
+ hsize: (_|_){
+ // [incomplete] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
+ // ./in.cue:5:28
}
}
service: (_|_){
- // [eval]
- description: (_|_){
+ let dep#1 = (_|_){
+ // [eval]
+ service: (_|_){
// [eval]
- // [eval]
- let configed#2 = (_|_){
- // [eval]
- labstr: (_|_){
Expand All @@ -415,26 +412,25 @@ diff old new
- // ./in.cue:40:19
- }
- }
+ ref: (#struct){
+ kind: (string){ "service" }
+ }
+ description: (_|_){
+ // [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
+ // ./in.cue:5:28
+ }
+ }
+ hsize: (_|_){
+ // [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
+ // ./in.cue:5:28
+ }
+ }
+ service: (_|_){
+ // [eval] #DoDeploy.deployment.description.dep.service.description: undefined field: service:
+ // ./in.cue:5:28
+ // [eval] _params.hsize.$_instances: conflicting values 1 and {$_instances:(>=0|*1)} (mismatched types int and struct):
+ // ./in.cue:55:12
+ // ./in.cue:56:22
+ // _params.hsize.$_instances: conflicting values >=0 and {$_instances:(>=0|*1)} (mismatched types number and struct):
+ // ./in.cue:55:12
+ // ./in.cue:56:15
+ // _params.hsize.$_instances.$_instances: field not allowed:
+ // ./in.cue:33:20
+ // ./in.cue:56:2
+ // _params.hsize.$_instances: invalid operands {$_instances:_|_(_params.hsize.$_instances.$_instances: field not allowed)} and 0 to '>=' (type _|_ and int):
+ // ./in.cue:56:15
+ // ./in.cue:56:17
+ // _params.hsize.$_instances.$_instances: field not allowed:
+ // ./in.cue:56:15
+ // ./in.cue:56:2
}
}
}
@@ -125,15 +135,38 @@
@@ -125,15 +144,38 @@
description: (_|_){
// [eval]
let configed#2 = (_|_){
Expand Down Expand Up @@ -480,7 +476,7 @@ diff old new
}
}
}
@@ -162,11 +195,13 @@
@@ -162,11 +204,13 @@
kind: (string){ "service" }
}
description: (_|_){
Expand Down
2 changes: 1 addition & 1 deletion internal/core/adt/composite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ func (v *Vertex) MatchAndInsert(ctx *OpContext, arc *Vertex) {
c.Env = &env

root := arc.rootCloseContext(ctx)
root.insertConjunct(ctx, root, c, c.CloseInfo, ArcMember, true, false)
root.insertConjunct(ctx, root, c, c.CloseInfo, ArcOptional, true, false)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/core/adt/constraints.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (n *nodeContext) insertConstraint(pattern Value, c Conjunct) bool {
}

if constraint == nil {
constraint = &Vertex{}
constraint = &Vertex{ArcType: ArcOptional}
pcs.Pairs = append(pcs.Pairs, PatternConstraint{
Pattern: pattern,
Constraint: constraint,
Expand Down
4 changes: 4 additions & 0 deletions internal/core/adt/fields.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,10 @@ func (c *closeContext) decDependent(ctx *OpContext, kind depKind, dependant *clo
// panic("unexpected allowed set")
}
pcs.Allowed = c.Expr
// Handle special case.
if pcs.Allowed == nil && c.isTotal {
pcs.Allowed = &Top{}
}
return
}
return
Expand Down
8 changes: 5 additions & 3 deletions internal/core/adt/fields_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func TestCloseContext(t *testing.T) {
string: {1 1}`,

// Should be empty or string only, as all fields match.
allowed: "",
allowed: "_",
}, {
name: "multi patterns",
run: func(x *adt.FieldTester) {
Expand Down Expand Up @@ -196,7 +196,7 @@ func TestCloseContext(t *testing.T) {
},
arcs: `a: {"foo" string}`,
patterns: `string: {string}`,
allowed: "", // all fields
allowed: "_", // all fields
}, {
name: "pattern defined before matching field",
run: func(x *adt.FieldTester) {
Expand All @@ -207,7 +207,7 @@ func TestCloseContext(t *testing.T) {
},
arcs: `a: {"foo" string}`,
patterns: `string: {string}`,
allowed: "", // all fields
allowed: "_", // all fields
}, {
name: "shared on one level",
run: func(x *adt.FieldTester) {
Expand All @@ -227,6 +227,7 @@ func TestCloseContext(t *testing.T) {
string: {1 2}
>"a": {shared}
>"b": {shared}`,
allowed: "_",
}, {
// The same conjunct in different groups could result in the different
// closedness rules. Hence they should not be shared.
Expand Down Expand Up @@ -295,6 +296,7 @@ func TestCloseContext(t *testing.T) {
c: {"bar" 2 3 "baz"}`,

patterns: `string: {2 3}`,
allowed: "_",
}, {
name: "conjunction in embedding",
run: func(x *adt.FieldTester) {
Expand Down
28 changes: 19 additions & 9 deletions internal/core/subsume/value_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,20 +332,30 @@ func TestValues(t *testing.T) {
},
438: {subsumes: true, in: `a: {}, b: {[_]: 6}`},

439: {subsumes: true, in: `a: close({[string]: 1}), b: close({foo?: 1})`, skip_v2: true},
440: {subsumes: false, in: `a: {[string]: 1}, b: foo?: 1`},
441: {subsumes: true, in: `a: #d: {[string]: 1}, b: #d: foo?: 1`, skip_v2: true},
442: {subsumes: true, in: `a: {["foo"]: 1}, b: foo?: 1`, skip_v2: true},
443: {subsumes: true, in: `a: {["foo"]: 1}, b: {["foo"]: 1}`, skip_v2: true},
444: {subsumes: true, in: `a: {["foo"|"bar"]: 1}, b: {foo?: 1, bar?: 1}`, skip_v2: true},
445: {subsumes: true, in: `a: {["foo"|"bar"]: 1}, b: {["foo"|"bar"]: 1}`, skip_v2: true},
446: {subsumes: true, in: `a: {["foo"|"bar"]: 1}, b: {["foo"]: 1, ["bar"]: 1}`, skip_v2: true},
447: {subsumes: true, in: `a: {["foo"]: 1, ["foo"|"bar"]: 1}, b: {["foo"]: 1, ["bar"]: 1}`, skip_v2: true},

// TODO: the subNoOptional mode used to be used by the equality check.
// Now this has its own implementation it is no longer necessary. Keep
// around for now in case we still need the more permissive equality
// check that can be created by using subsumption.
//
// 440: {subsumes: true, in: `a: {foo?: 1}, b: {}`, mode: subNoOptional},
// 441: {subsumes: true, in: `a: {}, b: {foo?: 1}`, mode: subNoOptional},
// 442: {subsumes: true, in: `a: {foo?: 1}, b: {foo: 1}`, mode: subNoOptional},
// 443: {subsumes: true, in: `a: {foo?: 1}, b: {foo?: 1}`, mode: subNoOptional},
// 444: {subsumes: false, in: `a: {foo: 1}, b: {foo?: 1}`, mode: subNoOptional},
// 445: {subsumes: true, in: `a: close({}), b: {foo?: 1}`, mode: subNoOptional},
// 446: {subsumes: true, in: `a: close({}), b: close({foo?: 1})`, mode: subNoOptional},
// 447: {subsumes: true, in: `a: {}, b: close({})`, mode: subNoOptional},
// 448: {subsumes: true, in: `a: {}, b: close({foo?: 1})`, mode: subNoOptional},
// 450: {subsumes: true, in: `a: {foo?: 1}, b: {}`, mode: subNoOptional},
// 451: {subsumes: true, in: `a: {}, b: {foo?: 1}`, mode: subNoOptional},
// 452: {subsumes: true, in: `a: {foo?: 1}, b: {foo: 1}`, mode: subNoOptional},
// 453: {subsumes: true, in: `a: {foo?: 1}, b: {foo?: 1}`, mode: subNoOptional},
// 454: {subsumes: false, in: `a: {foo: 1}, b: {foo?: 1}`, mode: subNoOptional},
// 455: {subsumes: true, in: `a: close({}), b: {foo?: 1}`, mode: subNoOptional},
// 456: {subsumes: true, in: `a: close({}), b: close({foo?: 1})`, mode: subNoOptional},
// 457: {subsumes: true, in: `a: {}, b: close({})`, mode: subNoOptional},
// 458: {subsumes: true, in: `a: {}, b: close({foo?: 1})`, mode: subNoOptional},

// embedded scalars
460: {subsumes: true, in: `a: {1, #foo: number}, b: {1, #foo: 1}`},
Expand Down
Loading

0 comments on commit 1fd209e

Please sign in to comment.