Skip to content
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

Compiler: fix crash when formatting syntax error inside macro #8055

Merged
merged 1 commit into from
Aug 12, 2019
Merged

Compiler: fix crash when formatting syntax error inside macro #8055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions spec/compiler/semantic/macro_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,4 +1427,12 @@ describe "Semantic: macro" do
),
"can't instantiate abstract class Foo"
end

it "doesn't crash on syntax error inside macro (regression, #8038)" do
expect_raises(Crystal::SyntaxException, "unterminated array literal") do
semantic(%(
{% begin %}[{% end %}
))
end
end
end
25 changes: 15 additions & 10 deletions src/compiler/crystal/exception.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,22 +242,27 @@ module Crystal

def minimize_indentation(source)
min_leading_white_space =
source.map do |line|
if match = line.match(/^(\s+)\S/)
spaces = match[1]?
spaces.size if spaces
end
end
.compact
.min
source.min_of? { |line| leading_white_space(line) } || 0

source = source.map do |line|
replace_leading_tabs_with_spaces(line).lchop(" " * min_leading_white_space)
if min_leading_white_space > 0
source = source.map do |line|
replace_leading_tabs_with_spaces(line).lchop(" " * min_leading_white_space)
end
end

{source, min_leading_white_space}
end

private def leading_white_space(line)
match = line.match(/^(\s+)\S/)
return 0 unless match

spaces = match[1]?
return 0 unless spaces

spaces.size
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this part is refactored, we can simplify it by avoiding using a regex:

line.each_char_with_index do |char, index|
  return index if char != ' '
end
0

Copy link
Member Author

Choose a reason for hiding this comment

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

No, \s matches tab, spaces, maybe also unicode space, I don't know.

This code runs once when the compiler finds an error, optimizing here has no impact at all. Maybe it's 5ms faster, the user can't possibly tell.

I also prefer avoiding changing code if it's not broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ho, there is char.ascii_whitespace?

But that's basically what's done here, the code is already changed.

end

def append_expanded_macro(io, source)
line_number = @line_number
if @error_trace || !line_number
Expand Down