fix: escape injected code in Common.replace_code/2

[ibarakaiev]

Contributor checklist

• [x] Bug fixes include regression tests
• [ ] Features include unit/acceptance tests

In Common.replace_code/2, the code should be escaped before being injected. For example, when updating an existing config value to a list of string tuples, Common.replace_code/2 will just inject it resulting in:

[{"hello", "world"}]

instead of

{:__block__,
  [
    trailing_comments: [],
    leading_comments: [],
    closing: [line: 1, column: 20],
    line: 1,
    column: 1
  ],
  [
    [
      {:__block__,
       [
         trailing_comments: [],
         leading_comments: [],
         closing: [line: 1, column: 19],
         line: 1,
         column: 2
       ],
       [
         {{:__block__,
           [
             trailing_comments: [],
             leading_comments: [],
             delimiter: "\"",
             line: 1,
             column: 3
           ], ["hello"]},
          {:__block__,
           [
             trailing_comments: [],
             leading_comments: [],
             delimiter: "\"",
             line: 1,
             column: 12
           ], ["world"]}}
       ]}
    ]
  ]}

and Sourceror then fails to put it back into string form resulting in

** (SyntaxError) invalid syntax found on nofile:9:42:
    error: syntax error before: '=>'
    │
  9 │       "hello" => "world",
    │               ^
    │
    └─ nofile:9:42

I think the fix is to just replace

def replace_code(zipper, code) do
  # code = use_aliases(code, zipper)
  Zipper.replace(zipper, code)
end

with

def replace_code(zipper, code) do
  # code = use_aliases(code, zipper)
  Zipper.replace(zipper, code |> Macro.to_string() |> Sourceror.parse_string!())
end

However, I don't know if this would break other things elsewhere 🤔 Or if it's something that should be handled in the default updater definitions:

updater = opts[:updater] || fn zipper -> {:ok, Common.replace_code(zipper, value)} end
                                                                           ^
                                                                           here

[zachdaniel]

Hey @ibarakaiev I merged this by accident, I meant to close it and add a comment.

We can't escape the value in add_code, because the point is to add "code", not a value. We're going to have to find some other way to handle the issue you were encountering without escaping the value. For example, using Macro.escape when adding configuration elsewhere. We can discuss alternatives.

[ibarakaiev]

I think I might have misused the word "escape". In this context, the change was intended to ensure a proper conversion from Elixir's AST to Sourceror's AST where necessary. Concretely, [{"hello", "world"}] is a valid Elixir AST expression but not a valid Sourceror expression.

I do see now that the currently proposed change is flawed because it doesn't check if the original code is already in the Sourceror's format, in which case that's what should just get injected (and it would fail the conversion using Macro.to_string/1 anyway).

So I wonder if the following is the best way to address this issue:

1. If the code that's being injected is not a Sourceror AST (can check if meta contains trailing_comments?) then it needs to be converted to string using Macro.to_string/1 and then get parsed with Sourceror.parse_string!/1 (I didn't find a more direct way to do the conversion). If the code that's being injected is already in Sourceror AST format (i.e. with many nested blocks) then do nothing.
2. Change default updater functions in configure to convert any "value" (i.e. [{"hello", "world"}]) to Sourceror's AST ahead of time.

The second would fix the immediate issue without necessarily requiring the first change to be implemented, but I do think that if the first is not implemented then it opens up the door for subtle bugs for end users using Igniter—for example, if they provide a custom updater function to configure/6 that uses put_in_keyword_key with something like [{"hello", "world"}].

[zachdaniel]

oh, you're right. I misread your code also, assuming it was doing Macro.escape.

It's an interesting question 🤔 I think [{"hello", "world"}] is valid sourceror AST node, it's just that it gets default formatting that we don't like. The answer may be some kind of explicit conversion process?

I think your original approach makes sense, assuming we detect an AST from sourceror as you mentioned. There could potentially be some "false positives", i.e if someone has a mixed AST w/ some regular code inserted into Sourceror AST, but I think that is fine for now.

[zachdaniel]

Could you open another PR?