Fix position format

[andreicek]

This was discovered by using helix, code editor. For reference the issue is here: helix-editor/helix#2029.

In the Position documentation it says:

Position in a text document expressed as zero-based line and zero-based character offset. A position is between two characters like an ‘insert’ cursor in an editor. Special values like for example -1 to denote the end of a line are not supported.

So this is binding it to be 0 if the position - 1 returns less.

I'm not sure this is the only place that the change needs to be done, so I'm open to some guidance so we can make this work as it should.

Thanks!

[lukaszsamson]

I'm against meging it in this form as it hides the real bug (why position is 0 when it is 1 based).
Furthermore, this is far from covering all the places where it can return invalid results. In fact helix-editor/helix#2029 (comment) suggests that character can be also negative.
@the-mikedavis @andreicek do you have a repro (or a full LSP protocol message dump) for that issue?

[the-mikedavis]

I managed to reproduce the -1 line-number in the test suite: 5b0dabc. I'll see if I can come up with a fix 👍

[the-mikedavis]

It looks like the build failure diagnostic is

%Mix.Task.Compiler.Diagnostic{
  compiler_name: "Elixir",
  details: nil,
  file:
    "/home/michael/dev-tools/code/elixir_ls/apps/elixir_ls_utils/.tmp/ElixirLS.LanguageServer.ServerTest/test reports ArgumentError in build diagnostics/lib/argument_error_in_compile.ex",
  message:
    "(ArgumentError) each element in tuple list has to be a {function_name :: atom, arity :: 0..255} tuple, got: [hello: 0]\n\nStacktrace:\n  │ (elixir 1.13.4) lib/module.ex:1325: anonymous fn/2 in Module.make_overridable/2\n  │ (stdlib 3.17) lists.erl:1342: :lists.foreach/2\n  │ lib/argument_error_in_compile.ex:4: (module)\n  │ (elixir 1.13.4) lib/kernel/parallel_compiler.ex:346: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7",
  position: 0,
  severity: :error
}

It looks like that diagnostic is returned directly from the mix compile task:

elixir-ls/apps/language_server/lib/language_server/build.ex

Line 200 in ae40661

case Mix.Task.run("compile", ["--return-errors", "--ignore-module-conflict"]) do

So I think clamping -1 to 0 as in this PR makes sense. What do you think?

[lukaszsamson]

So I think clamping -1 to 0 as in this PR makes sense. What do you think?

It may be but nevertheless it's worth checking that 0. If it comes from elixirr compiler then we should report it upstream.

[lukaszsamson]

It seems 0 is a legit response, see https://hexdocs.pm/mix/1.13.4/Mix.Task.Compiler.Diagnostic.html#t:position/0

[andreicek]

Thanks @lukaszsamson for taking this on!

[lukaszsamson]

Thanks @andreicek for the initial work but the problem turned out more complicated