Skip to content

Fix keyword list parsing crash #56

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukaszsamson
Copy link
Contributor

Fixes crash 3 from #48
Fixes #26
Supersedes #32

lib/spitfire.ex Outdated
Comment on lines 557 to 559
parser = parser |> next_token() |> next_token()
{item, parser} = parse_expression(parser, @kw_identifier, true, false, false)
{item, parser}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe this case actually should result in an error,

iex(1)> Code.string_to_quoted("[foo: \"bar\", :baz]")
{:error,
 {[line: 1, column: 12],
  "unexpected expression after keyword list. Keyword lists must always come last in lists and maps. Therefore, this is not allowed:\n\n    [some: :value, :another]\n    %{some: :value, another => value}\n\nInstead, reorder it to be the last entry:\n\n    [:another, some: :value]\n    %{another => value, some: :value}\n\nSyntax error after: ",
  "','"}}

Comment on lines +538 to +546
code = ~S'''
defmodule MyModule do
IO.inspect(
:stderr,
label: "label",
(__cursor__())
)
end
'''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an interesting result from the builtin parser

iex(5)> Code.string_to_quoted(~S'''
...(5)>       defmodule MyModule do
...(5)>         IO.inspect(
...(5)>           :stderr,
...(5)>           label: "label",
...(5)>           (__cursor__())
...(5)>         )
...(5)>       end
...(5)>       ''')
{:ok,
 {:defmodule, [line: 1],
  [
    {:__aliases__, [line: 1], [:MyModule]},
    [
      do: {{:., [line: 2], [{:__aliases__, [line: 2], [:IO]}, :inspect]},
       [line: 2], [:stderr, [{:label, "label"}, {:__cursor__, [line: 5], []}]]}
    ]
  ]}}
iex(6)> Code.string_to_quoted(~S'''
...(6)> defmodule MyModule do
...(6)>   IO.inspect(
...(6)>     :stderr,
...(6)>     label: "label",
...(6)>     :foo
...(6)>   )
...(6)> end
...(6)> '''
...(6)> )
{:error,
 {[line: 4, column: 19],
  "unexpected expression after keyword list. Keyword lists must always come as the last argument. Therefore, this is not allowed:\n\n    function_call(1, some: :option, 2)\n\nInstead, wrap the keyword in brackets:\n\n    function_call(1, [some: :option], 2)\n\nSyntax error after: ",
  "','"}}
iex(7)>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the builtin parser supports a macro call returning tuple AST node or this is an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility it is more forgiving for nodes with __cursor__() special form

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhanberg We have two options here.

  1. Have a special handling for __cursor__() nodes in keyword lists
  2. Make the parser error tolerant and either silently drop invalid list entries or produce invalid AST

@lukaszsamson lukaszsamson force-pushed the ls-fix-unfinished-keyword branch from b169652 to 41c96eb Compare February 11, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: failed to parse a bracketed keyword list
2 participants