elixir-lang / tree-sitter-elixir

Elixir grammar for tree-sitter
https://elixir-lang.org/tree-sitter-elixir
Apache License 2.0
245 stars 24 forks source link

handle stab clauses without right-hand-sides #33

Closed the-mikedavis closed 2 years ago

the-mikedavis commented 2 years ago

Currently a stab clause without a right-hand-side is parsed as an error:

Enum.map(xs, fn x ->
end)

And the end token ends up not being highlighted as a keyword. The compiler gives a warning about this syntax but it comes up pretty often when editing (writing a case block for example).

Implementation-wise, this might be a bug in tree-sitter? prec.right seems to fight with error recovery when the rightmost token(s) are optional.

fn ->
end

gets parsed as

(anonymous_function
  (stab_clause
    right: (body (identifier)))
  (MISSING "end"))

although the optional should allow this case. I've seen this in other grammars and it seems like the way around it is to replace the prec.right with a conflict.

jonatanklosko commented 2 years ago

Hey @the-mikedavis, good catch!

I think the issues comes from the fact that $.body has an optional leading $._terminator, so given fn -> \n, the newline is tokenized as terminator, and if the body doesn't match, there is no alternative path to consume the terminator.

We could address it by explicitly allowing body to be a single terminator like this:

body: ($) =>
  choice(
    $._terminator,
    seq(
      optional($._terminator),
      sep1($._expression, $._terminator),
      optional($._terminator)
    ),
  ),

This should correctly disallow two stab clauses with empty body, which is not a valid syntax. wdyt?

the-mikedavis commented 2 years ago

Oh hmm I just tried that out locally and the test case doesn't seem to like it:

- actual
+ expected

 (source
   (call
     (identifier)
     (do_block
       (stab_clause
         (arguments
-          (identifier))
-        (body
           (identifier)))
-      (ERROR
-        (operator_identifier)))))
+      (stab_clause
+        (arguments
+          (identifier))))))

I think in this case that syntax might already be allowed by the NEWLINE extra although I'm not sure how that precedence works out.

jonatanklosko commented 2 years ago

Oh, but that's what I meant :) The following should parse fine

fun do
  x ->
end

while this shouldn't

fun do
  x ->
  y ->
end

In Elixir, the first one results in a warning, but the second is actually a parsing error, so this seems accurate :)

the-mikedavis commented 2 years ago

Oh I forgot the test case had two stab clauses, I need some coffee 😅

Just checked with one clause and that works perfectly! I'll push a commit :+1: