From 5a77ce1d37532ac79eec6305678f5b5d40826876 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Tue, 20 Aug 2024 11:50:20 +0100 Subject: [PATCH] core/adt: identify duplicate errors when pairwise-combining Revert cue/errors/errors.go to before https://review.gerrithub.io/c/cue-lang/cue/+/1199401 The previous attempt to fix this issue was extremely fragile: it only worked if there was an attempt to append the exact same error contiguously. In practice we have seen that this may not happen, rendering that fix ineffective. So it is reverted here. By contrast, in core/adt *OpContext.node(), it can be the case that the call to unifyNode returns the opContext's own errors. The subsequent call to *OpContext.AddBottom() and CombineErrors() can therefore result in the duplication of the opContext's errors. Identifying and solving this in CombineErrors() seems to be a robust solution. Fixes #3307 Signed-off-by: Matthew Sackman Change-Id: Ib3a0bff023343d410273017ea24e92d54346d77c Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199723 TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine Reviewed-by: Marcel van Lohuizen --- cue/errors/errors.go | 16 +--- internal/core/adt/errors.go | 23 +++--- .../core/export/testdata/main/alias.txtar | 4 +- .../core/export/testdata/main/simplify.txtar | 79 ++++++++++++++++++- 4 files changed, 98 insertions(+), 24 deletions(-) diff --git a/cue/errors/errors.go b/cue/errors/errors.go index 261cf6b0640..666b0e4f347 100644 --- a/cue/errors/errors.go +++ b/cue/errors/errors.go @@ -274,22 +274,14 @@ func (e *posError) Position() token.Pos { return e.pos } // Append combines two errors, flattening Lists as necessary. func Append(a, b Error) Error { - switch a := a.(type) { + switch x := a.(type) { case nil: return b case list: - return appendToList(a, b) + return appendToList(x, b) } - switch b := b.(type) { - case nil: - return a - case list: - return appendToList(list{a}, b) - } - if a == b { - return a - } - return list{a, b} + // Preserve order of errors. + return appendToList(list{a}, b) } // Errors reports the individual errors associated with an error, which is diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go index 581779fc42f..ce814f14ef3 100644 --- a/internal/core/adt/errors.go +++ b/internal/core/adt/errors.go @@ -176,17 +176,22 @@ func CombineErrors(src ast.Node, x, y Value) *Bottom { a, _ := Unwrap(x).(*Bottom) b, _ := Unwrap(y).(*Bottom) - if a == b && isCyclePlaceholder(a) { - return a - } switch { - case a != nil && b != nil: - case a != nil: - return a - case b != nil: - return b - default: + case a == nil && b == nil: return nil + case a == nil: + return b + case b == nil: + return a + case a == b && isCyclePlaceholder(a): + return a + case a == b: + // Don't return a (or b) because they may have other non-nil fields. + return &Bottom{ + Src: src, + Err: a.Err, + Code: a.Code, + } } if a.Code != b.Code { diff --git a/internal/core/export/testdata/main/alias.txtar b/internal/core/export/testdata/main/alias.txtar index 062501af53b..a8ced3a4a40 100644 --- a/internal/core/export/testdata/main/alias.txtar +++ b/internal/core/export/testdata/main/alias.txtar @@ -366,7 +366,7 @@ was known to compile and is known to be correct. } selfRefValue: { struct: { - a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 3 more errors) + a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 1 more errors) } pattern: { a: {} @@ -516,7 +516,7 @@ diff old new selfRefValue: { struct: { - a: _|_ // cycle error -+ a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 3 more errors) ++ a: _|_ // valueAlias.selfRefValue.struct.a: incomplete list: _ (and 1 more errors) } pattern: { a: {} diff --git a/internal/core/export/testdata/main/simplify.txtar b/internal/core/export/testdata/main/simplify.txtar index 1520290dee6..325803dc143 100644 --- a/internal/core/export/testdata/main/simplify.txtar +++ b/internal/core/export/testdata/main/simplify.txtar @@ -41,7 +41,7 @@ additional: { [additional confs] -- diff/value/explanation -- Benign change in error message. --- out/value -- +-- out/value-v3 -- == Simplified { x: { @@ -105,3 +105,80 @@ Benign change in error message. } } } +-- diff/-out/value-v3<==>+out/value -- +diff old new +--- old ++++ new +@@ -32,7 +32,7 @@ + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ +- confs: _|_ // additional.confs: incomplete bool: _ (and 1 more errors) ++ confs: _|_ // additional.confs: incomplete bool: _ + } + } + == All +-- out/value -- +== Simplified +{ + x: { + y: int64 + } + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ + confs: { + if env {} + } + } +} +== Raw +{ + x: { + y: >=-9223372036854775808 & <=9223372036854775807 & int + } + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ + confs: { + if env {} + } + } +} +== Final +{ + x: { + y: int64 + } + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ + confs: _|_ // additional.confs: incomplete bool: _ (and 1 more errors) + } +} +== All +{ + x: { + y: int64 + } + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ + confs: { + if env {} + } + } +} +== Eval +{ + x: { + y: >=-9223372036854775808 & <=9223372036854775807 & int + } + s: strings.MinRunes(4) & strings.MaxRunes(7) + additional: { + env: _ + confs: { + if env {} + } + } +}