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: strange unified diff #60379

Closed
adonovan opened this issue May 24, 2023 · 4 comments
Closed

x/tools/gopls: strange unified diff #60379

adonovan opened this issue May 24, 2023 · 4 comments
Assignees
@pjweinb
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.14.0

Comments

@adonovan
Copy link
Member

The unified diff output gopls format includes a \ No newline at end of file line in the middle of the file. Is that valid?

xtools$ git log | head -n 1
commit 7a03febeeead7497d3821eb99461ecfeae51c0ba

xtools$ cat a/a.go 
package a

type S struct {
s fmt.Stringer
}
xtools$ go run ./gopls format -d ./a/a.go
--- /Users/adonovan/w/xtools/a/a.go.orig
+++ /Users/adonovan/w/xtools/a/a.go
@@ -1,5 +1,6 @@
 package a
 
 type S struct {
+	
\ No newline at end of file
 s fmt.Stringer
 }

xtools$ go run ./gopls format -w ./a/a.go

xtools$ cat ./a/a.go 
package a

type S struct {
	s fmt.Stringer
}
@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 May 24, 2023
@gopherbot gopherbot added this to the Unreleased milestone May 24, 2023
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.13.0 May 24, 2023
@pjweinb
Copy link

pjweinb commented May 27, 2023 via email

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/498975 mentions this issue: internal/diff: fix lineEdits

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499377 mentions this issue: internal/diff: fix LineEdits bug in slow path

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/499376 mentions this issue: internal/diff: fix LineEdits bug in fast path

gopherbot pushed a commit to golang/tools that referenced this issue Jun 1, 2023
@adonovan
internal/diff: fix LineEdits bug in slow path
0b4461b 
Previously, the expandEdit operation would expand to
the end of the line unconditionally, but this caused it
to gulp an extra line if it was already line-aligned.

This change causes it to do the expansion only if
the end is not line-aligned, or the replacement text
doesn't end with a newline.

Now, removing the fast path no longer causes tests to fail.
This also allows us to remove the logic added in CL 489695
to work around issue golang/go#59232.

Fixes golang/go#60379
Fixes golang/go#59232

Change-Id: Ia40e4e3bb714d75acb95103a38e8c49a8ef456de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499377
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
@golang golang locked and limited conversation to collaborators May 31, 2024
apstndb pushed a commit to apstndb/gotoolsdiff that referenced this issue Jan 11, 2025
@adonovan
internal/diff: fix LineEdits bug in fast path
7d36289 
The fast-path "optimization" that skips the main
algorithm when the input is already line-aligned
failed to check that the replacement text consisted
of complete lines.

(Scare quotes because removing the "optimization" causes
tests to fail. See CL 499377 next in stack for why.)

Thanks to pjw for diagnosing the root cause and
providing the test case in CL 498975.

Fixes golang/go#60379

Change-Id: I2ff92de4550754691442362b8a8932ee42971461
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499376
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
apstndb pushed a commit to apstndb/gotoolsdiff that referenced this issue Jan 11, 2025
@adonovan
internal/diff: fix LineEdits bug in slow path
e37d6d8 
Previously, the expandEdit operation would expand to
the end of the line unconditionally, but this caused it
to gulp an extra line if it was already line-aligned.

This change causes it to do the expansion only if
the end is not line-aligned, or the replacement text
doesn't end with a newline.

Now, removing the fast path no longer causes tests to fail.
This also allows us to remove the logic added in CL 489695
to work around issue golang/go#59232.

Fixes golang/go#60379
Fixes golang/go#59232

Change-Id: Ia40e4e3bb714d75acb95103a38e8c49a8ef456de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499377
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@pjweinb pjweinb

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.14.0
Development

No branches or pull requests
4 participants
@pjweinb @adonovan @gopherbot @findleyr