From da42f6b29ef718d7a57bc57fa294e421ba09caac Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 2 Apr 2025 09:14:15 -0400 Subject: [PATCH 1/4] don't re-add non-comparable diagnostics Non-comparable diagnostics were being adding multiple times, which might also panic in the equality comparison --- internal/tfdiags/diagnostics.go | 2 +- internal/tfdiags/diagnostics_test.go | 170 +++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 1 deletion(-) diff --git a/internal/tfdiags/diagnostics.go b/internal/tfdiags/diagnostics.go index fcb2e47cd089..5e549de8f8da 100644 --- a/internal/tfdiags/diagnostics.go +++ b/internal/tfdiags/diagnostics.go @@ -107,8 +107,8 @@ func (diags Diagnostics) AppendWithoutDuplicates(newDiags ...Diagnostic) Diagnos if !ok { // append what we cannot compare diags = diags.Append(newItem) + continue } - if diags.ContainsDiagnostic(cd) { continue } diff --git a/internal/tfdiags/diagnostics_test.go b/internal/tfdiags/diagnostics_test.go index 0f61f7fdabf9..7c533495d6fc 100644 --- a/internal/tfdiags/diagnostics_test.go +++ b/internal/tfdiags/diagnostics_test.go @@ -542,3 +542,173 @@ func TestWarnings(t *testing.T) { }) } } + +func TestAppendWithoutDuplicates(t *testing.T) { + type diagFlat struct { + Severity Severity + Summary string + Detail string + Subject *SourceRange + Context *SourceRange + } + + tests := map[string]struct { + Cons func(Diagnostics) Diagnostics + Want []diagFlat + }{ + "nil": { + func(diags Diagnostics) Diagnostics { + return nil + }, + nil, + }, + "errors.New": { + // these could be from different locations, so we can't dedupe them + func(diags Diagnostics) Diagnostics { + return diags.Append( + errors.New("oh no bad"), + errors.New("oh no bad"), + ) + }, + []diagFlat{ + { + Severity: Error, + Summary: "oh no bad", + }, + { + Severity: Error, + Summary: "oh no bad", + }, + }, + }, + "hcl.Diagnostic": { + func(diags Diagnostics) Diagnostics { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 3, Byte: 25}, + }, + Context: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 3, Column: 1, Byte: 30}, + }, + }) + // exact same diag + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 3, Byte: 25}, + }, + Context: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 1, Byte: 0}, + End: hcl.Pos{Line: 3, Column: 1, Byte: 30}, + }, + }) + // same diag as prev, different location + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 4, Column: 10, Byte: 40}, + End: hcl.Pos{Line: 5, Column: 3, Byte: 55}, + }, + Context: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 4, Column: 1, Byte: 40}, + End: hcl.Pos{Line: 6, Column: 1, Byte: 60}, + }, + }) + return diags + }, + []diagFlat{ + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 1, Column: 10, Byte: 9}, + End: SourcePos{Line: 2, Column: 3, Byte: 25}, + }, + Context: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 1, Column: 1, Byte: 0}, + End: SourcePos{Line: 3, Column: 1, Byte: 30}, + }, + }, + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 4, Column: 10, Byte: 40}, + End: SourcePos{Line: 5, Column: 3, Byte: 55}, + }, + Context: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 4, Column: 1, Byte: 40}, + End: SourcePos{Line: 6, Column: 1, Byte: 60}, + }, + }, + }, + }, + "simple warning": { + func(diags Diagnostics) Diagnostics { + diags = diags.Append(SimpleWarning("Don't forget your toothbrush!")) + diags = diags.Append(SimpleWarning("Don't forget your toothbrush!")) + return diags + }, + []diagFlat{ + { + Severity: Warning, + Summary: "Don't forget your toothbrush!", + }, + { + Severity: Warning, + Summary: "Don't forget your toothbrush!", + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + var deduped Diagnostics + + diags := test.Cons(nil) + deduped = deduped.AppendWithoutDuplicates(diags...) + + var got []diagFlat + for _, item := range deduped { + desc := item.Description() + source := item.Source() + got = append(got, diagFlat{ + Severity: item.Severity(), + Summary: desc.Summary, + Detail: desc.Detail, + Subject: source.Subject, + Context: source.Context, + }) + } + + if !reflect.DeepEqual(got, test.Want) { + // t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(test.Want)) + t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.Want) + + } + }) + } +} From aeb1c0db09724d89fb41ee48d7777f9b436b28ba Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 2 Apr 2025 09:48:10 -0400 Subject: [PATCH 2/4] we can't dedupe extra values in diags Extra values can be any type, and are only asserted by interface by the party expecting that interface so we can't attempt to compare them for equality. The only diagnostic type with Extra values which implements Equals is and hcl diagnostic. --- internal/tfdiags/diagnostics_test.go | 51 ++++++++++++++++++++++++++++ internal/tfdiags/hcl.go | 5 +++ 2 files changed, 56 insertions(+) diff --git a/internal/tfdiags/diagnostics_test.go b/internal/tfdiags/diagnostics_test.go index 7c533495d6fc..c3c12bc85236 100644 --- a/internal/tfdiags/diagnostics_test.go +++ b/internal/tfdiags/diagnostics_test.go @@ -682,6 +682,57 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, }, + "hcl.Diagnostic extra": { + // Extra can contain anything, and we don't know how to compare + // those values, so we can't dedupe them + func(diags Diagnostics) Diagnostics { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Extra: 42, + Subject: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 3, Byte: 25}, + }, + }) + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Extra: 38, + Subject: &hcl.Range{ + Filename: "foo.tf", + Start: hcl.Pos{Line: 1, Column: 10, Byte: 9}, + End: hcl.Pos{Line: 2, Column: 3, Byte: 25}, + }, + }) + return diags + }, + []diagFlat{ + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 1, Column: 10, Byte: 9}, + End: SourcePos{Line: 2, Column: 3, Byte: 25}, + }, + }, + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + Subject: &SourceRange{ + Filename: "foo.tf", + Start: SourcePos{Line: 1, Column: 10, Byte: 9}, + End: SourcePos{Line: 2, Column: 3, Byte: 25}, + }, + }, + }, + }, } for name, test := range tests { diff --git a/internal/tfdiags/hcl.go b/internal/tfdiags/hcl.go index edf93342e9d6..1e6ccd3ec2b3 100644 --- a/internal/tfdiags/hcl.go +++ b/internal/tfdiags/hcl.go @@ -75,6 +75,11 @@ func (d hclDiagnostic) Equals(otherDiag ComparableDiagnostic) bool { return false } + // we can't compare extra values without knowing what they are + if d.ExtraInfo() != nil || od.ExtraInfo() != nil { + return false + } + return true } From c6c21a89a13a04f4492c1a7ebb82edffc0bf18e4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 2 Apr 2025 10:55:01 -0400 Subject: [PATCH 3/4] hcl diagnostics must check Subject source The source check was skipped if either value was nil, but we can't be sure they are equal if there is no value. This may prove to be problematic is tests, as diagnostic equality for deduplication isn't strict equality, but rather relies on being able to prove the context is also equal. --- internal/tfdiags/compare_test.go | 33 +++++++++++++------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/internal/tfdiags/compare_test.go b/internal/tfdiags/compare_test.go index 69a08efc0319..760679d56753 100644 --- a/internal/tfdiags/compare_test.go +++ b/internal/tfdiags/compare_test.go @@ -17,6 +17,19 @@ func TestDiagnosticComparer(t *testing.T) { Severity: hcl.DiagError, Summary: "error", Detail: "this is an error", + Subject: &hcl.Range{ + Filename: "foobar.tf", + Start: hcl.Pos{ + Line: 0, + Column: 0, + Byte: 0, + }, + End: hcl.Pos{ + Line: 1, + Column: 1, + Byte: 1, + }, + }, } cases := map[string]struct { @@ -81,26 +94,6 @@ func TestDiagnosticComparer(t *testing.T) { }(), expectDiff: true, }, - "reports that diagnostics match even if sources (Context) are different; ignored in simple comparison": { - diag1: hclDiagnostic{&baseError}, - diag2: func() Diagnostic { - d := baseError - d.Context = &hcl.Range{ - Filename: "foobar.tf", - Start: hcl.Pos{ - Line: 0, - Column: 0, - Byte: 0, - }, - End: hcl.Pos{ - Line: 1, - Column: 1, - Byte: 1, - }, - } - return hclDiagnostic{&d} - }(), - }, } for tn, tc := range cases { From 291d26ce19f28024917a2d9b800987110ab8670b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 2 Apr 2025 10:58:19 -0400 Subject: [PATCH 4/4] we can't compare location if either are nil --- internal/tfdiags/diagnostics_test.go | 31 ++++++++++++++++++++++++++-- internal/tfdiags/hcl.go | 3 ++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/internal/tfdiags/diagnostics_test.go b/internal/tfdiags/diagnostics_test.go index c3c12bc85236..1cac152ef0eb 100644 --- a/internal/tfdiags/diagnostics_test.go +++ b/internal/tfdiags/diagnostics_test.go @@ -733,6 +733,35 @@ func TestAppendWithoutDuplicates(t *testing.T) { }, }, }, + "hcl.Diagnostic no-location": { + // Extra can contain anything, and we don't know how to compare + // those values, so we can't dedupe them + func(diags Diagnostics) Diagnostics { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + }) + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + }) + return diags + }, + []diagFlat{ + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + }, + { + Severity: Error, + Summary: "Something bad happened", + Detail: "It was really, really bad.", + }, + }, + }, } for name, test := range tests { @@ -756,9 +785,7 @@ func TestAppendWithoutDuplicates(t *testing.T) { } if !reflect.DeepEqual(got, test.Want) { - // t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(test.Want)) t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.Want) - } }) } diff --git a/internal/tfdiags/hcl.go b/internal/tfdiags/hcl.go index 1e6ccd3ec2b3..3531237477f8 100644 --- a/internal/tfdiags/hcl.go +++ b/internal/tfdiags/hcl.go @@ -85,8 +85,9 @@ func (d hclDiagnostic) Equals(otherDiag ComparableDiagnostic) bool { func hclRangeEquals(l, r *hcl.Range) bool { if l == nil || r == nil { - return l == r + return false } + if l.Filename != r.Filename { return false }