ci: Add (tfdiags.Diagnostics).Append checker to PR checks

[SarahFrench]

Background

A defect that can occur in our code is calling a tfdiags.Diagnostics's Append method and failing to append the result to the calling code's tfdiags.Diagnostics collection:

func doThing() tfdiags.Diagnostics {
  var diags tfdiags.Diagnostics
  result, resultDiagnotics := getResult()
  diags.Append(resultDiagnotics) // lost append result
  if diags.HasErrors() {
    // return early
    return diags
  }
  // else ... continue doing stuff
}

The fix is simple, when caught:

-   diags.Append(resultDiagnotics)
+   diags = diags.Append(resultDiagnotics)

Given that our code's control flow depends on calling code being able to detect error diagnostics, this type of defect could impact users depending on what part of the code it's in.

This type of defect is a potential source of low-effort contribution. Due to the potential impact on user-facing behaviour these fixes should be accompanied by investigating the surrounding code and checking if additional test coverage is necessary or possible. Recently an example of this defect being addresses uncovered a race condition that would have been nice to avoid entering the code base.

This PR adds some automated checks to help avoid new occurrences entering the codebase.

This PR

To be transparent, the Go code in this PR was vibecoded using Copilot, and I also used it when making PR-aware logic in the bash script. When I saw the initial Go code analyser solution using golang.org/x/tools/go/analysis I recognised that package as something I'd looked at briefly before but didn't realise/remember would be the appropriate tool to use in this situation. I'm glad this solution was made instead of a horrible grep/regex monster, and it's small enough to understand and take ownership of from the agent.

Going forward if we identify other code patterns we want to avoid we could make this code a more generalised checker with multiple Analyzer implementations.

The DiagsAppendAnalyzer analyzer identifies places in the codebase that contain this defect. If you run make tfdiagsappendcheck locally the scripts/tfdiagsappendcheck.sh script will detect all instances of the defect in the codebase.

The output looks like this:

/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:367:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:380:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:393:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:406:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:419:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:432:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:445:19: ignored return value from tfdiags.Diagnostics.Append
/Users/sarahfrench/go/src/github.com/hashicorp/terraform/internal/provider-simple-v6/provider.go:458:19: ignored return value from tfdiags.Diagnostics.Append
exit status 3

If the script is run in the context of a GHA (i.e. GITHUB_BASE_REF is present) then the script will instead detect new instances of the defect added in the PR. This is achieved by checking out the head and base branches, running the Go tool on them separately and then diffing the output.

Related

• test: Show warning when a -filter flag has an invalid value #38603
• fix: Capture previously-discarded diagnostics in test providers #38602
• fix: Append lost warnings in MaybeGetSchemas #38605

Target Release

N/A

Rollback Plan

• If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

• This change is user-facing and I added a changelog entry.
• This change is not user-facing.

[SarahFrench]

Currently the check is failing (as expected) on this PR due to remaining instances of this defect in the codebase

[SarahFrench]

Thinking to the future, I realised that if we wanted to extend this to catch other defects specific to this codebase it wouldn't be sustainable to always require fixing every instance of a problem before adding the check to PRs. I used Copilot to propose an approach for diffing the tools' results between the current and base branch.

6506440 contains those changes, but it's failing because it requires the tools added in this PR to be present in the target branch.

That will cause tons of issues in the context of backports etc. I'll update the approach

[SarahFrench]

Now in e4476d0 the binary used for the check is built from the head branch and is used both when analysing the head and base branches. This means that the target branch of a PR doesn't need to include the new tool.

I think this handles necessary scenarios:

• This PR into main (tool only available here, built from here)
• A backport from main into an old release (tool only available in head, so built from head)

If the tool is missing from head then the check won't be in CI, so that scenario is irrelevant. That could happen if someone checked out an old release branch and made a change directly, instead of via backport from main.

[radeksimko]

I think this is great. While we could be theoretically adding many "linters" I like how this is focused on a known issue and should produce no false positives.

My only minor question is about the naming and "general-purpose nature" of this new code.

It came to mind originally because tfdiagsappendcheck sounded mouthful 😅 but it still made me wonder if in the future we might add some other analysers, and so it could be refactored as analyser or something like that?

Don't let that be the blocker though if you disagree or feel more neutral about it.

[SarahFrench]

I've renamed the tool to be non-specific to checking diags-related stuff, but left the analyzer with the same name as that is specific to diags. I do hope we find other problems that can be solved this way, and that'll be done by implementing more analyzers, I imagine.

Also added code comments on how the test assertions are happening, as I realised that wasn't super clear.