hashicorp / hashicorp/terraform-plugin-framework

Consider Static Analysis Tooling for Unhandled Diagnostics

Open
#462 0 comments 1 reaction 0 assignees View on GitHub
enhancement thinking
Dominant language
Go
Stars
384
Forks
107
Avg merge
3m
Merged PRs (30d)
1

Description

### Module version

```
v0.11.1
```

### Use-cases

There are possibilities for provider developers to miss diagnostics handling in their logic, such as resource CRUD functionality or attribute plan modifiers. For example:

```go
var attr1Config, attr2Config types.Bool

diags := req.Config.GetAttribute(ctx, path.Root("attr1"), &attr1Config)
resp.Diagnostics.Append(diags...)
if resp.Diagnostics.HasError() {
return
}

// NOTE: Missing diags = so the variable is not updated
req.Config.GetAttribute(ctx, path.Root("attr2"), &attr2Config)
// NOTE: Old warning diagnostics are now doubled in the response
resp.Diagnostics.Append(diags...)
if resp.Diagnostics.HasError() {
return
}
```

Other examples may be less trivial. Please feel free to add other problematic cases in the comments.

### Attempted Solutions

Non-trivially catch via unit testing or catch via acceptance testing, which can miss potential cases. Catch during code reviews.

### Proposal

One framework design change to potentially handle this would be to _require_ a `diag.Diagnostics` parameter to this type of functionality. This feels suboptimal because it tightly couples a function return value with a function parameter, which beyond the pointer target of `As()`-like or value decoding functions in Go is likely considered an anti-pattern. Diagnostics were generally designed to be similar enough to `error` handling, which are almost exclusively handled via returns, so moving away from that design may introduce additional developer burden.

Previously, terraform-plugin-sdk based providers could use tooling such as [`tfproviderlint`](https://github.com/bflad/tfproviderlint) for static analysis of provider code and its quirks. Something similar could be done here, where [`go/analysis` passes](https://pkg.go.dev/golang.org/x/tools/go/analysis) can be implemented alongside the code or separately, however any CLI implementation could reside outside the scope of this repository.

### References

- https://github.com/kisielk/errcheck -- similar, widely deployed static analysis for `error` return checking
- https://github.com/hashicorp/terraform-provider-random/pull/297

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.