Error when loading plugin schemas outputs diagnostics suffixed with double period

[bendbennett]

Terraform Version

Terraform v1.8.0-dev
on darwin_arm64

v1.8.0-beta1

Terraform Configuration Files

resource "example_resource" "example" {
  configurable_attribute = provider::example::string_len("some-value")
}

Debug Output

https://gist.github.com/bendbennett/6e635384603062b983df30f74b9bbacc

Expected Behavior

Expected the last line of the practitioner-facing error message from the CLI to be suffixed with a single period:

│ Function "string_len": Parameter at position 0 does not have a name.

Actual Behavior

The last line of the practitioner-facing error message from the CLI is suffixed with a double period:

│ Function "string_len": Parameter at position 0 does not have a name..

Steps to Reproduce

1. terraform init
2. terraform apply

Additional Context

This appears to be because the diagnostics are suffixed with a period in two locations:

• https://github.com/hashicorp/terraform/blob/v1.8.0-beta1/internal/terraform/schemas.go#L109
• https://github.com/hashicorp/terraform/blob/v1.8.0-beta1/internal/terraform/context.go#L173

References

No response

[apparentlymart]

Hi @bendbennett,

Our typical convention is that error.Error() results should follow the Go convention of not ending with a period, whereas our human-oriented diagnostics are written as full sentences (or paragraphs) and so do use periods to end sentences.

Because of that, whenever we're using an error.Error() as part of a diagnostic message, we typically add a period after it to translate between the two conventions.

From the locations you linked it seems like this symptom would arise if the error.Error() result also had a period, which would be contrary to both our conventions and typical Go idiom. If that's true, I think the fix would be to remove the errant period from the end of the string representation of the error. Is that what's going on here, or did I misunderstand?

[bendbennett]

Hi @apparentlymart,

Thanks for looking into this.

Apologies if I've misunderstood. It seems the double period is because the error returned by loadSchemas() is generated from a call to diags.Err(), and the diagnostics already include a period. So if I'm following your line of reasoning, then perhaps the error returned by diags.Err() should not contain a suffixed period?

[apparentlymart]

Generating an error from a Diagnostics and then inserting its string representation into another diagnostic is indeed a situation that can cause this problem. We try to avoid doing that wherever possible -- a diagnostics wrapped in an error should typically be appended directly to another diagnostics, which then automatically unwraps the wrapped diagnostics and appends each of them separately. But if course that can't happen if it's formatted into part of another diagnostic message.

Looking around at the context near the code snippets you pointed to, I think what we have here is some leftover tech debt from the original shift from using error to diagnostics when reporting errors to users. During that transition period we had a lot of APIs that had to still keep returning error for a while due to having multiple callsites to update, and so we used this sort of error wrapping and then unwrapping again at the callers that had been updated.

There thankfully aren't many example of that left anymore, but this seems like one we missed. The loadSchemas function should change to return diagnostics instead of an error, and then the caller should just return those diagnostics directly instead of trying to build its own error diagnostic.