-
-
Notifications
You must be signed in to change notification settings - Fork 10
fix: ambiguous map update syntax #87
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
base: main
Are you sure you want to change the base?
Conversation
448f4dc to
566b271
Compare
566b271 to
3344e33
Compare
| # | ||
| # This section rewrites the parse attempt to match Elixir's behavior. | ||
| case pairs do | ||
| [{{call, call_meta, [_, _ | _] = args}, value}] -> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
callisb(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
[_, _ | _] = argsis checking forc, d, ...in that call valueis 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.
There was a problem hiding this comment.
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
lib/spitfire/context.ex
Outdated
| 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 |
There was a problem hiding this comment.
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.
454ab8e to
725126e
Compare
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 choosesElixir is aware of this ambiguity and warns about it:
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) withmap_contextand blocks=>from being consumed inside nested expressions. That keeps=>at the entry level and allows the existing ambiguity handling inparse_pipe_op/2to match Elixir’s choice.last_assoc_metacontext 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/4macro to make it easier to manage parser context. Setting and remember to unset parser context is very error prone, so this should help.