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

[sabiwara]

Attempt at fixing #13895, but I'm a bit stuck, sharing for discussion.

[sabiwara]

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.

[josevalim]

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.

[sabiwara]

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:

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.

[sabiwara]

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

[sabiwara]

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