Skip to content

Make source_location() and SourceFile's and line_starts consistent when the last source byte is a newline #552

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

Merged
merged 1 commit into from
May 18, 2025

Conversation

mlechu
Copy link
Member

@mlechu mlechu commented May 13, 2025

When constructing SourceFile's line_starts, we currently pretend there's a
newline at the end of the source if there isn't one, and then ignore the last
newline unconditionally. This became a problem in LSP testing, where calling
source_location with a cursor position at the end of the file would give a
wrong result only if the last character was a newline.

julia> source_location(SourceFile("\n\n\n"), 1)
(1, 1)

julia> source_location(SourceFile("\n\n\n"), 2)
(2, 1)

julia> source_location(SourceFile("\n\n\n"), 3)
(3, 1)

julia> source_location(SourceFile("\n\n\n"), 4)
(3, 2)

This change removes the conditional line and the code stripping it. This also
brings line_starts closer to its definition (one entry per line in the file).

Alternatively, we could add the dummy line unconditionally. It would make for
calculating the length of the last real line easier, but would make
line_starts less explainable.

when the last source byte is a newline
Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

This behavior feels more natural for what source_location returns, and it also seems like the implementation gets a bit simpler as a result.

mlechu added a commit to mlechu/JETLS.jl that referenced this pull request May 13, 2025
Note that the "one past the last byte" tests need
    JuliaLang/JuliaSyntax.jl#552 to pass
@aviatesk
Copy link
Member

@c42f I'm hoping to move this PR forward and release a new patch version soon. Could you give us some comments on this if you have any?

@aviatesk aviatesk merged commit 58082d9 into JuliaLang:main May 18, 2025
35 checks passed
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.

2 participants