incorrect handling of nil pointer receiver in Diagnostics.Append

[john-behm-bertelsmann]

It's not possible to handle the case when the receiver is nil in line 47 which is then a nil pointer to a slice (pointer).
This line is not covered by tests, as it is incorrect and impossible to handle.
It gives a false sense of that such a case can be handled, which it cannot.

terraform-plugin-framework/diag/diagnostics.go

Lines 37 to 53 in f35653e

	func (diags *Diagnostics) Append(in ...Diagnostic) {
	for _, diag := range in {
	if diag == nil {
	continue
	}
	if diags.Contains(diag) {
	continue
	}
	if diags == nil {
	*diags = Diagnostics{diag}
	} else {
	*diags = append(*diags, diag)
	}
	}
	}

[bflad]

Hi @john-behm-bertelsmann 👋 Thank you for reporting this. Did you encounter this as an issue when writing provider code or are you reporting this as a potential gotcha that someone might run into?

[john-behm-bertelsmann]

It's more of a gotcha that I stumbled upon when looking at the Diagnostics implementation while implementing a provider but not running into any issues with this.

The Append implementation implies that TestDiagnosticsAppend01 is possible without a panic.

package internal_test

import (
	"testing"

	"github.com/hashicorp/terraform-plugin-framework/diag"
)

// Is impossible in any case
func TestDiagnosticsAppend01(t *testing.T) {
	t.Parallel()
	defer func() {
		i := recover()
		if i == nil {
			t.Fatal("expected panic")
		}
	}()
	var d *diag.Diagnostics = nil
	d.Append(diag.NewErrorDiagnostic("error", "error message"))
}

// Is possible
func TestDiagnosticsAppend02(t *testing.T) {
	t.Parallel()
	defer func() {
		i := recover()
		if i != nil {
			t.Fatal("expected no panic")
		}
	}()
	var d diag.Diagnostics = nil
	d.Append(diag.NewErrorDiagnostic("error", "error message"))
}

The "correct" implementation would panic as well, because it's not possible to do that.
It would be just less code and just *diags = append(*diags, diag)instead of the if-else block.

[bflad]

Thank you for that additional context, @john-behm-bertelsmann! Would you like to submit the code fix for this?

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.