Skip to content

x/tools/go/analysis: overlapping suggested fixes result in a single, unhelpful error message #56535

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
Closed
vovean opened this issue Nov 2, 2022 · 5 comments
Closed

x/tools/go/analysis: overlapping suggested fixes result in a single, unhelpful error message #56535

vovean opened this issue Nov 2, 2022 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@vovean
Copy link

vovean commented Nov 2, 2022

What version of Go are you using (go version)?

$ go version
go version go1.19 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/vislovicvladimir/Library/Caches/go-build"
GOENV="/Users/vislovicvladimir/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/vislovicvladimir/go/1.19/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/vislovicvladimir/go/1.19"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.19/libexec"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.19/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1h/cgr8wr0j0653zvfgq_xwqpt40000gp/T/go-build3420330214=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a project with a single file main.go. It has the code:

package main

import "fmt"

type S struct {
	A int8
	B struct {
		B int
		A string
	}
	D int64
}

func main() {
	fmt.Print(S{})
}

I tried running fieldalignment tool on that:

$ fieldalignment ./...     
/Users/vislovicvladimir/GolandProjects/sandbox/main.go:5:8: struct with 24 pointer bytes could be 16
/Users/vislovicvladimir/GolandProjects/sandbox/main.go:7:4: struct with 16 pointer bytes could be 8

So, I see 2 issues and then I tried to fix them by adding -fix flag:

$ fieldalignment -fix ./...
fieldalignment: analyses applying overlapping text edits affecting pos range (55, 85) and (35, 96)

And I get this weird error. Googling it gives me an issue about the case when the same piece of code get's replaced twice, it was fixed about 2 months ago.

However, in my case, there are pieces of code which both need to be fixed and one is inside of the other.

What did you expect to see?

I expect fieldalignment tool to process my file correctly and to do its fixes

What did you see instead?

Error
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 2, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 2, 2022
@timothy-king
Copy link
Contributor

an issue about the case when the same piece of code get's replaced twice

You are getting overlapping suggestions. The first is to move B struct { B int; A string} above A int8 and a suggestion to move A string before B int. These edits overlap. The part of the tool doing the text rewrite then stops as it does not know how to edit the file text without a conflict. Right now edits are all independent. You can think of each edit as describing a patch, and the tool is trying to cherry pick them and failing because there is no strategy for merge conflicts.

One action that can be taken is to improve the error message to be less obscure. We can give the conflicting diffs that were not applied in the error message instead of giving the byte offsets. That way a user can apply one fix manually and then rerun the tool.

We do not want to modify the tool to give one error message with a single fix. That would be misleading in other contexts (gopls or without -fix). We also do not want to the tool to pick one, try it, reapply the tool to see if it gets new edits, and repeat. This loop or other automated strategies seem like they are best done by a human.

One option is to try to capture a "group edit" from checkers when generating a Diagnostic. These group edits could be applied when present and they do not overlapping with other groups of edits. Checkers that want to support these would need to produce them. This group edit idea would be a good candidate for a community contribution.

Yet another strategy would be to leave the code in a state with merge conflicts. Something like git's https://git-scm.com/docs/git-merge#_how_conflicts_are_presented. This would usually break compilation. If this happened to be in the middle of a string, the code may still compile.

Among these options I think the best is to give a better error message and to let users fix one of the issues until there are no overlapping edits.

@vovean
Copy link
Author

vovean commented Nov 3, 2022

@timothy-king thanks for the reply, now it is clear to me.

However, what do you think about another option, how about to add a flag to the tool, toggling which will skip fixing all such overlaping places in the code and will only fix those which it can without conflicts. Something like fieldalignment -fix -skip-errs ./...

I propose that because this I issue I have encountered is actually in a very big project in which fieldalignment has found about 200 places which could be fixed. And just because of a single overlapping place it cannot fix other 199 places. It would be great if it can fix those 199 and somehow highlight the fact that is had skipped some places because of some reason. Since you mentioned that edits are independent I believe such behavior will not be somehow incorrect or hard to implement.

@timothy-king
Copy link
Contributor

cc'ing @adonovan, @matloob

So the request would be that if -fix is on to apply the edits with no overlaps to any other proposed edits, and then report an error[s?] for the unapplied overlapping regions? Currently just the first overlapping pair is reported. We could maybe give a better warning by saying how many edits were not done. It would not necessarily do anything different for the other overlapping edits. For the example given, this would [maybe] only give a better error as there are exactly 2 edits. For the 200 edits case with 2 overlapping regions and 198 non-overlapping edits, to would apply 198 and then warn on the two overlapping.

To keep the implementation feasible this would need to go into x/tools/go/analysis/internal/checker and apply to all singlechecker and multichecker. A proposal is likely needed.

I feel like this would be a somewhat reasonable extension of the -fix flag. But it does mean that there are side effects after an error + an error exit code happens. Alternatively, we could add a new flag or add a new meaning to -fix, say -fix=continue, to indicate this is desired. What to others think?

And just because of a single overlapping place it cannot fix other 199 places.

FWIW progress can be made in this situation by fixing the 1 overlap that is indicated manually. The tool can solve the rest when it is rerun. I understand that this might not be the smoothest user experience.

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 3, 2022
@mdempsky mdempsky changed the title x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment: fix fails for nested anonymous struct x/tools/go/analysis: overlapping suggested fixes result in a single, unhelpful error message Nov 3, 2022
@turbolent
Copy link

turbolent commented Dec 7, 2022 •
edited
Loading

+1

I just ran into this while trying to optimize https://github.com/onflow/cadence:

fieldalignment -fix ./runtime/interpreter/...
fieldalignment: analyses applying overlapping text edits affecting pos range (861, 901) and (841, 957)

It would be great to have the suggested continuation feature, but before, it would be even more useful to just include the paths for the suggested edits. The path is missing right now, which requires tedious bisecting by trial and error of subpackages.

After tracking down the problem, it turned out the given position ranges were not even line numbers, but file offsets, so very unhelpful and confusing.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/457615 mentions this issue: go/analysis: improve error message on duplicate fixes

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Feb 3, 2023
@golang golang locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. 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
6 participants
@mdempsky @turbolent @timothy-king @dmitshur @gopherbot @vovean