Skip to content

Attemptive fix for edge case in normalizer for keyword args - wip #13916

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Attemptive fix for edge case in normalizer for keyword args - wip #13916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96212bb
Attemptive fix for edge case in normalizer for keyword args - wip
sabiwara Oct 19, 2024
66775f5
Failing test
sabiwara Oct 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/elixir/lib/code/normalizer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ defmodule Code.Normalizer do
args = normalize_args(args, %{state | parent_meta: meta})
{form, meta, args}

Keyword.has_key?(meta, :do) or match?([{{:__block__, _, [:do]}, _} | _], last) ->
Keyword.has_key?(meta, :do) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially trying to add some check, like

Suggested change
Keyword.has_key?(meta, :do) ->
Keyword.has_key?(meta, :do) or (match?([{{:__block__, _, [:do]}, _} | _], last) and keyword?(last)) ->

but I couldn't find a case where this would actually work:

  • keyword? would return false because the blocks don't have the :format metadata
  • even if I force it, normalize_kw_blocks is useless at fixing the second test case I added

The full test suite otherwise passes without that match? clause.

Copy link
Member

Choose a reason for hiding this comment

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

I think the correct fix would be something like:

Keyword.has_key?(meta, :do) or (match?([{{:__block__, _, [:do]}, _} | _], last) and keyword_wrapped_in_block?(last)) ->

But perhaps, it is best to not use Keyword.has_key? or keyword? and instead have your own function called keyword_ast_key? (which checks both for :do and the wrapped one) and keyword_ast? which checks if all keys are wrapped in blocks or a regular atoms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @josevalim for checking this 🙇

I think the correct fix would be something like:

Keyword.has_key?(meta, :do) or (match?([{{:__block__, _, [:do]}, _} | _], last) and keyword_wrapped_in_block?(last)) ->

I tried but even if we make sure we get through this clause, it doesn't fix the test added here, which let me think I misunderstood something.

But perhaps, it is best to not use Keyword.has_key? or keyword? and instead have your own function called keyword_ast_key? (which checks both for :do and the wrapped one) and keyword_ast? which checks if all keys are wrapped in blocks or a regular atoms.

I tried something like this too, like the first clause being just checking the meta: Keyword.has_key?(meta, :do) and the second clause checking if it's a :do kw ast, disregarding if it's wrapped or not. But this would trigger some other test to fail:

Screenshot 2024-10-22 at 21 29 27

Maybe the test I tried to add shouldn't be passing in the first place, and I'm overthinking this, but I just couldn't find a test that the existing or match?([{{:__block__, _, [:do]}, _} | _], last) would otherwise make pass.

# def foo do :ok end
# def foo, do: :ok
normalize_kw_blocks(form, meta, args, state)
Expand Down
37 changes: 37 additions & 0 deletions lib/elixir/test/elixir/code_normalizer/quoted_ast_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,43 @@ defmodule Code.Normalizer.QuotedASTTest do
assert quoted_to_string(quote(do: foo |> [bar: :baz])) == "foo |> [bar: :baz]"
end

test "keyword arg edge case: cursor" do
input = "def foo, do: :bar, __cursor__()"
expected = "def foo, [{:do, :bar}, __cursor__()]"

ast = Code.string_to_quoted!(input)

assert quoted_to_string(ast) == expected

ast =
Code.string_to_quoted!(input,
literal_encoder: &{:ok, {:__block__, &2, [&1]}}
)

assert quoted_to_string(ast) == expected
end

test "keyword arg edge case: literal encoder" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is a reverse engineering attempt to understand how the match? clause works, but it doesn't seem to

input = """
foo(Bar) do
:ok
end
"""

expected = String.trim(input)

ast = Code.string_to_quoted!(input)

assert quoted_to_string(ast) == expected

ast =
Code.string_to_quoted!(input,
literal_encoder: &{:ok, {:__block__, &2, [&1]}}
Copy link
Contributor Author

@sabiwara sabiwara Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  • with token_metadata: true, this passes without needing the match? clause
  • without token_metadata: true, this fails with or without the match? clause

)

assert quoted_to_string(ast) == expected
end

test "list in module attribute" do
assert quoted_to_string(
quote do
Expand Down
Loading