Skip to content

Fix sigil formatting bug that produced invalid mixed syntax #371

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
williamthome wants to merge 7 commits into from
+197 −3
Closed

Fix sigil formatting bug that produced invalid mixed syntax #371

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d102ae4
Fix sigil formatting bug that produced invalid mixed syntax
williamthome Aug 30, 2025
77d7f6a
Improve sigil formatting to break containers with multiline sigils
williamthome Sep 24, 2025
fe1edd4
Refine sigil formatting to handle triple-quoted sigils properly
williamthome Sep 24, 2025
552a2e9
Add comprehensive tests for triple-quoted sigil behavior
williamthome Sep 24, 2025
08de55a
Remove unreachable fallback clause from sigil handling
williamthome Sep 25, 2025
d8cbea4
Improve test string formatting for better readability
williamthome Sep 25, 2025
0899828
Fix triple-quoted sigils to break containers consistently
williamthome Sep 25, 2025
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
19 changes: 17 additions & 2 deletions src/erlfmt_format.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,24 @@ do_expr_to_algebra({clauses, _Meta, Clauses}) ->
clauses_to_algebra(Clauses);
do_expr_to_algebra({body, _Meta, Exprs}) ->
block_to_algebra(Exprs);
do_expr_to_algebra({sigil, _Meta, Prefix, Content, Suffix}) ->
do_expr_to_algebra({sigil, _Meta, Prefix, {string, ContentMeta, _Value}, Suffix}) ->
PrefixDoc = concat(<<"~">>, do_expr_to_algebra(Prefix)),
concat(concat(PrefixDoc, do_expr_to_algebra(Content)), do_expr_to_algebra(Suffix));
Text = erlfmt_scan:get_anno(text, ContentMeta),
SigilDoc = concat(concat(PrefixDoc, string(Text)), do_expr_to_algebra(Suffix)),
case string:split(Text, "\n", all) of
% Triple-quoted sigils don't need force_breaks
["\"\"\"" ++ _ | _] -> SigilDoc;
Copy link
Member

@michalmuskala michalmuskala Sep 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@williamthome williamthome Sep 25, 2025

Choose a reason for hiding this comment

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

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.

% Single-line sigils don't need force_breaks
[_] -> SigilDoc;
% Multi-line non-triple-quoted sigils need force_breaks
_ -> concat([force_breaks(), SigilDoc])
end;
do_expr_to_algebra({sigil, _Meta, Prefix, {Atomic, ContentMeta, _Value}, Suffix}) when
Copy link
Member

@michalmuskala michalmuskala Sep 25, 2025

Choose a reason for hiding this comment

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

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'}.

Copy link
Author

@williamthome williamthome Sep 25, 2025

Choose a reason for hiding this comment

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

08de55a

?IS_ATOMIC(Atomic)
->
PrefixDoc = concat(<<"~">>, do_expr_to_algebra(Prefix)),
Text = erlfmt_scan:get_anno(text, ContentMeta),
concat(concat(PrefixDoc, string(Text)), do_expr_to_algebra(Suffix));
do_expr_to_algebra({sigil_prefix, _Meta, ''}) ->
<<"">>;
do_expr_to_algebra({sigil_prefix, _Meta, SigilName}) ->
Expand Down
121 changes: 120 additions & 1 deletion test/erlfmt_format_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,126 @@ sigils(Config) when is_list(Config) ->
),
?assertSame("~s\"\"\"\n\\tabc\n\\tdef\n\"\"\"\n"),
?assertSame("\"\"\"\nTest\nMultiline\n\"\"\"\n"),
?assertSame("~\"\"\"\nTest\nMultiline\n\"\"\"\n").
?assertSame("~\"\"\"\nTest\nMultiline\n\"\"\"\n"),
%% Test multiline sigils in lists cause list to break
?assertFormat(
"[~\"\n foo\n \"].\n",
Copy link
Member

@michalmuskala michalmuskala Sep 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Author

@williamthome williamthome Sep 25, 2025

Choose a reason for hiding this comment

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

d8cbea4

"[\n"
" ~\"\n"
" foo\n"
" \"\n"
"].\n"
),
%% Triple-quoted sigils don't cause container breaks (they handle indentation properly)
?assertSame("[~\"\"\"\n foo\n bar\n \"\"\"].\n"),
%% Triple-quoted sigils with prefixes also don't break containers
?assertSame("[~s\"\"\"\n content\n with prefix\n \"\"\"].\n"),
%% Triple-quoted sigils in tuples don't break containers
?assertSame("{~\"\"\"\n tuple content\n \"\"\", other}.\n"),
%% Triple-quoted sigils in function calls don't break containers
?assertSame("process(~\"\"\"\n function arg\n \"\"\").\n"),
%% Edge case: minimal triple-quoted sigil content
?assertSame("[~\"\"\"\n\"\"\"].\n"),
%% Mixed triple-quoted and regular multiline sigils show different behavior
?assertFormat(
"[~\"\"\"\n triple quoted\n \"\"\", ~\"\n regular multiline\n \"].\n",
"[\n"
" ~\"\"\"\n"
" triple quoted\n"
" \"\"\",\n"
" ~\"\n"
" regular multiline\n"
" \"\n"
"].\n"
),
%% Test sigils with prefixes and modifiers in lists
?assertFormat(
"[~s\"\n foo\n \"].\n",
"[\n"
" ~s\"\n"
" foo\n"
" \"\n"
"].\n"
),
?assertFormat(
"[~b\"\n foo\n \"x].\n",
"[\n"
" ~b\"\n"
" foo\n"
" \"x\n"
"].\n"
),
%% Test mixed regular and sigil strings in lists
?assertFormat(
"[\"regular\n multiline\", ~\"sigil\n multiline\"].\n",
"[\n"
" \"regular\\n\"\n"
" \" multiline\",\n"
" ~\"sigil\n multiline\"\n"
"].\n"
),
%% Test sigils in tuples
?assertFormat(
"{~\"\n foo\n \", bar}.\n",
"{\n"
" ~\"\n"
" foo\n"
" \",\n"
" bar\n"
"}.\n"
),
%% Test sigils in function calls
?assertFormat(
"foo(~\"\n multiline\n \").\n",
"foo(\n"
" ~\"\n"
" multiline\n"
" \"\n"
").\n"
),
%% Test sigils in function arguments with other expressions
?assertFormat(
"foo(\"regular\n string\", ~\"sigil\n string\", atom).\n",
"foo(\n"
" \"regular\\n\"\n"
" \" string\",\n"
" ~\"sigil\n string\",\n"
" atom\n"
").\n"
),
%% Test sigils in maps
?assertFormat(
"#{key => ~\"\n value\n \"}.\n",
"#{\n"
" key =>\n"
" ~\"\n"
" value\n"
" \"\n"
"}.\n"
),
%% Test sigils in records
?assertFormat(
"#rec{field = ~\"\n value\n \"}.\n",
"#rec{\n"
" field =\n"
" ~\"\n"
" value\n"
" \"\n"
"}.\n"
),
%% Test sigils cause containers to break but preserve sigil content
?assertFormat(
"test() ->\n"
" [~\"\n"
" \", b, c].\n",
"test() ->\n"
" [\n"
" ~\"\n"
" \",\n"
" b,\n"
" c\n"
" ].\n"
).

dotted(Config) when is_list(Config) ->
?assertSame("<0.1.2>\n"),
Expand Down