Skip to content

Conversation

@doorgan
Copy link
Contributor

@doorgan doorgan commented Jan 31, 2026

Code like %{a do :ok end | b c, d => e} is ambiguous because it can be interpreted as:

%{(... | b(c, d)) => e} - what Spitfires parsed
%{... | b(c, d) => e} - what Elixir chooses

Elixir is aware of this ambiguity and warns about it:

warning: missing parentheses on expression following operator "|", you must add parentheses to avoid ambiguities

Since this is ambiguous syntax, Spitfire it not entirely wrong here, but we should follow what Elixir produces anyways.

The only way I got this to trigger(from examples in the property tests in Lukasz prs) is with the lhs of | being a do/end block, I'm not 100% sure why, but withut it the Elixir parser straight out returns a syntax error.

We were allowing => to be consumed inside the no-parens call arguments and the map entry parser never got a chance to see it. The fix marks map entry contexts (map/struct literals and map updates) with map_context and blocks => from being consumed inside nested expressions. That keeps => at the entry level and allows the existing ambiguity handling in parse_pipe_op/2 to match Elixir’s choice. last_assoc_meta context is used to reconstruct the ast after resolving the ambiguity, and put the assoc metadata in the right node.

I added a new with_state/4 macro to make it easier to manage parser context. Setting and remember to unset parser context is very error prone, so this should help.

@doorgan doorgan marked this pull request as draft January 31, 2026 04:08
@doorgan doorgan force-pushed the doorgan/ambiguous-map-update branch 2 times, most recently from 448f4dc to 566b271 Compare January 31, 2026 07:50
@doorgan doorgan marked this pull request as ready for review January 31, 2026 08:02
@doorgan doorgan force-pushed the doorgan/ambiguous-map-update branch from 566b271 to 3344e33 Compare January 31, 2026 16:54
#
# This section rewrites the parse attempt to match Elixir's behavior.
case pairs do
[{{call, call_meta, [_, _ | _] = args}, value}] ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment that has an example of code that matches each clause? I'm getting confused cuz this says pairs but it's a list of one element.

Copy link
Contributor Author

@doorgan doorgan Jan 31, 2026

Choose a reason for hiding this comment

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

Done. It was called pairs because it's the key/value pairs for a => b, c => d. It may also be a single element(the second clause) so I renamed this to entries instead.
I added a comment with the shape each clause handles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think I was getting confused at to what the ast for this is. The args are the pairs and the "call" is the pipe?

Copy link
Contributor Author

@doorgan doorgan Feb 1, 2026

Choose a reason for hiding this comment

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

So this pattern [{{call, call_meta, [_, _ | _] = args}, value}] matches this interpretation: %{... | (b(c, d)) => e}
Where:

  • call is b(c, d)
  • The ambiguity happens with a 2+ args call(as the comma separating the args causes the ambiguity when the source code has no parens), so [_, _ | _] = args is checking for c, d, ... in that call
  • value is e
  • There is no :"=>" ast node, this pair represents that

The pipe parsing happens in parse_expression when the flag is_map is true, then parse_pipe_op -> parse_map_update_pairs -> parse_comma_list. The last function there is what produces a list of entries for the map, and from that list is where this "pair" comes from if it finds an assoc_op(=>) token.

Then, in parse_pipe_op(which is only called for map updates, not list cons like [head | tail], we call this function that sees the ambiguity and resolves it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going over this again I realize if there's more entries like %{a do :ok end | b c, d => e, f => g} then there's still a mismatch, so I'll have to fix that too

Use the `:capture` option to return a map of selected keys as they were
at the end of the block, before state is restored.
"""
defmacro with_state(parser, updates, opts \\ [], do: block) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you write a few unit tests for this?

Code like `%{a do :ok end | b c, d => e}` is ambiguous because it can be
interpreted as:

`%{(... | b(c, d)) => e}` - what Spitfires parsed
`%{... | b(c, d) => e}` - what Elixir chooses

Elixir is aware of this ambiguity and warns about it:
```
warning: missing parentheses on expression following operator "|", you must add parentheses to avoid ambiguities
```

Since this is ambiguous syntax, Spitfire it not entirely wrong here, but
we should follow what Elixir produces anyways.

The only way I got this to trigger(from examples in the property tests
in Lukasz prs) is with the lhs of `|` being a do/end block, I'm not 100%
sure why, but withut it the Elixir parser straight out returns a syntax error.

We were allowing `=>` to be consumed inside the no-parens call arguments
and the map entry parser never got a chance to see it.
The fix marks map entry contexts (map/struct literals and map updates) with
`map_context` and blocks `=>` from being consumed inside nested expressions.
That keeps `=>` at the entry level and allows the existing ambiguity handling
in `parse_pipe_op/2` to match Elixir’s choice.
@doorgan doorgan force-pushed the doorgan/ambiguous-map-update branch from 454ab8e to 725126e Compare February 1, 2026 02:48
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.

2 participants