Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/go/analysis/analysistest: support multiple, mutually exclusive suggested fixes #38431

Closed
dominikh opened this issue Apr 14, 2020 · 5 comments
Closed

x/tools/go/analysis/analysistest: support multiple, mutually exclusive suggested fixes #38431

dominikh opened this issue Apr 14, 2020 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@dominikh
Copy link
Member

A single diagnostic may have multiple suggested fixes. For example, staticcheck has an analysis that finds double negations (!!x) and provides two suggested fixes: turn into a single negation, or remove the double negation.

analysistest doesn't support testing such an analysis. Currently, all suggested fixes get applied to the same source, and the result is compared against a single golden file. For example, !!x turns into !xx. In the worst case, the final result will not even be syntactically valid.

When dealing with just one diagnostic per analysis run, it would be pretty straightforward to adopt a file format like the one used by the Playground to support multiple files (https://play.golang.org/p/KLZR7NlVZNX). The virtual files could be named according to the descriptions of the suggested fixes, to avoid depending on the order of suggested fixes. This would result in a golden file looking something like this:

-- turn into single negation --
package pkg

func fn(x bool) { _ = !x }

-- remove double negation --
package pkg

func fn(x bool) { _ = x }

This, however, doesn't really work if an analysis produces multiple diagnostics, especially if different diagnostics have different sets of suggested fixes. If d1 has the fixes f1 and f2, and d2 has the fixes f1 and f3, what should the golden file look like? Should there be one virtual file for every unique suggested fix, identified by the fixes' descriptions?

I'm very open to better ideas.

/cc @matloob @ridersofrohan
@dominikh dominikh added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 14, 2020
@gopherbot gopherbot added this to the Unreleased milestone Apr 14, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Apr 14, 2020
@dominikh dominikh mentioned this issue Apr 14, 2020
Closed
@matloob
Copy link
Contributor

matloob commented Apr 14, 2020

I'd be open and welcome something like this, but I don't know that I have better ideas for how to handle multiple fixes produced by multiple diagnostics.

I think adding a virtual file for each unique suggested fix is a good idea, but we could potentially run into the same problem again if there are two diagnostics producing fixes with the same name that affect overlapping areas of code. (Which may or may not be too rare to matter).

@dominikh
Copy link
Member Author

but we could potentially run into the same problem again if there are two diagnostics producing fixes with the same name that affect overlapping areas of code

I'm not sure how we could fix that without having a one to one mapping between a single diagnostic and a single golden file. But in most cases, the user could achieve this by splitting the fixture into multiple files, each triggering a single diagnostic.

I've implemented my suggested idea to get a feel for the golden files. It's not terrible, but suffers somewhat from well-written messages for suggested fixes. For example, SA1016 flags untrappable signals (like os.Kill) and offers to remove the signal, or use a more appropriate signal. For UX, the message contains the names of the signals, so for my fixture that tests with os.Kill and syscall.SIGKILL, there are 4 possible suggested fixes

  • remove syscall.SIGKILL from list of arguments
  • remove os.Kill from list of arguments
  • use syscall.SIGTERM instead of syscall.SIGKILL
  • use syscall.SIGTERM instead of os.Kill

which results in this somewhat unwieldy golden file: https://play.golang.org/p/b1cVvL7Y_rV – I might be able to clean it up by having multiple files, one per signal, but multiple files rarely make things easier to use.

@dominikh
Copy link
Member Author

If this is the best we can come up with, then I'll send a CL shortly.

@matloob
Copy link
Contributor

matloob commented Apr 20, 2020

Yeah, I can't think of a anything significantly better for now

@gopherbot
Copy link

Change https://golang.org/cl/229257 mentions this issue: go/analysis/analysistest: support testing mutually exclusive suggested fixes

@golang golang locked and limited conversation to collaborators Apr 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@dominikh @gopherbot @matloob