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

x/tools/gopls/internal/analyzer/modernize: bug in slices.Contains transformation #71313

Closed
adonovan opened this issue Jan 17, 2025 · 4 comments
Closed
Assignees
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

[split out of https://github.com//issues/70815#issuecomment-2598663385]

@findleyr says:
I encountered a bug in the slices.Contains modernizer today.

Modernizing using slices.Contains here results in an error:
https://cs.opensource.google/go/x/tools/+/master:gopls/internal/golang/highlight.go;l=348;drc=344e48255740736de8c8277e9a286cf3231c7e13

		case *ast.CallExpr:
			// If cursor is an arg in a callExpr, we don't want control flow highlighting.
			if i > 0 {
				for _, arg := range n.Args {
					if arg == path[i-1] {
						return
					}
				}
			}

Error is: S (type []ast.Expr) does not satisfy ~[]E.
For now, I think we need to check that the types in the match condition are identical.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jan 17, 2025
@gopherbot gopherbot added this to the Unreleased milestone Jan 17, 2025
@adonovan
Copy link
Member Author

Related: we need a means of testing the welltypedness of the transformed code. I'll keep that in mind as I rework the apply-fix logic (which is currently forked between RunWithSuggestedFixes and analysis/checker, but I think deserves a common implementation).

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Jan 17, 2025
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.18.0 Jan 17, 2025
@adonovan
Copy link
Member Author

There's a similar problem in the maps.Copy modernizer if the two maps don't have the same type:

func (fs FolderSettings) set(opts *runConfig) {
	// Re-use the Settings type, for symmetry, but translate back into maps for
	// the editor config.
	folders := make(map[string]map[string]any)
	maps.Copy(folders, fs)  /// oops

	opts.editor.FolderSettings = folders
}

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645856 mentions this issue: gopls/internal/analysis/modernize: fix bug in slicescontains

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/645876 mentions this issue: gopls/internal/analysis/modernize: mapsloop: fix two bugs

gopherbot pushed a commit to golang/tools that referenced this issue Feb 6, 2025
As with slices.Contains, there was a bug in the maps.Copy
modernizer resulting from not checking for implicit widening
conversions in m[k]=v; and another, from not checking that m
is a map. This CL fixes both.

Modernizers are hard. :(

Fixes golang/go#71313

Change-Id: Ie59aa9419868b3010435b7113bb5e67f0abbb4d3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/645876
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants