Errors: Changes reporting and handling

- Relies on the parsing errors raised in ErbContent instead of rescuing them in the parser, this allows us to keep the line number of the error. - Removes location from all error messages - Adds more tests that actually check the error messages and line number
davidwessman · Apr 28, 2024 · 98c142b · 98c142b
1 parent 2922893
commit 98c142b
Show file tree

Hide file tree

Showing 4 changed files with 192 additions and 82 deletions.
diff --git a/lib/syntax_tree/erb/nodes.rb b/lib/syntax_tree/erb/nodes.rb
@@ -370,10 +370,20 @@ def prepare_content(content)
  end
  rescue SyntaxTree::Parser::ParseError
  # Try to add the keyword to see if it parses
- result = ErbContent.new(value: [keyword, *content])
- @keyword = nil
-
- result
+ begin
+ result = ErbContent.new(value: [keyword, *content])
+ @keyword = nil
+
+ result
+ rescue SyntaxTree::Parser::ParseError => error
+ raise(
+ SyntaxTree::Parser::ParseError.new(
+ "Could not parse ERB-tag: #{error.message}",
+ @opening_tag.location.start_line + error.lineno - 1,
+ error.column
+ )
+ )
+ end
  end
  end
 

diff --git a/lib/syntax_tree/erb/parser.rb b/lib/syntax_tree/erb/parser.rb
@@ -52,7 +52,7 @@ def parse_any_tag
  if @found_doctype
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Only one doctype element is allowed",
+ "Duplicate doctype declaration",
  tag.location.start_line,
  0
  )
@@ -129,7 +129,7 @@ def make_tokens
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Unexpected character at #{index}: #{source[index]}",
+ "Unexpected character: #{source[index]}",
  line,
  0
  )
@@ -218,7 +218,7 @@ def make_tokens
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Unexpected character in string at #{index}: #{source[index]}",
+ "Unexpected character, #{source[index]}, when looking for closing single quote",
  line,
  0
  )
@@ -246,7 +246,7 @@ def make_tokens
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Unexpected character in string at #{index}: #{source[index]}",
+ "Unexpected character, #{source[index]}, when looking for closing double quote",
  line,
  0
  )
@@ -305,7 +305,7 @@ def make_tokens
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Unexpected character at #{index}: #{source[index]}",
+ "Unexpected character, #{source[index]}, when parsing HTML- or ERB-tag",
  line,
  0
  )
@@ -406,7 +406,7 @@ def parse_html_opening_tag
  if name.value =~ /\A[@:#]/
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Invalid html-tag name #{name}",
+ "Invalid HTML-tag name #{name.value}",
  name.location.start_line,
  0
  )
@@ -470,7 +470,7 @@ def parse_html_element
  if closing.nil?
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Missing closing tag for <#{opening.name.value}> at #{opening.location}",
+ "Missing closing tag for <#{opening.name.value}>",
  opening.location.start_line,
  0
  )
@@ -480,7 +480,7 @@ def parse_html_element
  if closing.name.value != opening.name.value
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Expected closing tag for <#{opening.name.value}> but got <#{closing.name.value}> at #{closing.location}",
+ "Expected closing tag for <#{opening.name.value}> but got <#{closing.name.value}>",
  closing.location.start_line,
  0
  )
@@ -505,10 +505,11 @@ def parse_erb_case(erb_node)
 
  unless erb_tag.is_a?(ErbCaseWhen) || erb_tag.is_a?(ErbElse) ||
  erb_tag.is_a?(ErbEnd)
+ location = erb_tag&.location || erb_node.location
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching erb-tag to the if-tag at #{erb_node.location}",
- erb_node.location.start_line,
+ "No matching ERB-tag for the <% #{erb_node.keyword.value} %>",
+ location.start_line,
  0
  )
  )
@@ -532,7 +533,7 @@ def parse_erb_case(erb_node)
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching when- or else-tag to the case-tag at #{erb_node.location}",
+ "No matching when- or else-tag for the case-tag",
  erb_node.location.start_line,
  0
  )
@@ -552,7 +553,7 @@ def parse_erb_if(erb_node)
  unless erb_tag.is_a?(ErbControl) || erb_tag.is_a?(ErbEnd)
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching erb-tag to the if-tag at #{erb_node.location}",
+ "No matching ERB-tag for the <% if %>",
  erb_node.location.start_line,
  0
  )
@@ -584,7 +585,7 @@ def parse_erb_if(erb_node)
  else
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching elsif- or else-tag to the if-tag at #{erb_node.location}",
+ "No matching <% elsif %> or <% else %> for the <% if %>",
  erb_node.location.start_line,
  0
  )
@@ -600,7 +601,7 @@ def parse_erb_else(erb_node)
  unless erb_end.is_a?(ErbEnd)
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching end-tag for the else-tag at #{erb_node.location}",
+ "No matching <% end %> for the <% else %>",
  erb_node.location.start_line,
  0
  )
@@ -642,8 +643,8 @@ def parse_erb_tag
  if !closing_tag.is_a?(ErbClose)
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching closing tag for the erb-tag at #{opening_tag.location}",
- opening_tag.location.start_line,
+ "No matching closing tag for the <% #{keyword.value} %>",
+ closing_tag.location.start_line,
  0
  )
  )
@@ -678,7 +679,7 @@ def parse_erb_tag
  unless erb_end.is_a?(ErbEnd)
  raise(
  SyntaxTree::Parser::ParseError.new(
- "Found no matching end-tag for the do-tag at #{erb_node.location}",
+ "No matching <% end %> for the <% do %>",
  erb_node.location.start_line,
  0
  )
@@ -695,27 +696,6 @@ def parse_erb_tag
  erb_node
  end
  end
- rescue SyntaxTree::Parser::ParseError => error
- # If we have parsed tokens that we cannot process after we parsed <%, we should throw a ParseError
- # and not let it be handled by a `maybe`.
- if opening_tag
- message =
- if error.message.include?("Could not parse ERB-tag")
- error.message
- else
- "Could not parse ERB-tag: #{error.message}"
- end
-
- raise(
- SyntaxTree::Parser::ParseError.new(
- message,
- opening_tag.location.start_line,
- 0
- )
- )
- else
- raise(error)
- end
  end
 
  def parse_until_erb_close

diff --git a/test/erb_test.rb b/test/erb_test.rb
@@ -12,21 +12,47 @@ def test_empty_file
  end
 
  def test_missing_erb_end_tag
- assert_raises(SyntaxTree::Parser::ParseError) do
- ERB.parse("<% if no_end_tag %>")
- end
+ example = <<-HTML
+ <ul>
+ <% if condition %>
+ <li>A</li>
+ <li>B</li>
+ <li><%= "C" %></li>
+ </ul>
+ HTML
+ ERB.parse(example)
+ rescue SyntaxTree::Parser::ParseError => error
+ assert_equal(2, error.lineno)
+ assert_equal(0, error.column)
+ assert_match(/No matching ERB-tag for the <% if %>/, error.message)
  end
 
  def test_missing_erb_block_end_tag
- assert_raises(SyntaxTree::Parser::ParseError) do
- ERB.parse("<% no_end_tag do %>")
- end
+ example = <<-HTML
+ <% no_end_tag do %>
+ <h1>What</h1>
+ HTML
+ ERB.parse(example)
+ rescue SyntaxTree::Parser::ParseError => error
+ assert_equal(1, error.lineno)
+ assert_equal(0, error.column)
+ assert_match(/No matching <% end %> for the <% do %>/, error.message)
  end
 
  def test_missing_erb_case_end_tag
- assert_raises(SyntaxTree::Parser::ParseError) do
- ERB.parse("<% case variabel %>\n<% when 1>\n Hello\n")
- end
+ example = <<-HTML
+ <% case variabel %>
+ <% when 1 %>
+ Hello
+ <% when 2 %>
+ World
+ <h1>What</h1>
+ HTML
+ ERB.parse(example)
+ rescue SyntaxTree::Parser::ParseError => error
+ assert_equal(4, error.lineno)
+ assert_equal(0, error.column)
+ assert_match(/No matching ERB-tag for the <% when %>/, error.message)
  end
 
  def test_erb_code_with_non_ascii
@@ -35,19 +61,42 @@ def test_erb_code_with_non_ascii
  assert_instance_of(SyntaxTree::ERB::ErbNode, parsed.elements.first)
  end
 
- def test_erb_errors
+ def test_erb_syntax_error
  example = <<-HTML
-<ul>
-<% if @items.each do |i|%>
-<li><%= i %></li>
-<% end.blank? %>
-<li>No items</li>
-<% end %>
-</ul>
-HTML
+ <ul>
+ <% if @items.each do |i|%>
+ <li><%= i %></li>
+ <% end.blank? %>
+ <li>No items</li>
+ <% end %>
+ </ul>
+ HTML
  ERB.parse(example)
  rescue SyntaxTree::Parser::ParseError => error
- assert_equal(2, error.lineno)
+ assert_equal(4, error.lineno)
+ assert_equal(0, error.column)
+ assert_match(/Could not parse ERB-tag/, error.message)
+ end
+
+ def test_erb_syntax_error2
+ example = <<-HTML
+ <%= content_tag :header do %>
+ <div class="flex-1 min-w-0">
+ <h2>
+ <%= yield :page_header %>
+ </h2>
+ <%= content_tag :div do %>
+ <%= yield :page_subheader %>
+ <% end if content_for?(:page_subheader) %>
+ </div>
+ <%= content_tag :div do %>
+ <%= yield :page_actions %>
+ <% end if content_for?(:page_actions) %>
+ <% end if content_for?(:page_header) %>
+ HTML
+ ERB.parse(example)
+ rescue SyntaxTree::Parser::ParseError => error
+ assert_equal(8, error.lineno)
  assert_equal(0, error.column)
  assert_match(/Could not parse ERB-tag/, error.message)
  end