Elixir: Formatting of with and config blocks with inner comments

[Nicd]

Environment

• Elixir & Erlang versions (elixir --version): Erlang 20, Elixir 1.6.0-dev (b3992aa)
• Operating system: macOS 10.13

Current behavior

When formatting a with or config clause with comments inside for specific lines, the comments are moved right before the block. See these snippets for diffs:

config: https://gitlab.com/snippets/1682142

with: https://gitlab.com/snippets/1682143

This makes documenting the intention of with clauses and config keys cumbersome as they need to be explained before the whole block.

Expected behavior

Comments stay where they are.

[jgmchan]

I would like to help a little on this issue if possible. I've done some code diving focusing only on the with issue and I think it has something to do with how the generator expressions are handled in the call_args_to_algebra_no_blocks/6 function.

It seems that the generators are handled after the left argument has already been transformed to an Algebra document so the comments end up being grouped together before the entire lot.

This actually means for comprehensions also have the same issue.

I have confirmed this behaviour with the following test cases:

  test "with inner comments in for comprehensions" do
    assert_same ~S"""
      for n <- list,
          # This is something
          times <- 1..n do
        String.duplicate("*", times)
      end
    """
  end

  test "with inner comments inside with blocks" do
    assert_same ~S"""
      with 0 <- 0,
           # Do something
           1 <- 1,
           # Do something else
           2 <- 2,
           # Check some stuff
           false <- true do
       IO.puts("blah")
     end
    """
  end

The tests fail with this:

  1) test with inner comments inside with blocks (Code.Formatter.CommentsTest)
     lib/elixir/test/elixir/code_formatter/comments_test.exs:1005
     Assertion with == failed
     code:  assert IO.iodata_to_binary(Code.format_string!(good, opts)) == String.trim(good)
     left:  "# Do something\n# Do something else\n# Check some stuff\nwith 0 <- 0,\n     1 <- 1,\n     2 <- 2,\n     false <- true do\n  IO.puts(\"blah\")\nend"
     right: "with 0 <- 0,\n       # Do something\n       1 <- 1,\n       # Do something else\n       2 <- 2,\n       # Check some stuff\n       false <- true do\n   IO.puts(\"blah\")\n end"
     stacktrace:
       lib/elixir/test/elixir/code_formatter/comments_test.exs:1006: (test)



  2) test with inner comments in for comprehensions (Code.Formatter.CommentsTest)
     lib/elixir/test/elixir/code_formatter/comments_test.exs:995
     Assertion with == failed
     code:  assert IO.iodata_to_binary(Code.format_string!(good, opts)) == String.trim(good)
     left:  "# This is something\nfor n <- list,\n    times <- 1..n do\n  String.duplicate(\"*\", times)\nend"
     right: "for n <- list,\n      # This is something\n      times <- 1..n do\n    String.duplicate(\"*\", times)\n  end"
     stacktrace:
       lib/elixir/test/elixir/code_formatter/comments_test.exs:996: (test)

I'm still very new to Elixir and Algebra documents but I guess the way to fix this would be to break down the generator expressions before transforming them into an Algebra document, similar to how statements with :-> are processed. Would this be correct?

[jgmchan]

Seems like it might not be as easy just dealing with the :<- tokens as for comprehensions can also contain filters like for n <- [1, 2, 3, 4, 5, 6], rem(n, 2) == 0, do: n

[josevalim]

Hi @jgmchan. It is not related to the generators but to the fact we don't handle comments inside function calls at all.

[jgmchan]

Yeah I had a play around with it today and found that if I replace the following line:

{left_doc, state} = args_to_algebra(left, state, &quoted_to_algebra(&1, context, &2))

with

{left_doc, state} = args_to_algebra_with_comments(left, meta, next_break_fits?, state, &quoted_to_algebra(&1, context, &2))

I could at least get the left side to keep the comments on the correct line (although the indentation is a little off). This suggested to me that it might be more complex than I had thought.

I would like to help with this but I don't really know where exactly to tackle this problem.

[josevalim]

@jgmchan yup, that's general place that needs to be changed but I assume that indeed other changes are needed and I don't know upfront where they are without digging into the code too.

[josevalim]

@jgmchan the changes were larger than I expected as it required a good amount of code to move around. You can see them here: 5ea7321

[jgmchan]

Wow, this was more complicated than I thought, thanks @josevalim for showing me the changes.