Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
82 changes: 82 additions & 0 deletions spec/compiler/parser/to_s_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -749,4 +749,86 @@ describe "ASTNode#to_s" do
.map do |v| v end
%}
CR

expect_to_s <<-'CR'
{%
(
1
)
%}
CR

expect_to_s <<-'CR'
{%
(
1
2
)
%}
CR

expect_to_s <<-'CR'
{%
(
true ||
false
Comment on lines +773 to +774
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be indented the same as the regular code, I guess?

Suggested change
true ||
false
true ||
false

ditto all below

Copy link
Copy Markdown
Member Author

@Blacksmoke16 Blacksmoke16 Apr 30, 2025

Choose a reason for hiding this comment

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

Dunno, formatter doesn't do anything with it in this macro context so imo it's fine as it is. The main objective was to have proper line numbers. Column numbers are less important. Esp when it's subjective what the indentation should be.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose the formatter really should act on code inside macro expressions. Let's leave that for a follow-up, though.

)
%}
CR

expect_to_s <<-'CR'
{%
(
true ||
false
) && true
%}
CR

expect_to_s <<-'CR'
{%
true && (
true ||
false
)
%}
CR

expect_to_s <<-'CR', <<-'CR'
{%
if (v = 5) &&
(
1 < v ||
(v < 4 && 5 < 6)
)
123
end
%}
CR
{%
if (v = 5) &&
(
1 < v ||
(v < 4 && 5 < 6)
)
123
end
%}
CR

expect_to_s <<-'CR', <<-'CR'
{%
if (true || false) &&
true
1
end
%}
CR
{%
if (true || false) &&
true
1
end
%}
CR
end
85 changes: 55 additions & 30 deletions src/compiler/crystal/syntax/to_s.cr
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,13 @@ module Crystal
end

def visit(node : Expressions)
is_multiline = false

case node.keyword
in .paren?
@str << '('
# Handled via dedicated #in_parenthesis call below
is_multiline = (loc = node.location) && (first_loc = node.expressions.first?.try &.location) && (first_loc.line_number > loc.line_number)
append_indent if is_multiline
in .begin?
@str << "begin"
@indent += 1
Expand All @@ -280,33 +284,35 @@ module Crystal
# Not a special condition
end

if @inside_macro > 0
node.expressions.each &.accept self
else
last_node = nil
in_parenthesis node.keyword.paren?, is_multiline do
if @inside_macro > 0
node.expressions.each &.accept self
else
last_node = nil

node.expressions.each_with_index do |exp, i|
unless exp.nop?
write_extra_newlines (last_node || exp).end_location, exp.location
node.expressions.each_with_index do |exp, i|
unless exp.nop?
write_extra_newlines (last_node || exp).end_location, exp.location

append_indent unless node.keyword.paren? && i == 0
exp.accept self
append_indent unless node.keyword.paren? && i == 0
exp.accept self

if (root = @root_level_macro_expressions) && root.same?(node) && i == node.expressions.size - 1
# Do not add a trailing newline after the last node in the root `Expressions` within a `MacroExpression`.
# This is handled by the `MacroExpression` logic.
elsif !(node.keyword.paren? && i == node.expressions.size - 1)
newline
end
if (root = @root_level_macro_expressions) && root.same?(node) && i == node.expressions.size - 1
# Do not add a trailing newline after the last node in the root `Expressions` within a `MacroExpression`.
# This is handled by the `MacroExpression` logic.
elsif !(node.keyword.paren? && i == node.expressions.size - 1)
newline
end

last_node = exp
last_node = exp
end
end
end
end

case node.keyword
in .paren?
@str << ')'
# Handled via dedicated #in_parenthesis call above
in .begin?
@indent -= 1
append_indent
Expand Down Expand Up @@ -585,18 +591,28 @@ module Crystal
end
end

def in_parenthesis(need_parens, &)
if need_parens
@str << '('
yield
@str << ')'
else
yield
def in_parenthesis(need_parens, is_multiline = false, &)
@str << '(' if need_parens

if is_multiline
newline
@indent += 1
append_indent
end

yield

if is_multiline
newline
@indent -= 1
append_indent
end

@str << ')' if need_parens
end

def in_parenthesis(need_parens, node)
in_parenthesis(need_parens) do
def in_parenthesis(need_parens, node, is_multiline = false)
in_parenthesis(need_parens, is_multiline) do
if node.is_a?(Expressions) && node.expressions.size == 1
node.expressions.first.accept self
else
Expand Down Expand Up @@ -1269,14 +1285,23 @@ module Crystal

def to_s_binary(node, op)
left_needs_parens = need_parens(node.left)
in_parenthesis(left_needs_parens, node.left)
left_parens_multiline = left_needs_parens && (begin_loc = node.left.location) && (end_loc = node.left.end_location) && (end_loc.line_number > begin_loc.line_number)
in_parenthesis(left_needs_parens, node.left, left_parens_multiline)

@str << ' '
@str << op
@str << ' '

if (right_loc = node.right.location) && (left_end_loc = node.left.end_location) && (right_loc.line_number > left_end_loc.line_number)
newline
append_indent
else
@str << ' '
end

right_needs_parens = need_parens(node.right)
in_parenthesis(right_needs_parens, node.right)
right_parens_multiline = right_needs_parens && (begin_loc = node.right.location) && (end_loc = node.right.end_location) && (end_loc.line_number > begin_loc.line_number)
in_parenthesis(right_needs_parens, node.right, right_parens_multiline)

false
end

Expand Down