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

Fix extra newline being added when formatting document #525

Merged
merged 4 commits into from Apr 16, 2018

Conversation

Projects
None yet
4 participants
@bubba
Collaborator

bubba commented Apr 13, 2018

Fixes #521

@@ -44,7 +46,8 @@ brittanyCmd tabSize uri range =
l = length textLines
c = T.length $ last textLines
textLines = T.lines text
textEdit = J.TextEdit (Range startPos endPos) newText
newTextWithoutNewline = T.init newText -- get rid of extraneous \n

This comment has been minimized.

@wz1000

wz1000 Apr 13, 2018

Collaborator

Wouldn't it be a better idea to remove all trailing newlines specifically instead of blindly truncating the text?

This comment has been minimized.

@bubba

bubba Apr 13, 2018

Collaborator

Probably! Brittany seems to only remove the first trailing newline though, so should we try to match this?

@lspitzner

This comment has been minimized.

Contributor

lspitzner commented Apr 13, 2018

The use of T.lines above looks suspicious to me, btw. Afaict, T.lines interprets "newline" as purely "\n" and leaves "\r" in place. I have not considered/tested the exact consequences of this, but it seems not all unlikely to be behind this newline bug and also potentially other newline insertion problems that were reported for HIE.

@bubba Unrelated: Under what circumstances does brittany remove trailing newlines?

@bubba

This comment has been minimized.

Collaborator

bubba commented Apr 13, 2018

@lspitzner I was mistaken, brittany doesn't seem to remove newlines at all.
I also checked T.lines for carriage return and you're right:

Prelude T> x = T.pack "hello\r\nworld"
Prelude T> T.lines x
["hello\r","world"]

But this bug happens regardless of the line encoding used.
I think the cause of this one is actually coming from T.readFile which adds on an extra '\n':

> cat test
hello world
this is a test with no newline
Prelude T> readFile "test"
"hello world\nthis is a test with no newline\n"
@alanz

This comment has been minimized.

Collaborator

alanz commented Apr 14, 2018

@lspitzner what do we do with this PR?

@lspitzner

This comment has been minimized.

Contributor

lspitzner commented Apr 14, 2018

I cannot reproduce the issue with readFile:

> :!hexdump -C test.txt
00000000  66 69 72 73 74 0d 0a 73  65 63 6f 6e 64           |first..second|
0000000d
> TIO.readFile "test.txt"
"first\r\nsecond"

I also point out that it is really hard to tell from

> cat test
hello world
this is a test with no newline

whether the file actually terminates with a newline. It really should look like this:

> cat test
hello world
this is a test with no newline> [cursor here]
@lspitzner

This comment has been minimized.

Contributor

lspitzner commented Apr 14, 2018

As this might be system/locale dependent, I should add that I have tested on linux only, with "\n" and "\r\n". @bubba what environment are you seeing this in?

@bubba

This comment has been minimized.

Collaborator

bubba commented Apr 15, 2018

Sorry, disregard what I said about T.readFile! I was naïve to think that echoing to a file wouldn't add a newline. I'm on macOS 10.13. I will try to figure out more about what is exactly causing this.

@bubba bubba closed this Apr 15, 2018

@bubba

This comment has been minimized.

Collaborator

bubba commented Apr 15, 2018

What I'm now convinced was happening is that the range of text being replaced didn't include the last newline character, which meant that it stayed around after the formatted text was inserted, adding an extra line. From the specification:

If you want to specify a range that contains a line including the line ending character(s) then use an end position denoting the start of the next line.

Although I found that the end range column needs to be past zero, contradictory to above. This was tested on VS Code.

@bubba bubba reopened this Apr 15, 2018

@wz1000

This comment has been minimized.

Collaborator

wz1000 commented Apr 15, 2018

The Position type is zero indexed(as specified by LSP), but the tuple (Int,Int) is assumed to always be one indexed in HIE(as GHC reports positions as one indexed tuples). toPos :: (Int, Int) -> Position converts between these two representations. So

toPos (1,1) == Position 0 0

The mixing of types here is unfortunate(probably my fault, and how this bug originated). It would be better to use one of these position types consistently here.

Otherwise it looks good.

@bubba

This comment has been minimized.

Collaborator

bubba commented Apr 15, 2018

@wz1000 It all makes sense now, pushed a change for that!

@alanz

This comment has been minimized.

Collaborator

alanz commented Apr 16, 2018

@wz1000 when you are happy, please merge this.

@wz1000 wz1000 merged commit 676feb9 into haskell:master Apr 16, 2018

3 checks passed

ci/circleci: ghc-8.0.2 Your tests passed on CircleCI!
Details
ci/circleci: ghc-8.2.1 Your tests passed on CircleCI!
Details
ci/circleci: ghc-8.2.2 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment