Skip to content
This repository was archived by the owner on Aug 30, 2025. It is now read-only.

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

Open
zachallaun wants to merge 1 commit into
mainfrom
za-strict-positions
Open

Guard against new positions with line or char less than 1#800
zachallaun wants to merge 1 commit into
mainfrom
za-strict-positions

Conversation

@zachallaun

Copy link
Copy Markdown
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

@scohen scohen left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants