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

Jump to bottom

x/tools/gopls: safetoken.Offset fails on a Stmt.End value from the parser #57484

Closed
adonovan opened this issue Dec 27, 2022 · 6 comments
Closed

x/tools/gopls: safetoken.Offset fails on a Stmt.End value from the parser #57484

adonovan opened this issue Dec 27, 2022 · 6 comments
Assignees
@adonovan
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.12.0

Comments

@adonovan
Copy link
Member

adonovan commented Dec 27, 2022 •
edited

@findleyr reported this error message at tip:

A bug occurred on the server: internal error converting diagnostic from analyzer unreachable:
in SuggestedFixes: end: pos 22988 is not in range for file [3697:22987)

It means that a SuggestedFix emitted by the unreachable analyzer had an end position that was beyond the end of the token.File, by exactly one byte. (The safetoken error message is wrong: it should use a ] to indicate an inclusive interval.)
The unreachable analyzer seems fine: the Start and End positions both come from a Stmt, which should be within the file. The logic in posToLocation also looks sound.

Needs investigating. Also, we should get the safetoken error to include the correct interval, and report the filename.
@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 Dec 27, 2022
@gopherbot gopherbot added this to the Unreleased milestone Dec 27, 2022
@adonovan adonovan changed the title x/tools/gopls: x/tools/gopls: strange safetoken.Offset error message in new analysis driver Dec 27, 2022
@adonovan adonovan self-assigned this Dec 27, 2022
@gopherbot
Copy link

Change https://go.dev/cl/459637 mentions this issue: gopls/internal/lsp/safetoken: fix error message

@rfindley
Copy link

@adonovan,
I think your message is intended for @findleyr (rather than @rfindley)

gopherbot pushed a commit to golang/tools that referenced this issue Dec 28, 2022
@adonovan
gopls/internal/lsp/safetoken: fix error message
ef1ec5d 
This change uses [x:y] notation in the error message to indicate
an inclusive interval, which is what the logic actually implements.
We also include the file name.

Also, put the inequalities in lo <= x <= hi form for clarity,
and inline/simplify the token.File.Pos/Offset calls to avoid
the redundant double-check.

Updates golang/go#57484

Change-Id: I63e10d0e6659aae2613b5b7d51e87a8a4bfb225d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459637
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@adonovan
Copy link
Member Author

adonovan commented Dec 28, 2022 •
edited

I suspect the cause is a discrepancy in the length of the file as reported by ast.File.{Pos,End} and by token.File.Size.

See this program for an example:

	src := `package p; func f() { var x struct`
	fset := token.NewFileSet()
	file, _ := parser.ParseFile(fset, "", src, 0)
	tf := fset.File(file.Pos())

	fmt.Printf("file.Pos,End=(%d,%d) End-Pos=%d tf.Size=%d len(src)=%d\n",
		file.Pos(), file.End(), file.End()-file.Pos(), tf.Size(), len(src))

	fn := file.Decls[0].(*ast.FuncDecl)
	stmt := fn.Body.List[0]
	fmt.Printf("stmt.Pos,End=(%d,%d)\n", stmt.Pos(), stmt.End())

Output:

file.Pos,End=(1,36) End-Pos=35 tf.Size=34 len(src)=34
stmt.Pos,End=(23,36)

Observe that the File syntax node reports a length of 35 bytes, including the implicit newline at EOF, whereas the token.File reports only 34 bytes, the true length of the input. So, when the parser sees an incomplete statement such as the struct type here, it performs recovery, effectively synthesizing the LPAREN and RPAREN at the EOF position, which is beyond the token.File size, thus causing the End position of any of the incomplete nodes (StructType, AssignStmt, BlockStmt, FuncDecl) to be invalid as an argument to safetoken.Offset as currently implemented.

The parser source already flags this risk in the some cases (such as ast.Bad{Decl,Stmt,Expr}), as witnessed by this comment, but it seems the problem is more pervasive.

@griesemer

@adonovan adonovan changed the title x/tools/gopls: strange safetoken.Offset error message in new analysis driver x/tools/gopls: safetoken.Offset fails on a Stmt.End value from the parser Dec 28, 2022
@suzmue suzmue modified the milestones: Unreleased, gopls/v0.12.0 Dec 28, 2022
@adonovan
Copy link
Member Author

adonovan commented Dec 28, 2022 •
edited

I think this is a bug in the parser, since it creates Pos values that are beyond the block that it reserves in a call to FileSet.AddFile. AddFile is very tightly specified and can't be changed, but I think we could make the parser reserve one extra byte of space for EOF without breaking anything. We can also add a workaround in safetoken.Offset.

I've reported the parser bug separately as #57490. This issue can be about gopls.

@adonovan adonovan mentioned this issue Dec 28, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/459735 mentions this issue: gopls/internal/lsp/safetoken: fix bug in Offset at EOF

@gopherbot
Copy link

Change https://go.dev/cl/459736 mentions this issue: gopls/internal/lsp/safetoken: funnel more calls through this package

gopherbot pushed a commit to golang/tools that referenced this issue Dec 29, 2022
@adonovan @gopherbot
gopls/internal/lsp/safetoken: funnel more calls through this package
81e741e 
This change expands the dominion of safetoken to include calls to
token.File{,Set}.Position{,For}, since all need workarounds similar
to that in Offset. As a side benefit, we now have a centralized place
to implement the workaround for other bugs such as golang/go#41029,
the newline at EOF problem).

Unfortunately the former callers of FileSet.Position must stipulate
whether the Pos is a start or an end, as the same value may denote
the position 1 beyond the end of one file, or the start of the
following file in the file set. Hence the two different functions,
{Start,End}Position.

The static check has been expanded, as have the tests.

Of course, this approach is not foolproof: gopls has many dependencies
that call methods of File and FileSet directly.

Updates golang/go#41029
Updates golang/go#57484
Updates golang/go#57490

Change-Id: Ia727e3f55ca2703dd17ff2cac05e786793ca38eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/459736
Run-TryBot: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
@adonovan adonovan mentioned this issue Apr 7, 2023
Closed
@golang golang locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FrozenDueToAge 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
Milestone
gopls/v0.12.0
Development

No branches or pull requests
4 participants
@rfindley @adonovan @suzmue @gopherbot