LibXML: Include line and column numbers in parse error messages

Author: https://github.com/tcl3
Commit: c0f30f31d1
Pull-request: https://github.com/LadybirdBrowser/ladybird/pull/943
Reviewed-by: https://github.com/alimpfard ✅

Tests/LibWeb/Text/data/bad.xml

@@ -1 +1,2 @@
-<this-will-not-parse
+<?xml version="1.0" encoding="UTF-8"?>
+<this-will-not-parse</this-will-not-parse>

Tests/LibWeb/Text/expected/XML/error-page.txt

@@ -1,3 +1,3 @@
   Got load event
 [object HTMLDocument]
-Failed to parse XML document: Expected '>' at offset 21
+Failed to parse XML document: Expected '>' at line: 1, col: 20 (offset 59)

Userland/Libraries/LibXML/Parser/Parser.cpp

@@ -195,7 +195,7 @@ ErrorOr<void, ParseError> Parser::skip_whitespace(Required required)
     // S ::= (#x20 | #x9 | #xD | #xA)+
     auto matched = m_lexer.consume_while(is_any_of("\x20\x09\x0d\x0a"sv));
     if (required == Required::Yes && matched.is_empty())
-        return parse_error(m_lexer.tell(), "Expected whitespace");
+        return parse_error(m_lexer.current_position(), "Expected whitespace");
 
     rollback.disarm();
     return {};
@@ -220,12 +220,12 @@ ErrorOr<void, ParseError> Parser::parse_internal()
     auto matched_source = m_source.substring_view(0, m_lexer.tell());
     if (auto it = find_if(matched_source.begin(), matched_source.end(), s_restricted_characters); !it.is_end()) {
         return parse_error(
-            it.index(),
+            m_lexer.position_for(it.index()),
             ByteString::formatted("Invalid character #{:x} used in document", *it));
     }
 
     if (!m_lexer.is_eof())
-        return parse_error(m_lexer.tell(), "Garbage after document");
+        return parse_error(m_lexer.current_position(), "Garbage after document");
 
     return {};
 }
@@ -236,7 +236,7 @@ ErrorOr<void, ParseError> Parser::expect(StringView expected)
 
     if (!m_lexer.consume_specific(expected)) {
         if (m_options.treat_errors_as_fatal)
-            return parse_error(m_lexer.tell(), ByteString::formatted("Expected '{}'", expected));
+            return parse_error(m_lexer.current_position(), ByteString::formatted("Expected '{}'", expected));
     }
 
     rollback.disarm();
@@ -250,7 +250,7 @@ requires(IsCallableWithArguments<Pred, bool, char>) ErrorOr<StringView, ParseErr
     auto start = m_lexer.tell();
     if (!m_lexer.next_is(predicate)) {
         if (m_options.treat_errors_as_fatal)
-            return parse_error(m_lexer.tell(), ByteString::formatted("Expected {}", description));
+            return parse_error(m_lexer.current_position(), ByteString::formatted("Expected {}", description));
     }
 
     m_lexer.ignore();
@@ -271,7 +271,7 @@ requires(IsCallableWithArguments<Pred, bool, char>) ErrorOr<StringView, ParseErr
 
     if (m_lexer.tell() == start) {
         if (m_options.treat_errors_as_fatal) {
-            return parse_error(m_lexer.tell(), ByteString::formatted("Expected {}", description));
+            return parse_error(m_lexer.current_position(), ByteString::formatted("Expected {}", description));
         }
     }
 
@@ -353,7 +353,7 @@ ErrorOr<void, ParseError> Parser::parse_version_info()
         m_in_compatibility_mode = true;
     } else {
         if (version_string != "1.1" && m_options.treat_errors_as_fatal)
-            return parse_error(m_lexer.tell(), ByteString::formatted("Expected '1.1', found '{}'", version_string));
+            return parse_error(m_lexer.current_position(), ByteString::formatted("Expected '1.1', found '{}'", version_string));
     }
 
     m_version = Version::Version11;
@@ -415,7 +415,7 @@ ErrorOr<void, ParseError> Parser::parse_standalone_document_decl()
 
     auto value = m_lexer.consume_quoted_string();
     if (!value.is_one_of("yes", "no"))
-        return parse_error(m_lexer.tell() - value.length(), "Expected one of 'yes' or 'no'");
+        return parse_error(m_lexer.position_for(m_lexer.tell() - value.length()), "Expected one of 'yes' or 'no'");
 
     m_standalone = value == "yes";
 
@@ -445,7 +445,7 @@ ErrorOr<void, ParseError> Parser::parse_misc()
         return {};
     }
 
-    return parse_error(m_lexer.tell(), "Expected a match for 'Misc', but found none");
+    return parse_error(m_lexer.current_position(), "Expected a match for 'Misc', but found none");
 }
 
 // 2.5.15 Comment, https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-Comment
@@ -521,7 +521,7 @@ ErrorOr<Name, ParseError> Parser::parse_processing_instruction_target()
 
     if (target.equals_ignoring_ascii_case("xml"sv) && m_options.treat_errors_as_fatal) {
         return parse_error(
-            m_lexer.tell() - target.length(),
+            m_lexer.position_for(m_lexer.tell() - target.length()),
             "Use of the reserved 'xml' name for processing instruction target name is disallowed");
     }
 
@@ -579,7 +579,7 @@ ErrorOr<void, ParseError> Parser::parse_doctype_decl()
                     auto declarations = TRY(parse_external_subset());
                     if (!m_lexer.is_eof()) {
                         return parse_error(
-                            m_lexer.tell(),
+                            m_lexer.current_position(),
                             ByteString::formatted("Failed to resolve external subset '{}': garbage after declarations", doctype.external_id->system_id.system_literal));
                     }
                     doctype.markup_declarations.extend(move(declarations));
@@ -634,7 +634,7 @@ ErrorOr<void, ParseError> Parser::parse_element()
 
     // Well-formedness constraint: The Name in an element's end-tag MUST match the element type in the start-tag.
     if (m_options.treat_errors_as_fatal && closing_name != tag.name)
-        return parse_error(tag_location, "Invalid closing tag");
+        return parse_error(m_lexer.position_for(tag_location), "Invalid closing tag");
 
     rollback.disarm();
     return {};
@@ -720,7 +720,7 @@ ErrorOr<ByteString, ParseError> Parser::parse_attribute_value_inner(StringView d
 
         if (m_lexer.next_is('<')) {
             // Not allowed, return a nice error to make it easier to debug.
-            return parse_error(m_lexer.tell(), "Unescaped '<' not allowed in attribute values");
+            return parse_error(m_lexer.current_position(), "Unescaped '<' not allowed in attribute values");
         }
 
         if (m_lexer.next_is('&')) {
@@ -774,7 +774,7 @@ ErrorOr<Variant<Parser::EntityReference, ByteString>, ParseError> Parser::parse_
         }
 
         if (!code_point.has_value() || !s_characters.contains(*code_point))
-            return parse_error(reference_start, "Invalid character reference");
+            return parse_error(m_lexer.position_for(reference_start), "Invalid character reference");
 
         TRY(expect(";"sv));
 
@@ -995,7 +995,7 @@ ErrorOr<Optional<MarkupDeclaration>, ParseError> Parser::parse_markup_declaratio
         return Optional<MarkupDeclaration> {};
     }
 
-    return parse_error(m_lexer.tell(), "Expected one of elementdecl, attlistdecl, entitydecl, notationdecl, PI or comment");
+    return parse_error(m_lexer.current_position(), "Expected one of elementdecl, attlistdecl, entitydecl, notationdecl, PI or comment");
 }
 
 // 2.8.28a DeclSep, https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-DeclSep
@@ -1016,7 +1016,7 @@ ErrorOr<Optional<ByteString>, ParseError> Parser::parse_declaration_separator()
         return Optional<ByteString> {};
     }
 
-    return parse_error(m_lexer.tell(), "Expected either whitespace, or a PEReference");
+    return parse_error(m_lexer.current_position(), "Expected either whitespace, or a PEReference");
 }
 
 // 4.1.69 PEReference, https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-PEReference
@@ -1269,7 +1269,7 @@ ErrorOr<ElementDeclaration::ContentSpec, ParseError> Parser::parse_content_spec(
                     if (auto result = parse_name(); !result.is_error())
                         names.set(result.release_value());
                     else
-                        return parse_error(m_lexer.tell(), "Expected a Name");
+                        return parse_error(m_lexer.current_position(), "Expected a Name");
                 }
                 TRY(skip_whitespace());
                 TRY(expect(")*"sv));
@@ -1331,7 +1331,7 @@ ErrorOr<ElementDeclaration::ContentSpec, ParseError> Parser::parse_content_spec(
                 TRY(expect(")"sv));
 
                 if (choices.size() < 2)
-                    return parse_error(m_lexer.tell(), "Expected more than one choice");
+                    return parse_error(m_lexer.current_position(), "Expected more than one choice");
 
                 TRY(skip_whitespace());
                 auto qualifier = parse_qualifier();
@@ -1709,7 +1709,7 @@ ErrorOr<ByteString, ParseError> Parser::resolve_reference(EntityReference const&
 {
     static HashTable<Name> reference_lookup {};
     if (reference_lookup.contains(reference.name))
-        return parse_error(m_lexer.tell(), ByteString::formatted("Invalid recursive definition for '{}'", reference.name));
+        return parse_error(m_lexer.current_position(), ByteString::formatted("Invalid recursive definition for '{}'", reference.name));
 
     reference_lookup.set(reference.name);
     ScopeGuard remove_lookup {
@@ -1737,17 +1737,17 @@ ErrorOr<ByteString, ParseError> Parser::resolve_reference(EntityReference const&
                 },
                 [&](EntityDefinition const& definition) -> ErrorOr<void, ParseError> {
                     if (placement == ReferencePlacement::AttributeValue)
-                        return parse_error(m_lexer.tell(), ByteString::formatted("Attribute references external entity '{}'", reference.name));
+                        return parse_error(m_lexer.current_position(), ByteString::formatted("Attribute references external entity '{}'", reference.name));
 
                     if (definition.notation.has_value())
-                        return parse_error(0u, ByteString::formatted("Entity reference to unparsed entity '{}'", reference.name));
+                        return parse_error(m_lexer.position_for(0), ByteString::formatted("Entity reference to unparsed entity '{}'", reference.name));
 
                     if (!m_options.resolve_external_resource)
-                        return parse_error(0u, ByteString::formatted("Failed to resolve external entity '{}'", reference.name));
+                        return parse_error(m_lexer.position_for(0), ByteString::formatted("Failed to resolve external entity '{}'", reference.name));
 
                     auto result = m_options.resolve_external_resource(definition.id.system_id, definition.id.public_id);
                     if (result.is_error())
-                        return parse_error(0u, ByteString::formatted("Failed to resolve external entity '{}': {}", reference.name, result.error()));
+                        return parse_error(m_lexer.position_for(0), ByteString::formatted("Failed to resolve external entity '{}': {}", reference.name, result.error()));
 
                     resolved = result.release_value();
                     return {};
@@ -1767,7 +1767,7 @@ ErrorOr<ByteString, ParseError> Parser::resolve_reference(EntityReference const&
             return "'";
         if (reference.name == "quot")
             return "\"";
-        return parse_error(0u, ByteString::formatted("Reference to undeclared entity '{}'", reference.name));
+        return parse_error(m_lexer.position_for(0), ByteString::formatted("Reference to undeclared entity '{}'", reference.name));
     }
 
     StringView resolved_source = *resolved;

Userland/Libraries/LibXML/Parser/Parser.h

@@ -22,7 +22,7 @@
 namespace XML {
 
 struct ParseError {
-    size_t offset;
+    LineTrackingLexer::Position position {};
     ByteString error;
 };
 
@@ -185,7 +185,7 @@ private:
             if (rule_name.starts_with("parse_"sv))
                 rule_name = rule_name.substring_view(6);
             m_parse_errors.append({
-                error.offset,
+                error.position,
                 ByteString::formatted("{}: {}", rule_name, error.error),
             });
         }
@@ -219,6 +219,6 @@ template<>
 struct AK::Formatter<XML::ParseError> : public AK::Formatter<FormatString> {
     ErrorOr<void> format(FormatBuilder& builder, XML::ParseError const& error)
     {
-        return Formatter<FormatString>::format(builder, "{} at offset {}"sv, error.error, error.offset);
+        return Formatter<FormatString>::format(builder, "{} at line: {}, col: {} (offset {})"sv, error.error, error.position.line, error.position.column, error.position.offset);
     }
 };