lexical-lsp / lexical

Lexical is a next-generation elixir language server
888 stars 82 forks source link

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

Open zachallaun opened 3 months ago

zachallaun commented 3 months ago

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 commented 3 months ago

Agreed; added to 0.8 milestone.