From 08e814f8e10192d5443991ca8f13b2e4de02b837 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Tue, 19 Jan 2021 13:43:59 +0100 Subject: [PATCH] internal/core/adt: add support info for trim This change adds different span types to track in a closeInfo struct. A SpanType keeps track how nodes were introduced, for instance by comprehension, definition, or constraint. The added closeInfo nodes also improve error messages. Previously, only referred structs were adding position information, but indirectly added structs, for instance those added by a constraint, did not result in additional position information. The information introduced here is intended for trim, but could very well be useful for other algorithms. It is kept as a separate CL, mostly to allow tracking down performance issues, if such were to arise. So far the trim refactoring seems to improve performance, though, because some of the hot paths have been improved. Change-Id: I656bad2ced723a1bc54971efbd130711c47a75be Reviewed-on: https://cue-review.googlesource.com/c/cue/+/8237 Reviewed-by: CUE cueckoo Reviewed-by: Paul Jolly Reviewed-by: Marcel van Lohuizen --- cmd/cue/cmd/testdata/script/vet_file.txt | 2 + cue/testdata/benchmarks/dedupelem.txtar | 4 +- cue/testdata/comprehensions/issue293.txtar | 2 + cue/testdata/definitions/033_Issue_#153.txtar | 2 + .../037_closing_with_comprehensions.txtar | 4 ++ ...over_closedness_to_enclosed_template.txtar | 3 ++ internal/core/adt/closed.go | 30 +++++++++++---- internal/core/adt/eval.go | 6 ++- internal/core/adt/expr.go | 1 + internal/core/adt/optional.go | 38 +++++++++++-------- 10 files changed, 66 insertions(+), 26 deletions(-) diff --git a/cmd/cue/cmd/testdata/script/vet_file.txt b/cmd/cue/cmd/testdata/script/vet_file.txt index 37976814b..d5e5fcd79 100644 --- a/cmd/cue/cmd/testdata/script/vet_file.txt +++ b/cmd/cue/cmd/testdata/script/vet_file.txt @@ -8,11 +8,13 @@ cmp stderr expect-stderr2 translations.hello.lang: incomplete value string translations.hello.lang: conflicting values false and string (mismatched types bool and string): ./data.yaml:13:11 + ./vet.cue:3:25 ./vet.cue:3:31 -- expect-stderr2 -- translations.hello.lang: incomplete value string translations.hello.lang: conflicting values false and string (mismatched types bool and string): ./data.yaml:13:11 + ./vet.cue:3:25 ./vet.cue:3:31 -- vet.cue -- package foo diff --git a/cue/testdata/benchmarks/dedupelem.txtar b/cue/testdata/benchmarks/dedupelem.txtar index ca1ed5bd4..b83f06ce7 100644 --- a/cue/testdata/benchmarks/dedupelem.txtar +++ b/cue/testdata/benchmarks/dedupelem.txtar @@ -19,7 +19,7 @@ foo.0: 2 errors in empty disjunction: foo.0.type: conflicting values "int" and "float": ./in.cue:3:16 ./in.cue:5:14 - ./in.cue:5:30 + ./in.cue:6:30 foo.0.type: conflicting values "string" and "float": ./in.cue:5:14 ./in.cue:10:14 @@ -41,7 +41,7 @@ Result: // foo.0.type: conflicting values "int" and "float": // ./in.cue:3:16 // ./in.cue:5:14 - // ./in.cue:5:30 + // ./in.cue:6:30 // foo.0.type: conflicting values "string" and "float": // ./in.cue:5:14 // ./in.cue:10:14 diff --git a/cue/testdata/comprehensions/issue293.txtar b/cue/testdata/comprehensions/issue293.txtar index 3bd38ef88..30a81661a 100644 --- a/cue/testdata/comprehensions/issue293.txtar +++ b/cue/testdata/comprehensions/issue293.txtar @@ -27,6 +27,7 @@ t: p: "foo" -- out/eval -- Errors: z.x: field `f2` not allowed: + ./in.cue:2:2 ./in.cue:5:12 ./in.cue:11:4 ./in.cue:14:3 @@ -46,6 +47,7 @@ Result: f1: (int){ 99 } f2: (_|_){ // [eval] z.x: field `f2` not allowed: + // ./in.cue:2:2 // ./in.cue:5:12 // ./in.cue:11:4 // ./in.cue:14:3 diff --git a/cue/testdata/definitions/033_Issue_#153.txtar b/cue/testdata/definitions/033_Issue_#153.txtar index 1eaf9f503..1524dd491 100644 --- a/cue/testdata/definitions/033_Issue_#153.txtar +++ b/cue/testdata/definitions/033_Issue_#153.txtar @@ -67,6 +67,7 @@ listOfCloseds.0: field `b` not allowed: ./in.cue:5:10 ./in.cue:13:1 ./in.cue:14:18 + ./in.cue:15:3 ./in.cue:16:4 Result: @@ -83,6 +84,7 @@ Result: // ./in.cue:5:10 // ./in.cue:13:1 // ./in.cue:14:18 + // ./in.cue:15:3 // ./in.cue:16:4 } } diff --git a/cue/testdata/definitions/037_closing_with_comprehensions.txtar b/cue/testdata/definitions/037_closing_with_comprehensions.txtar index 289eda3f4..95024d235 100644 --- a/cue/testdata/definitions/037_closing_with_comprehensions.txtar +++ b/cue/testdata/definitions/037_closing_with_comprehensions.txtar @@ -108,9 +108,11 @@ Errors: ./in.cue:1:5 ./in.cue:27:5 ./in.cue:27:10 + ./in.cue:28:2 ./in.cue:29:3 a: field `f3` not allowed: ./in.cue:1:5 + ./in.cue:3:1 ./in.cue:4:5 ./in.cue:4:11 @@ -139,6 +141,7 @@ Result: // ./in.cue:1:5 // ./in.cue:27:5 // ./in.cue:27:10 + // ./in.cue:28:2 // ./in.cue:29:3 } } @@ -149,6 +152,7 @@ Result: f3: (_|_){ // [eval] a: field `f3` not allowed: // ./in.cue:1:5 + // ./in.cue:3:1 // ./in.cue:4:5 // ./in.cue:4:11 } diff --git a/cue/testdata/resolve/029_non-closed_definition_carries_over_closedness_to_enclosed_template.txtar b/cue/testdata/resolve/029_non-closed_definition_carries_over_closedness_to_enclosed_template.txtar index f94d398d2..c184206f4 100644 --- a/cue/testdata/resolve/029_non-closed_definition_carries_over_closedness_to_enclosed_template.txtar +++ b/cue/testdata/resolve/029_non-closed_definition_carries_over_closedness_to_enclosed_template.txtar @@ -114,6 +114,7 @@ b.w: field `c` not allowed: ./in.cue:10:4 ./in.cue:11:6 c.w.0: field `d` not allowed: + ./in.cue:14:12 ./in.cue:14:13 ./in.cue:16:4 ./in.cue:17:7 @@ -148,6 +149,7 @@ Result: // ./in.cue:11:6 c: (_|_){ // [eval] b.w: field `c` not allowed: + // ./in.cue:8:12 // ./in.cue:8:23 // ./in.cue:10:4 // ./in.cue:11:6 @@ -165,6 +167,7 @@ Result: // [eval] d: (_|_){ // [eval] c.w.0: field `d` not allowed: + // ./in.cue:14:12 // ./in.cue:14:13 // ./in.cue:16:4 // ./in.cue:17:7 diff --git a/internal/core/adt/closed.go b/internal/core/adt/closed.go index 327d7a5ac..6e978f30e 100644 --- a/internal/core/adt/closed.go +++ b/internal/core/adt/closed.go @@ -91,9 +91,9 @@ const ( // TODO: merge with closeInfo: this is a leftover of the refactoring. type CloseInfo struct { - IsClosed bool - *closeInfo + + IsClosed bool } // TODO(perf): remove: error positions should always be computed on demand @@ -141,6 +141,22 @@ func (c CloseInfo) SpawnGroup(x Expr) CloseInfo { return c } +// SpawnSpan is used to track that a value is introduced by a comprehension +// or constraint. Definition and embedding spans are introduced with SpawnRef +// and SpawnEmbed, respectively. +func (c CloseInfo) SpawnSpan(x Node, t SpanType) CloseInfo { + var span SpanType + if c.closeInfo != nil { + span = c.span + } + c.closeInfo = &closeInfo{ + parent: c.closeInfo, + location: x, + span: span | t, + } + return c +} + func (c CloseInfo) SpawnRef(arc *Vertex, isDef bool, x Expr) CloseInfo { var span SpanType if c.closeInfo != nil { @@ -153,6 +169,7 @@ func (c CloseInfo) SpawnRef(arc *Vertex, isDef bool, x Expr) CloseInfo { } if isDef { c.mode = closeDef + c.closeInfo.span |= DefinitionSpan } return c } @@ -187,12 +204,9 @@ const ( // EmbeddingSpan means that this value was embedded at some point and should // not be included as a possible root node in the todo field of OpContext. EmbeddingSpan SpanType = 1 << iota - - // TODO: - // from comprehension - // from template. - // from definition - + ConstraintSpan + ComprehensionSpan + DefinitionSpan ) type closeInfo struct { diff --git a/internal/core/adt/eval.go b/internal/core/adt/eval.go index 54058de4d..766f33958 100644 --- a/internal/core/adt/eval.go +++ b/internal/core/adt/eval.go @@ -1787,8 +1787,12 @@ func (n *nodeContext) injectEmbedded(all *[]envYield) (progress bool) { continue } + if len(sa) == 0 { + continue + } + id := d.id.SpawnSpan(d.yield, ComprehensionSpan) for _, st := range sa { - n.addStruct(st.env, st.s, d.id) + n.addStruct(st.env, st.s, id) } } diff --git a/internal/core/adt/expr.go b/internal/core/adt/expr.go index 2a6e1aae4..ba5aabd42 100644 --- a/internal/core/adt/expr.go +++ b/internal/core/adt/expr.go @@ -127,6 +127,7 @@ func (o *StructLit) Init() { if x.Value == nil { o.IsOpen = true o.types |= IsOpen + // TODO(perf): encode more efficiently. expr = &Top{} } else { o.types |= HasAdditional diff --git a/internal/core/adt/optional.go b/internal/core/adt/optional.go index 504d0f679..c648431d3 100644 --- a/internal/core/adt/optional.go +++ b/internal/core/adt/optional.go @@ -40,22 +40,26 @@ outer: return } - bulkEnv := *env - bulkEnv.DynamicLabel = arc.Label - bulkEnv.Deref = nil - bulkEnv.Cycles = nil - - // match bulk optional fields / pattern properties - for _, b := range o.Bulk { - // if matched && f.additional { - // continue - // } - if matchBulk(c, env, b, arc.Label) { - matched = true - arc.AddConjunct(MakeConjunct(&bulkEnv, b, closeInfo)) + if len(o.Bulk) > 0 { + bulkEnv := *env + bulkEnv.DynamicLabel = arc.Label + bulkEnv.Deref = nil + bulkEnv.Cycles = nil + + // match bulk optional fields / pattern properties + for _, b := range o.Bulk { + // if matched && f.additional { + // continue + // } + if matchBulk(c, env, b, arc.Label) { + matched = true + info := closeInfo.SpawnSpan(b.Value, ConstraintSpan) + arc.AddConjunct(MakeConjunct(&bulkEnv, b, info)) + } } } - if matched { + + if matched || len(o.Additional) == 0 { return } @@ -65,7 +69,11 @@ outer: // match others for _, x := range o.Additional { - arc.AddConjunct(MakeConjunct(&addEnv, x, closeInfo)) + info := closeInfo + if _, ok := x.(*Top); !ok { + info = info.SpawnSpan(x, ConstraintSpan) + } + arc.AddConjunct(MakeConjunct(&addEnv, x, info)) } }