Fix sigil formatting bug that produced invalid mixed syntax

[williamthome]

Fixes #369

When formatting multiline sigil strings, erlfmt was incorrectly applying regular string transformation logic, creating invalid mixed syntax like:

~"\n"
"    foo\n"
"    "

This fix leaves the sigil content entirely unchanged to prevent the mixed syntax bug.

I'm not entirely sure if this is the best solution - sigils might still be able to participate in container line-breaking while keeping their raw content intact.

I welcome maintainer feedback on whether full preservation fits erlfmt's formatting philosophy or if an alternative approach would be better.

[michalmuskala]

Sorry for the delay in reviewing this.

I'm fairly convinced this won't work correctly to indent the resulting strings inside sigils - the formatting algebra doesn't expect newlines inside strings - it expects to manage all newlines explicitly.

[michalmuskala]

To give a concrete example where this goes wrong:

test() ->
    [~"
    ", b, c].

Code like this wouldn't be changed by the formatter after your changes - this is incorrect. Since there's a multi-line expression inside the list, the list itself should be broken into separate expressions line-by-line - we don't allow mixing multi-line and single-line expressions inside a container.

If we want to preserve sigils unchanged and keep the formatting rules, this would need to be formatted as:

test() ->
    [
        ~"
    ",
        b,
        c
    ].

TBH all the Erlang sigils multi-line with delimiter other than triple-quotes manage whitespace and indentation in quite, at least to me, unintuitive ways, and and up looking just plain ugly.

[williamthome]

Thank you for the detailed feedback on the sigil formatting issue. You were absolutely right about the problem, and I've implemented a fix that addresses your concerns while preserving sigil content integrity.

The core issue was that sigils containing newlines weren't signaling to their parent containers that they should break into multiple lines, leading to the mixed formatting you pointed out. The fix adds a simple check in the sigil formatting logic - when a sigil contains newlines, it now uses force_breaks() to tell parent containers like lists, tuples, and function calls to format across multiple lines.

Your specific test case now works correctly:

%% Before (problematic):
test() -> [~"
", b, c].

%% After (fixed):
test() ->
    [
        ~"
",
        b,
        c
    ].

The sigil content stays exactly the same (preserving binary data), but the list properly breaks into multiple lines. This works consistently across all container types:

%% Tuples break properly:
{~"
    first element
    multiline
    ", second_element, third}
% becomes:
{
    ~"
    first element
    multiline
    ",
    second_element,
    third
}

%% Function calls break properly:
process(~"
    multiline argument
    in function call
    ")
% becomes:
process(
    ~"
    multiline argument
    in function call
    "
)

The key difference from regular strings is that sigils stay as single units while regular strings get broken apart:

%% Mixed case showing the difference:
["regular
    multiline string", ~"sigil
    multiline content", atom]
% becomes:
[
    "regular\n"                % <- regular string gets split
    "    multiline string",
    ~"sigil                    % <- sigil stays intact
    multiline content",
    atom
]

The implementation turned out to be quite complex because of the interaction between the formatting algebra and container detection logic. The challenge was ensuring that sigils trigger proper container formatting without modifying their own content, which would change their semantic meaning. Regular strings get broken into concatenated parts during formatting, but sigils must stay as single atomic units.

I've added comprehensive test cases covering all the scenarios discussed, including mixed regular strings and sigils, nested containers, and various sigil types with prefixes and suffixes. All tests are passing and the formatter now handles these cases consistently.

The fix is minimal but touches a critical part of the formatting system, so I'd appreciate your review to make sure it handles all the edge cases correctly and maintains the formatting principles you outlined.

[michalmuskala]

The only thing I'm still a bit apprehensive about is triple-quote strings - since then it's easy to properly handle newlines (and we already do it), I think for sigils with a triple-quote string inside we should do the previous code. They're not problematic since they strip-out the indentation whitespace properly. You can check out the snapshot tests in the tripple_string.erl - I think if we add a sigil to each one, they should behave the same as without it.

[williamthome]

Hi @michalmuskala,

Thank you for the detailed feedback on the triple-quoted sigils. I've updated the implementation to address your concerns.

The key fix ensures that ~"""...""" sigils (with any prefix) remain unchanged in containers since they properly handle indentation, while regular multiline sigils like ~"...\n..." still cause container breaking as intended.

All tests pass and the implementation now correctly handles the nuanced differences between sigil types. Ready for another review when you have a chance.

Thanks again for the feedback!

[michalmuskala]

Per the parser, sigil can only have string inside. This clause will never be executed.

erlfmt/src/erlfmt_parse.yrl

Line 653 in 1292f7c

atomic -> sigil_prefix string sigil_suffix : {sigil, ?range_anno('$1', '$3'), '$1', '$2', '$3'}.

[williamthome]

08de55a

[michalmuskala]

This will not work correctly for multiple nesting levels. We need to do the same logic as is done in normal triple-quoted strings in string_to_algebra, just add prefix and suffix around it.
Furthermore, ideally we'd avoid duplicating this logic and just share the code.

[williamthome]

Could you clarify what specific aspect of nesting isn't working correctly and provide a specific example of a case that's currently broken?

I want to make sure I understand the issue properly before implementing a fix.

A concrete failing test case would help me understand the exact issue you're referring to.

[michalmuskala]

For tests, please follow the existing code style and avoid \n directly in strings - split them across multiple lines. This makes it possible to easier understand what's going on from looking at the test - this is very hard to understand how things are moved around.

[williamthome]

d8cbea4

[michalmuskala]

This isn't correct because of the issue how triple-quoted strings I handled that I mentioned before.

It should be formatted as:

[
    ~"""
    foo
    bar
    """
].

[williamthome]

0899828

[williamthome]

I'm unsure why CI is failing. Locally it's all fine:

$ make checkfmt 
rebar3 as release escriptize
===> Verifying dependencies...
===> Analyzing applications...
===> Compiling erlfmt
===> Building escript for erlfmt...
_build/release/bin/erlfmt -c --exclude-files="src/erlfmt_parse.erl"
Checking formatting...
All matched files use erlfmt code style!