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/gopls: analysis driver spuriously reports invalid suggested fixes due to AST repair #71659

Open
adonovan opened this issue Feb 11, 2025 · 14 comments
Open

x/tools/gopls: analysis driver spuriously reports invalid suggested fixes due to AST repair #71659

adonovan opened this issue Feb 11, 2025 · 14 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls/telemetry-wins gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.18.2

Comments

@adonovan
Copy link
Member

adonovan commented Feb 11, 2025 •
edited
Loading 

#!stacks
"bug.Reportf" && "cache.(*action).exec.func3:+3" &&
  "unreachable.(*deadState).findDead:+17"

Issue created by stacks.

This stack indicates that the unreachable analyzer violates the SuggestedFix preconditions now documented and checked by the gopls analysis driver.

This stack Gf7EXQ was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.5 windows/amd64 vscode (1)

Dups: EmaIvQ KznUPA JuVvTw 10HCWA mAqhog S-mQmQ
@adonovan adonovan added gopls Issues related to the Go language server, gopls. gopls/telemetry-wins NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository. labels Feb 11, 2025
@adonovan adonovan added this to the gopls/v0.18.0 milestone Feb 11, 2025
@adonovan adonovan mentioned this issue Feb 11, 2025
Open
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 11, 2025
@prattmic
Copy link
Member

prattmic commented Feb 11, 2025 •
edited
Loading

In v0.17.1, line 1143 was the third line of cache.(*action).exec.func3 (but is not a call to Reportf). Did stacks fetch the wrong version of the source?

https://cs.opensource.google/go/x/tools/+/refs/tags/gopls/v0.17.1:gopls/internal/cache/analysis.go;l=1143

Edit: oops, meant to post this on #71660.

@adonovan
Copy link
Member Author

adonovan commented Feb 11, 2025 •
edited
Loading

Yes, I think a version skew is possible since the line numbers are not always off by one by some variable amount, which could correspond to recent changes in the code.

@adonovan
Copy link
Member Author

adonovan commented Feb 11, 2025 •
edited
Loading

This stack indicates that the unreachable analyzer violates the SuggestedFix preconditions now documented and checked by the gopls analysis driver.

This call to pass.Report has a diagnostic with a single SuggestedFix of a single TextEdit (a deletion). The various possible errors returned by validateFix are:

  • "missing file info for pos (%v)" (File(start) == nil)
  • "pos (%v) > end (%v)" (end < start)
  • "malformed end position %v" (File(end) == nil, end))
  • "edit spans files %v and %v" (endFile != file)
  • "end is (%v) beyond end of file (%v)" (edit.End > file.Base() + file.Size()))

("overlapping edits" is not possible, nor is "analyzer %q suggests two fixes with same Message (%s)")

This means that at least one of stmt.Pos(), stmt.End() is invalid (zero, beyond EOF, or not mapped to a token.File). stmt is an element of ast.BlockStmt.List. The logic in the unreachable analyzer is unimpeachable, so I think either the parser recovery or the AST fixing logic must be to blame. Perhaps the input file is incomplete, something like func _() {return; var _ struct (EOF), and the parser's recovery has filled in values for the missing braces that are invalid or beyond EOF. This is especially likely if the Stmt.End() is computed by addition of a constant to the pos of some actual token.

@adonovan
Copy link
Member Author

Indeed, some variant of func _() {return; var _ struct did reproduce the bug.Report, but unfortunately the failure is silent to the user--the fix is discarded--so I had to check gopls bug to see it... but I lost the actual input as I tried many in a short span of time.

Nonetheless, it confirms that it's a truncated file AST repair issue, in other words another variant of #66790 (comment) (which is perhaps also the root cause of #64547)

@adonovan adonovan changed the title x/tools/go/analysis/passes/unreachable: invalid suggested fix x/tools/gopls: analysis driver spuriously reports invalid suggested fixes due to AST repair Feb 11, 2025
@adonovan adonovan mentioned this issue Feb 11, 2025
Open
@findleyr findleyr modified the milestones: gopls/v0.18.0, gopls/v0.18.1 Feb 12, 2025
@adonovan
Copy link
Member Author

This stack KznUPA was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 darwin/arm64 vscode (1)

@adonovan
Copy link
Member Author

This stack JuVvTw was reported by telemetry:

golang.org/x/tools/[email protected] go1.24rc3 darwin/arm64 sublimetext (1)

This stack 10HCWA was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 darwin/arm64 vscode,vscode-insiders (1)

@adonovan
Copy link
Member Author

This stack EmaIvQ was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 linux/amd64 vscode (1)

@gabyhelp gabyhelp mentioned this issue Feb 18, 2025
Open
@adonovan
Copy link
Member Author

This stack KznUPA was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.4 darwin/amd64 vscode (1)

This stack JuVvTw was reported by telemetry:

golang.org/x/tools/[email protected] go1.24.0 darwin/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24.0 windows/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24.0 linux/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24rc3 darwin/arm64 sublimetext (1)

@adonovan
Copy link
Member Author

This stack EmaIvQ was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 linux/amd64 vscode (1)

This stack KznUPA was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.4 darwin/amd64 vscode (1)

This stack JuVvTw was reported by telemetry:

golang.org/x/tools/[email protected] go1.24.0 windows/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24.0 linux/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24rc3 darwin/arm64 sublimetext (1)
golang.org/x/tools/[email protected] go1.24.0 darwin/amd64 vscode (1)

@adonovan
Copy link
Member Author

This stack 10HCWA was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 linux/amd64 other,vscode (1)
golang.org/x/tools/[email protected] go1.23.6 darwin/arm64 vscode (1)
golang.org/x/tools/[email protected] go1.23.5 windows/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.23.6 darwin/arm64 other,vscode (1)

@adonovan
Copy link
Member Author

adonovan commented Feb 20, 2025 •
edited by gabyhelp
Loading

Repro, found while investigating https://go.dev/cl/638395, which changes the parser behavior. Nonetheless it's pretty clear what class of errors this represents:

The missing label in the goto statement is currently not a parse error; but with CL 638395, it is an error, and it causes the parser to consume the }, which creates a cascade of errors. The FuncLit body is never closed, so the BlockStmt.End is computed to be (one) beyond the end of the file.

var _ = func() {
	goto
}

func _() int {
	switch {
	case 1:
		println()
	}
	println()
}

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/651155 mentions this issue: go/analysis/passes/unreachable/testdata: relax test for CL 638395

@findleyr findleyr modified the milestones: gopls/v0.18.1, gopls/v0.18.2 Feb 21, 2025
@adonovan
Copy link
Member Author

This stack mAqhog was reported by telemetry:

golang.org/x/tools/[email protected] go1.24.0 linux/amd64 vscode (1)
golang.org/x/tools/[email protected] go1.24.0 darwin/arm64 vscode (1)

This stack S-mQmQ was reported by telemetry:

golang.org/x/tools/[email protected] go1.23.6 darwin/arm64 vscode (1)

gopherbot pushed a commit to golang/tools that referenced this issue Feb 24, 2025
@adonovan @gopherbot
go/analysis/passes/unreachable/testdata: relax test for CL 638395
d2fcd36 
This test case is about to become a parse error. To allow us
to submit the change to the parser, we must relax this test.

Updates golang/go#71659
Updates golang/go#70957

Change-Id: Ic4fbfedb69d152d691dec41a94bb402149463f84
Reviewed-on: https://go-review.googlesource.com/c/tools/+/651155
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BugReport Issues describing a possible bug in the Go implementation. gopls/telemetry-wins gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.18.2
Development

No branches or pull requests
5 participants
@prattmic @adonovan @gopherbot @findleyr @gabyhelp