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.

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

go/analysis/analysistest: RunWithSuggestedFixes cannot handle overlapping fixes (such as ones that add imports) #67049

Open
lfolger opened this issue Apr 26, 2024 · 11 comments
Labels
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

Comments

@lfolger
Copy link
Contributor

lfolger commented Apr 26, 2024

Go version

gc-tip (should reproduce with all versions)

Output of go env in your module/workspace:

N/A

What did you do?

Running the inline analyzer on a package where it would inline multiple functions leads to duplicate suggested fixes that some tools cannot deal with (including the analysistest package). As far as I can tell it tries to remove the import twice. In general it seems to be an issue that it always tries to remove the import even if it is still needed (see example below).

Here is a self contained reproducer test:

func TestRemovingImport(t *testing.T) {
	files := map[string]string{
		"some/package/pkg/foo.go": `package pkg

			// inlineme
			func ToInline () {}

			func Bar () {}
		`,
		"b/c/foo.go": `package c

			import (
				"some/package/pkg"
			)

			func foo() {
				pkg.ToInline() // want "inline call of pkg.ToInline"
			}

			func bar() {
				pkg.ToInline() // want "inline call of pkg.ToInline"
				pkg.Bar() // ok
			}
		`,
		"b/c/foo.go.golden": `package c
			func foo() {
			}

			func bar() {
			}`,
	}
	dir, cleanup, err := analysistest.WriteFiles(files)
	if err != nil {
		t.Fatal(err)
	}
	analysistest.RunWithSuggestedFixes(t, dir, analyzer.Analyzer, "b/c")
	cleanup()
}

What did you see happen?

The example fails with:

...analysistest.go:263: /tmp/analysistest3675113240/src/b/c/foo.go: error applying fixes: diff has overlapping edits (see possible explanations at RunWithSuggestedFixes)

What did you expect to see?

I expected the analyzer to not remove the import when it is still needed and if it is no longer needed to only remove the import once.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Apr 26, 2024
@gopherbot gopherbot added this to the Unreleased milestone Apr 26, 2024
@lfolger
Copy link
Contributor Author

lfolger commented Apr 26, 2024

cc: @adonovan

@adonovan
Copy link
Member

adonovan commented Apr 26, 2024

I plan to improve the API for the inliner so that it includes (for example) options to control literalization, consideration of side effects, and suchlike. I think it might also be useful (and I seem to recall writing a TODO comment to this effect) to return not just one but diff, but a more structured result that separates the diff around the call site from the logical changes to the import declaration. This would also allow Bazel/Blaze to interpose their visibiliity checking.

@adonovan adonovan changed the title x/tools/internal/refactor/inline: analyer generates duplicate diagnostics for removing the import x/tools/internal/refactor/inline: analyzer diffs that remove same import lead to (spurious) conflict Apr 26, 2024
@adonovan
Copy link
Member

We may also need to make the -fix conflict checker more tolerant to redundant but identical diff chunks.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/581802 mentions this issue: internal/refactor/inline: extensible API

@joedian joedian added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 26, 2024
@lfolger
Copy link
Contributor Author

lfolger commented Apr 29, 2024

So what you are saying is that it is not the Analyzer to deduplicate these findings but it is the user of that analyzer to determine this?

I think it is a general problem that you want fixes to be independent of each other so that you can apply them independently?

I don't think this works because following that, you can never generate an import removal unless it is the only usage of that import in the file. Because as soon as there are two or more usages, removing any of them (independently) would require keeping the import because of the other usage.

In other words, the inliner should only ever remove the import if it is removing the only usage of that import. If it is removing something and there is another usage, it should not generate an import removal (even if there are other suggested fixes removing all other usages).

Side note: especially in case where there are usages that are not removed by the inliner, it should not generate a suggested fix to remove the import as it does right now.

@adonovan
Copy link
Member

You raise a number of good questions. You're right that in general two fixes may be safe individually but not together, for example because they each remove the second-last reference to an import. Or that two fixes may spuriously conflict with each other because they each try to remove the same import; a simple conflict resolution strategy would work in this case.

We don't yet have a good calculus for composing fixes. Perhaps analyzers shouldn't even try to solve the goimports problem (since it isn't composable), and the driver's -fix flag should run goimports after any batch of fixes.

In other words, the inliner should only ever remove the import if it is removing the only usage of that import. If it is removing something and there is another usage, it should not generate an import removal (even if there are other suggested fixes removing all other usages).
Side note: especially in case where there are usages that are not removed by the inliner, it should not generate a suggested fix to remove the import as it does right now.

I totally agree, but it sounds like you are describing a plain and simple bug. If the tool prematurely removes an import while there is still an existing use, please file a bug report and I will fix it.

@lfolger
Copy link
Contributor Author

lfolger commented Apr 30, 2024

I totally agree, but it sounds like you are describing a plain and simple bug. If the tool prematurely removes an import while there is still an existing use, please file a bug report and I will fix it.

I thought saw a but while preparing this example and then was to lazy file a separate bug which is why I folded this into this one. You prompted me to look into this again and while I was trying to reproduce this, I noticed that my report here is incorrect.

The duplicate findings are not the import removal but the formatting changes.

To summarize:

  • The inliner does not generate any import removals for the given example. Even when you remove the call to pkg.Bar, it does not remove the import.
  • The inliner produces duplicate suggested fixes to remove the identation. Note that all of the files are not properly indented so that they fit better into the test structure. All lines are indented by three \t and the inline analyzer attempts to remove these \t on each suggested fix.

I'm not sure if this is considered a bug or if the inliner is only supposed to work on files that a formated with gofmt.

Feel free to close this issue if you think this is intended behavior and sorry for the noise.

PS: I think I got confused because the test framework just reports diff has overlapping edits but not what theses diffs are.

gopherbot pushed a commit to golang/tools that referenced this issue May 4, 2024
This CL introduces Options and Result struct types to the
public Inline function so that we can extend the input
and outputs as needed.

Options.IgnoreEffects allows a client to choose to
ignore consideration of potential side effects of
call arguments, an unsound "style optimization".

Result.Literalized reports whether the chosen strategy
was literalization. (Some clients may reject the
transformation in that case.)

A follow-up change will refine the API to separate
the diff at the callsite from the logical diff to
the import declaration.

Updates golang/go#67049

Change-Id: Ifcec19d8cfd18fa797e16c7d6994ac916d77dab5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/581802
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@metonymic-smokey
Copy link

I plan to improve the API for the inliner so that it includes (for example) options to control literalization, consideration of side effects, and suchlike. I think it might also be useful (and I seem to recall writing a TODO comment to this effect) to return not just one but diff, but a more structured result that separates the diff around the call site from the logical changes to the import declaration. This would also allow Bazel/Blaze to interpose their visibiliity checking.

@adonovan is this the TODO you're referring to -
https://cs.opensource.google/go/x/tools/+/master:internal/refactor/inline/inline.go;l=55-58?
I was looking to contribute to the inlining part of the codebase and came across this when I was trying to understand it.

@adonovan
Copy link
Member

adonovan commented Jun 12, 2024

In our meeting today we decided on an agreed interpretation of multiple fixes:

Each Diagnostic has a set of associated SuggestedFixes, each of which are logical alternatives; if there are more than one, the user must choose at most one. Once these choices have been made, we have a set of fixes to apply. Conceptually, all of these fixes are independent changes to the baseline file state, analogous to git commits with the same parent.

To combine them, we must merge them in some order, analogous to a git merge. Each merge may succeed, if the accumulated changes and the latest change do not overlap, or if the overlapping parts are changed in the same way; or it may fail, in which case the user must be informed of the conflict, and the latest change must be rejected. (It's up to the tool whether it proceeds as best it can with the next change, or aborts the whole operation, so long as it reports the error.)

Naturally, the quality of the merge algorithm will determine how robust it is. Our existing conflict logic can de-duplicate identical changes, such as redundant additions of the same import at the same place, but nothing more; by contrast git-mergetool can often successfully resolve more complicated conflicts.

I propose to document this approach at analysis.SuggestedFix, and (eventually) to implement it in checker -fix and in gopls. Though I expect some common logic to resolve a set of fixes will be usefully shared among the various tools, we needn't discuss it here since we don't plan to share it.

The original problem in this thread is that RunWithSuggestedFixes tries to apply all the chosen fixes at once. I hit a related problem this morning in the context of https://go.dev/cl/592155; the solution in that case was to rewrite the test to use a txtar file as the .golden file, with one section per alternative suggested fix, using the description of the fix as the section title. But I don't think that will work here, because in this case we have multiple chosen fixes (not alternatives), all with the same description, and RunWithSuggestedFixes assumes that the descriptions are unique.

There is still a lot of room for improvement of the ergonomics of analysistest. Also, we have four open issues related to the difficulty of using modules in analysis tests:

The situation was improved @timothy-king's recent change to make it easy to extract testdata files from .txtar files, so that go.mod files needn't be part of the source tree. However, that solution is incompatible with the use of .txtar files in RunWithSuggestedFixes, since txtar files do not nest. Definitely more work to do here.

(apologies for prematurely published first draft)

@adonovan adonovan changed the title x/tools/internal/refactor/inline: analyzer diffs that remove same import lead to (spurious) conflict go/analysis/analysistest: RunWithSuggestedFixes cannot handle overlapping fixes (such as ones that add imports) Jun 13, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/592495 mentions this issue: go/analysis: document interpretation of multiple diagnostics

gopherbot pushed a commit to golang/tools that referenced this issue Jun 13, 2024
RunWithSuggestedFixes was written without a clear understanding
of what the semantics of multiple fixes should be. We believe
the only sensible interpretation is the one described here:
golang/go#67049 (comment)

This change documents the interpretation at Diagnostics.SuggestedFixes,
and adds a TODO at RunWithSuggestedFixes to note that its
conflict resolution is problematic, plus a suggested workaround.

A later change will fix RunWithSuggestedFixes, or provide a
replacement if that should prove infeasible.

Updates golang/go#67049

Change-Id: I59d93d77f05e13e2458daa2d9897057ed9f82d06
Reviewed-on: https://go-review.googlesource.com/c/tools/+/592495
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593655 mentions this issue: go/analysis: deal with multiple fixes for same diagnostic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

No branches or pull requests

5 participants