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

Fix range formatting #1190

Merged
merged 20 commits into from
Nov 16, 2022
Merged

Fix range formatting #1190

merged 20 commits into from
Nov 16, 2022

Conversation

mroavi
Copy link
Contributor

@mroavi mroavi commented Oct 31, 2022

Fixes #1180, #1187, #1188, and probably #1184

Here is the idea:

  1. We take as input the entire file and the range that should be formatted.
  2. We enclose the given range by injecting two markers in the form of comments to the input file.
  3. We format the result of step 2 (entire file with markers) using JuliaFormatter.jl. The output should include the injected markers.
  4. We replace the given range in the input file with the code inside the markers of the output of step 3, and return this.

This approach avoids the need of having to deal with EXPRs and formats the given range in the context of the entire file.

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM and works very nicely. I've left a few comments inline.

src/requests/features.jl Outdated Show resolved Hide resolved
src/requests/features.jl Outdated Show resolved Hide resolved
src/requests/features.jl Outdated Show resolved Hide resolved
src/requests/features.jl Outdated Show resolved Hide resolved
src/requests/features.jl Outdated Show resolved Hide resolved
@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

After the last changes, this case stopped working:

:lua vim.lsp.buf.format({ range = { ['start'] = {1,0}, ['end'] = {6,0} } })

Input:

function printx(x)

println("x = $x")
return

end

Output:

function printx(x)

  println("x = $x")
  return

endend

Since now we are only replacing the text inside the given range, it is more difficult to test because we cannot specify how the entire file has to look after formatting (see c05eaa7). This case is not caught by any of the tests because the returned formatted text is correct. However, it is misplaced in the input file. Any idea how we could make these tests more robust? I will look into why this case fails later this week.

@fredrikekre
Copy link
Member

the returned formatted text is correct

Where does the extra end come from then?

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

the returned formatted text is correct

Where does the extra end come from then?

The original one is not copied over, so we end up with two of them. We are probably making a wrong assumption about the presence of a newline char at the end of the given range. I noticed that if you add an empty line at the end of the file, then it works fine.

@fredrikekre
Copy link
Member

What do you mean with copied over? Sounds like a mismatch in how you mark what is going to be formatted then? The end is included in the text to be formatted, but excluded from the range of replacement?

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

What do you mean with copied over? Sounds like a mismatch in how you mark what is going to be formatted then? The end is included in the text to be formatted, but excluded from the range of replacement?

I mean that the end should be overwritten with the formatted end, which in this case looks exactly the same. I'm not sure where the issue comes from. It could be indeed an issue with how the range is marked. It could also be that the marking is being done properly but not the way in which we are replacing the text within the marks with the formatted text. I'll take a closer look today when I get home.

@fredrikekre
Copy link
Member

I mean that since you insert regular line comments you must mark full lines to format, and then you must also replace the full line, not the original range, right?

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

I mean that since you insert regular line comments you must mark full lines to format

Yes.

and then you must also replace the full line

we need to replace the introduced marks (comments) and all the lines inside the range.

We use this regex to do that:

https://github.com/mroavi/LanguageServer.jl/blob/1a12fc86426d8d37bcb554a8f2244ed32486793d/src/requests/features.jl#L214-L216

I see a \n in there which might be the issue. I think it should be optional in order to cover this case.

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

And this is the other part that we modified:

https://github.com/mroavi/LanguageServer.jl/blob/1a12fc86426d8d37bcb554a8f2244ed32486793d/src/requests/features.jl#L220

which could also be the source of the problem. I don't have the laptop configured to debug LS.jl at the moment. I'll post an update as soon as I can test this a bit more.

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

I'm sorry. This was all noise. The range formatting is working fine. I think I accidentally typed the undo button and disregarded the fix made here a765897. 😳

@fredrikekre
Copy link
Member

Yea I couldn't reproduce. I played around a bit with the PR, is it OK if I push some changes to hopefully make it more resilient towards different lineendings?

@mroavi
Copy link
Contributor Author

mroavi commented Nov 14, 2022

Please do so!

src/requests/features.jl Outdated Show resolved Hide resolved
@fredrikekre fredrikekre merged commit 9ace567 into julia-vscode:master Nov 16, 2022
@mroavi mroavi deleted the fix-range-formatting branch November 16, 2022 14:06
@fredrikekre fredrikekre mentioned this pull request Jul 18, 2023
@HusterRichard
Copy link

When there is an empty line at the end of the code, the format selection operation will be performed. There will be the following error:
ERROR: BoundsError: attempt to access 3-element Vector{String} at index [1:4]
Stacktrace:
[1] throw_boundserror(A::Vector{String}, I::Tuple{UnitRange{Int64}})
@ Base .\abstractarray.jl:691
[2] checkbounds
@ .\abstractarray.jl:656 [inlined]
[3] view
@ .\subarray.jl:177 [inlined]
[4] textDocument_range_formatting_request(params::LanguageServer.DocumentRangeFormattingParams, server::LanguageServerInstance, conn::JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint})
@ LanguageServer c:\Users\admin.vscode\extensions\julialang.language-julia-1.47.2\scripts\packages\LanguageServer\src\requests\features.jl:198
[5] (::LanguageServer.var"#111#112"{typeof(LanguageServer.textDocument_range_formatting_request), LanguageServerInstance})(conn::JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, params::LanguageServer.DocumentRangeFormattingParams)
@ LanguageServer c:\Users\admin.vscode\extensions\julialang.language-julia-1.47.2\scripts\packages\LanguageServer\src\languageserverinstance.jl:267
[6] dispatch_msg(x::JSONRPC.JSONRPCEndpoint{Base.PipeEndpoint, Base.PipeEndpoint}, dispatcher::JSONRPC.MsgDispatcher, msg::Dict{String, Any})
@ JSONRPC c:\Users\admin.vscode\extensions\julialang.language-julia-1.47.2\scripts\packages\JSONRPC\src\typed.jl:67
[7] run(server::LanguageServerInstance)
@ LanguageServer c:\Users\admin.vscode\extensions\julialang.language-julia-1.47.2\scripts\packages\LanguageServer\src\languageserverinstance.jl:387
[8] top-level scope
@ c:\Users\admin.vscode\extensions\julialang.language-julia-1.47.2\scripts\languageserver\main.jl:104

@fredrikekre
Copy link
Member

Fixed by #1228?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Range formatting doesn't work with (0,0) as the initial position
4 participants