Column positions in diagnostics are off by one

[lukaszsamson]

Elixir and Erlang/OTP versions

Erlang/OTP 26 [erts-14.1.1] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]

Elixir 1.16.0-rc.1 (81a12b7) (compiled with Erlang/OTP 26)

Operating system

macOS

Current behavior

The documentation and typespec for https://hexdocs.pm/mix/1.15.7/Mix.Task.Compiler.Diagnostic.html#t:position/0 states that line is 1-based and column is 0-based
In case of https://hexdocs.pm/elixir/Code.html#t:position/0 it's not documented but the typespec is exactly the same.

However, when I try it, the APIs return 1-based columns for both console prints and diagnostics

Here I expect the position to column to be {1, 0} (though we can argue wether the error actually is on 0 or 1 column)

%Mix.Task.Compiler.Diagnostic{
  file: "lib/a.ex",
  severity: :error,
  message: "** (TokenMissingError) token missing on lib/a.ex:1:2:\n    error: missing terminator: ]\n    │\n  1 │ [\n    │ │└ missing closing delimiter (expected \"]\")\n    │ └ unclosed delimiter\n    │\n    └─ lib/a.ex:1:2\n    (elixir 1.16.0-rc.1) lib/kernel/parallel_compiler.ex:428: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/8\n",
  position: {1, 1},
  compiler_name: "Elixir",
  span: nil,
  details: %TokenMissingError{
    file: "lib/a.ex",
    line: 1,
    column: 1,
    end_line: 2,
    line_offset: 0,
    snippet: "[\n",
    opening_delimiter: :"[",
    description: "missing terminator: ]"
  },
  stacktrace: [
    {:elixir_compiler, :string, 3, [file: ~c"src/elixir_compiler.erl", line: 7]},
    {Kernel.ParallelCompiler, :"-spawn_workers/8-fun-0-", 5,
     [file: ~c"lib/kernel/parallel_compiler.ex", line: 428]}
  ]
}

iex(3)> try do
...(3)> Code.with_diagnostics([log: true], fn -> Code.compile_string("[") end)
...(3)> rescue
...(3)> e -> e
...(3)> end

** (TokenMissingError) token missing on nofile:1:2:
    error: missing terminator: ]
    │
  1 │ [
    │ │└ missing closing delimiter (expected "]")
    │ └ unclosed delimiter
    │
    └─ nofile:1:2
    (elixir 1.16.0-rc.1) src/elixir_compiler.erl:7: :elixir_compiler.string/3
    iex:3: (file)
    iex:3: (file)

%TokenMissingError{
  file: "nofile",
  line: 1,
  column: 1,
  end_line: 1,
  line_offset: 0,
  snippet: "[",
  opening_delimiter: :"[",
  description: "missing terminator: ]"
}

Here the situation is clear - I expect {1, 0} and based on typespec from https://hexdocs.pm/elixir/main/Code.html#t:diagnostic/1 I expect span to be {1, 3}

iex(2)> Code.with_diagnostics([log: true], fn -> Code.compile_string("asd = 0") end)
warning: variable "asd" is unused (if the variable is not meant to be used, prefix it with an underscore)
└─ nofile:1:1

{[],
 [
   %{
     message: "variable \"asd\" is unused (if the variable is not meant to be used, prefix it with an underscore)",
     position: {1, 1},
     file: "nofile",
     stacktrace: [],
     span: {1, 4},
     severity: :warning
   }
 ]}

Expected behavior

All this is really inconsistent and confusing. Some APIs (e.g. Code.Fragment) use 1-based columns, some use 0-based, some does not work as documented. I'm not sure what's the way forwards.

[josevalim]

Actually, I see what happened. When the Task.Diagnostic was added, we didn't have any column information in Elixir, so it was probably a specification of how it would look like, but internally the compiler was always 1-based. It is similar to the four element tuple inside the Task.Diagnostic, but that's never used anywhere afaik and now there is a separate span field. Now is the time to unify all of this. So the follow up question is: do you ever match or receive a four element tuple as position anywhere?

[josevalim]

I have pushed a commit that aligns the spec and the docs with the actual behaviour (which was always 1-based).

I also think that add a span field is better than using a four-element tuple as the position, even if the position allowed a four element tuple in its specification, because if we suddenly start returning the position as a four element tuple, I can see us breaking code that did not account for that.

There is a chance a compiler somewhere is emitting a four element tuple though. If that's the case, we need to add a deprecation pass and normalize the diagnostic to use spans. Please let me know if you are aware of such cases (in theory nothing should break if that's the case, but we should help folks align with the restricted typespec).

[lukaszsamson]

The zero-based in the typespec may be using zero as no column information available

I'd use line only instead of invalid out of range position

do you ever match or receive a four element tuple as position anywhere?

I can't really tell. The code handling it in ElixirLS landed in May 2022. Please remember that other compilers may be using Task.Diagnostic API so I'd check the major ones and include that in the release notes as possibly breaking change

[josevalim]

Agreed and agreed. Thanks!

[lukaszsamson]

I noticed there is a new undocumented span property in Code.diagnostics/1 type. Shouldn't it be exposed to Task.Diagnostic? It's typespec indicates it is still 0-based

[josevalim]

Fixed.

[lukaszsamson]

There is a chance a compiler somewhere is emitting a four element tuple though. If that's the case, we need to add a deprecation pass and normalize the diagnostic to use spans. Please let me know if you are aware of such cases

@josevalim https://hexdocs.pm/boundary/Boundary.html seems to be emitting 4-elem tuples as evident in this issue lexical-lsp/lexical#501