x/tools: implement three-way merge and use in all tools that apply fixes #68765
Assignees
Labels
Analysis
Issues related to static analysis (vet, x/tools/go/analysis)
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Refactoring
Issues related to refactoring tools
Tools
This label describes issues relating to any tools in the x/tools repository.
Milestone
Comments
|
Change https://go.dev/cl/643196 mentions this issue: |
|
Change https://go.dev/cl/643695 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Jan 24, 2025
This change contains some minor cleanups split out of the forthcoming three-way merge work. 1. Plumb pass.ReadFile down from a (hidden) checker option. Factor CheckedReadFile helper. 2. In "assign" checker, improve SuggestedFix title. 3. Fix bug in error handling in fix_test.go. 4. Define testenv.RedirectStderr helper to temporarily redirect stderr and log output to t.Log, to declutter the test output. Update golang/go#68765 Update golang/go#67049 Change-Id: Icac62afdeb160a2dfa3cc3637b79fe7d89e92272 Reviewed-on: https://go-review.googlesource.com/c/tools/+/643695 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Robert Findley <rfindley@google.com> Commit-Queue: Alan Donovan <adonovan@google.com> Auto-Submit: Alan Donovan <adonovan@google.com>
|
Change https://go.dev/cl/644835 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Jan 30, 2025
This CL addes a Merge operator to the diff package. It performs a simple three-way merge on two ordered lists of valid Edits, and reports a conflict if any edit could not be applied cleanly. I suspect there is considerable latitude in the implementation. This versions considers two identical insertions as non-conflicting, as is the case for redundant imports of the same package; however, it may be inappropriate for, say, identical statements that increment a counter, where the correct resolution is to keep both copies. + tests. Update golang/go#68765 Update golang/go#67049 Change-Id: I7d8bf5b0b2e601c15d3ee787499e6adc012f884b Reviewed-on: https://go-review.googlesource.com/c/tools/+/643196 Auto-Submit: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Commit-Queue: Alan Donovan <adonovan@google.com> Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Feb 7, 2025
This CL adds support for three-way merging to the checker's -fix operation. It consists of three parts: 1. a rewritten applyFix function that applies as many changes as can be cleanly merged; 2. a script-based test framework that allows all existing and new tests to be written as txtar files in testdata instead of ad-hoc Go logic; and 3. a data-driven "marker" analyzer that reports diagnostics containing fixes according to //@f comments in the target Go source files. Also, it adds a -diff flag to the checker tools that causes them to print the computed file changes instead of directly applying them. The new applyFix treats each SuggestedFix as an independent change, analogous to a git commit. Fixes are combined by invoking a three-way merge algorithm, diff.Merge, analogous to git merge, except simpler since it works on the list of []diff.Edit instead of text. If any fix does not apply cleanly, we discard it, and report that we did so, with a hint to run the tool again until a fixed point is reached. (This is just a starting point; a better UX would be for the tool to do this itself.) If a diagnostic has multiple suggested fixes, we select the first one. The old behavior of attempting to apply them all makes no sense. The support for filesystem-level aliases (e.g. symbolic and hard links) previously implemented using FileID has been removed, as its interactions with the new logic were tricky. I ran gopls' modernize singlechecker on k8s/... and it was able to cleanly resolved 142 edits across 53 files; the result builds, and symbolic links were not evidently a problem. Update golang/go#68765 Update golang/go#67049 Change-Id: Id3fb55118b3d0612cafe7e86f52589812bd74a96 Reviewed-on: https://go-review.googlesource.com/c/tools/+/644835 Reviewed-by: Robert Findley <rfindley@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Alan Donovan <adonovan@google.com>
|
Change https://go.dev/cl/647798 mentions this issue: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When analysis and refactoring tools suggest multiple fixes to the same file, those fixes should be treated as independent changes, each analogous to a separate CL. Just as multiple CLs are resolved into a linear sequence using a tool such as diff3 or git-mergetool, our fixes should be reconciled intelligently too. For example, two fixes that add the same import should compose to a single diff that adds the desired import only once.
Merging two changes may fail; in that case, the (arbitrary) winner should be applied, and the loser skipped and reported to the caller.
The task of this issue is to design and implement a three-way merge algorithm and use it from all our tools (multichecker, analysistest, gopls, etc) that produce multiple independent edits. A poor-quality implementation (e.g. fail on any conflict) may serve as a temporary placeholder for a more sophisticated one (e.g. fork+exec diff3 or git-mergetool, or implement the diff3 algorithm atop x/tools/internal/diff/lcs).
For more background, see:
@pjweinb @findleyr @timothy-king
The text was updated successfully, but these errors were encountered: