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

Guard against new positions with line or char less than 1 #800

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zachallaun
Copy link
Collaborator

There was a report in the Elixir Language Discord of an error that occurred during a completion:

Error - 20:03:25] Request textDocument/completion failed.
  Message: ** (FunctionClauseError) no function clause matching in String.slice/3
    (elixir 1.17.2) lib/string.ex:2310: String.slice("    {:ok, assign(socket, form: form, email_empty = t), temporary_assigns: [form: form]}", 0, -1)
    (lx_common 0.5.0) lib/lexical/code_unit.ex:23: LXical.CodeUnit.utf8_position_to_utf16_offset/2
    (lx_protocol 0.5.0) lib/lexical/protocol/conversions.ex:146: LXical.Protocol.Conversions.extract_lsp_character/1
    (lx_protocol 0.5.0) lib/lexical/protocol/conversions.ex:120: LXical.Protocol.Conversions.to_lsp/1
    (lx_protocol 0.5.0) lib/lexical/protocol/conversions.ex:98: LXical.Protocol.Conversions.to_lsp/1
    (lx_protocol 0.5.0) lib/lexical/protocol/convertibles/lexical.document.edit.ex:11: LXical.Convertible.Lexical.Document.Edit.to_lsp/1
    (lx_lexical_shared 0.5.0) lib/lexical/convertible.ex:9: anonymous fn/3 in LXical.Convertible.Helpers.apply/2
    (elixir 1.17.2) lib/enum.ex:4858: Enumerable.List.reduce/3

  Code: -32603

This can only occur when a Lexical.Document.Position is created with :character set to 0. There is therefore an implicit invariant that any positions intended to be sent in a response have a positive character.

This PR makes that invariant explicit by adding a guard to Position.new/3 that will raise of any line/character that is not greater-than-or-equal-to 1.

There were a few places where we were using "utility positions" with line and/or character set to 0, but as far as I can tell, replacing these cases with line/character 1 does not change any behavior.

It is likely that there are still position-related bugs in Lexical, but this should help us catch them closer to where the bug is instead of during conversion to LSP where the stacktrace is pretty useless.

@zachallaun zachallaun requested a review from scohen July 25, 2024 21:23
Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

This will have the temporary effect of making lexical more likely-ish to crash. We should merge this post 0.7 release

@zachallaun zachallaun added this to the 0.8 Release milestone Jul 26, 2024
@zachallaun
Copy link
Collaborator Author

Agreed; added to 0.8 milestone.

@zachallaun zachallaun force-pushed the za-strict-positions branch from 80b5cf1 to 99285e8 Compare July 28, 2024 16:34
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