hashicorp / hashicorp/terraform-plugin-framework
Consider Static Analysis Tooling for Unhandled Diagnostics
- 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
Assessment
This issue has not been assessed yet.