From 710b438a0a57aacee52da24d97014e36d8e4d4e7 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Thu, 22 Aug 2024 16:16:48 +0200 Subject: [PATCH] internal/core/adt: allow referencing required fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is intended to be a temporary measure to help the transition of JSON schema to use ! for required properties. Currently, CUE code that relied on the JSON schema generator to generate regular fields for required fields could just reference those fields. This code is currently broken by this transition. In practice, however, there does not seem to be much harm in allowing reference to refer to required fields. CUE allows errors to be elided if another error is guaranteed to be given otherwise. This is not always the case, but but it seems that this is fine for any practical purpose. Example where this is fully okay: #Foo: { a!: 1 b: a } // c currently has errors for c.a (required field missing) and // c.b (referencing required field). With this change, the // second error is removed. But since there is still an error // for the underlying problem (a missing required field), this // is okay. c: #Foo If, on the other hand, we would write: c: #Foo.a then `c` will not produce an error where before this change it would. Note that, for export at least, this is only true if a required field is used in combination with a fully concrete value. We also think this solution does not complicate finding a more principled solution later. If the JSON Schema change is left in, users will be forced to adjust their code as is. To facilitate such transition, we would have to write a `cue fix`. This same mechansim could later be used to move code that references required fields to using some alternative. This CL allows for a smoother transition either way. Note that there was a discrepancy in the semantics between v2 and v3 that has been removed by this change. Signed-off-by: Marcel van Lohuizen Change-Id: I6b79bcc858ea7c2a76418b58e6ef22839c558df2 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199898 TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine Reviewed-by: Daniel Martí --- cue/testdata/eval/required.txtar | 86 +++++++------------------------- internal/core/adt/context.go | 4 +- internal/core/adt/unify.go | 13 ++++- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/cue/testdata/eval/required.txtar b/cue/testdata/eval/required.txtar index 49c471929a5..d482a6789b2 100644 --- a/cue/testdata/eval/required.txtar +++ b/cue/testdata/eval/required.txtar @@ -207,11 +207,11 @@ Leaks: 0 Freed: 66 Reused: 58 Allocs: 8 -Retain: 13 +Retain: 10 Unifications: 66 -Conjuncts: 92 -Disjuncts: 79 +Conjuncts: 90 +Disjuncts: 76 -- out/evalalpha -- (_|_){ // [eval] @@ -269,16 +269,10 @@ Disjuncts: 79 #Foo: (#struct){ a!: (int){ int } } - b: (_|_){ - // [incomplete] reference.toWithinDefinition.p1.b: cannot reference optional field: a: - // ./in.cue:43:11 - } + b: (int){ int } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toWithinDefinition.p2.b: cannot reference optional field: a: - // ./in.cue:46:11 - } + b: (int){ int } #Foo: (#struct){ a!: (int){ int } } @@ -292,16 +286,10 @@ Disjuncts: 79 b: (#struct){ a!: (int){ int } } - c: (_|_){ - // [incomplete] reference.toFieldFromDefinition.p1.c: cannot reference optional field: a: - // ./in.cue:52:8 - } + c: (int){ int } } p2: (struct){ - c: (_|_){ - // [incomplete] reference.toFieldFromDefinition.p2.c: cannot reference optional field: a: - // ./in.cue:55:8 - } + c: (int){ int } b: (#struct){ a!: (int){ int } } @@ -315,16 +303,10 @@ Disjuncts: 79 x: (struct){ y!: (_){ _ } } - b: (_|_){ - // [incomplete] reference.toNonDefinition.p1.b: cannot reference optional field: y: - // ./in.cue:61:8 - } + b: (_){ _ } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toNonDefinition.p2.b: cannot reference optional field: y: - // ./in.cue:64:8 - } + b: (_){ _ } x: (struct){ y!: (_){ _ } } @@ -333,16 +315,10 @@ Disjuncts: 79 toConcrete: (struct){ p1: (struct){ a!: (int){ 1 } - b: (_|_){ - // [incomplete] reference.toConcrete.p1.b: cannot reference optional field: a: - // ./in.cue:69:6 - } + b: (int){ 2 } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toConcrete.p2.b: cannot reference optional field: a: - // ./in.cue:72:6 - } + b: (int){ 2 } a!: (int){ 1 } } } @@ -422,16 +398,10 @@ Missing position. #Foo: (#struct){ a!: (int){ int } } - b: (_|_){ - // [incomplete] reference.toWithinDefinition.p1.b: cannot reference optional field: a: - // ./in.cue:43:11 - } + b: (int){ int } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toWithinDefinition.p2.b: cannot reference optional field: a: - // ./in.cue:46:11 - } + b: (int){ int } #Foo: (#struct){ a!: (int){ int } } @@ -445,16 +415,10 @@ Missing position. b: (#struct){ a!: (int){ int } } - c: (_|_){ - // [incomplete] reference.toFieldFromDefinition.p1.c: cannot reference optional field: a: - // ./in.cue:52:8 - } + c: (int){ int } } p2: (struct){ - c: (_|_){ - // [incomplete] reference.toFieldFromDefinition.p2.c: cannot reference optional field: a: - // ./in.cue:55:8 - } + c: (int){ int } b: (#struct){ a!: (int){ int } } @@ -468,16 +432,10 @@ Missing position. x: (struct){ y!: (_){ _ } } - b: (_|_){ - // [incomplete] reference.toNonDefinition.p1.b: cannot reference optional field: y: - // ./in.cue:61:8 - } + b: (_){ _ } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toNonDefinition.p2.b: cannot reference optional field: y: - // ./in.cue:64:8 - } + b: (_){ _ } x: (struct){ y!: (_){ _ } } @@ -486,16 +444,10 @@ Missing position. toConcrete: (struct){ p1: (struct){ a!: (int){ 1 } - b: (_|_){ - // [incomplete] reference.toConcrete.p1.b: cannot reference optional field: a: - // ./in.cue:69:6 - } + b: (int){ 2 } } p2: (struct){ - b: (_|_){ - // [incomplete] reference.toConcrete.p2.b: cannot reference optional field: a: - // ./in.cue:72:6 - } + b: (int){ 2 } a!: (int){ 1 } } } diff --git a/internal/core/adt/context.go b/internal/core/adt/context.go index 4358a2e70f4..1e5b0b4f161 100644 --- a/internal/core/adt/context.go +++ b/internal/core/adt/context.go @@ -948,7 +948,9 @@ func (c *OpContext) lookup(x *Vertex, pos token.Pos, l Feature, flags combinedFl c.unify(a, deprecated(c, partial)) } - if a.IsConstraint() { + // TODO(refRequired): see comment in unify.go:Vertex.lookup near the + // namesake TODO. + if a.ArcType == ArcOptional { code := IncompleteError if hasCycle { code = CycleError diff --git a/internal/core/adt/unify.go b/internal/core/adt/unify.go index 5045b18a2a6..65bdd21630e 100644 --- a/internal/core/adt/unify.go +++ b/internal/core/adt/unify.go @@ -727,10 +727,19 @@ func (v *Vertex) lookup(c *OpContext, pos token.Pos, f Feature, flags combinedFl handleArcType: switch arc.ArcType { - case ArcMember: + case ArcMember, ArcRequired: return arcReturn - case ArcOptional, ArcRequired: + case ArcOptional: + // Technically, this failure also applies to required fields. We assume + // however, that if a reference field that is made regular will already + // result in an error, so that piling up another error is not strictly + // necessary. Note that the spec allows for eliding an error if it is + // guaranteed another error is generated elsewhere. This does not + // properly cover the case where a reference is made directly within the + // definition, but this is fine for the purpose it serves. + // TODO(refRequired): revisit whether referencing required fields should + // fail. label := f.SelectorString(c.Runtime) b := &Bottom{ Code: IncompleteError,