From 9838b7ae86a56bc6dff4f1f92a7b69d9645b8ec1 Mon Sep 17 00:00:00 2001 From: Frank Dellaert Date: Mon, 27 Jul 2026 13:49:35 +0200 Subject: [PATCH] Improve interface parser diagnostics --- gtwrap/interface_parser/__init__.py | 28 +++ gtwrap/interface_parser/classes.py | 68 ++++-- gtwrap/interface_parser/diagnostics.py | 290 +++++++++++++++++++++++++ gtwrap/interface_parser/enum.py | 4 +- gtwrap/interface_parser/function.py | 2 +- gtwrap/interface_parser/module.py | 22 +- gtwrap/matlab_wrapper/wrapper.py | 4 +- gtwrap/pybind_wrapper.py | 16 +- scripts/matlab_wrap.py | 7 +- scripts/pybind_wrap.py | 22 +- tests/test_parser_diagnostics.py | 204 +++++++++++++++++ 11 files changed, 633 insertions(+), 34 deletions(-) create mode 100644 gtwrap/interface_parser/diagnostics.py create mode 100644 tests/test_parser_diagnostics.py diff --git a/gtwrap/interface_parser/__init__.py b/gtwrap/interface_parser/__init__.py index 3be52d7d..4a7713d0 100644 --- a/gtwrap/interface_parser/__init__.py +++ b/gtwrap/interface_parser/__init__.py @@ -23,6 +23,8 @@ from .template import * from .tokens import * from .type import * +from .variable import * +from .diagnostics import InterfaceParseError, track_rule as _track_rule # Fix deepcopy issue with pyparsing # Can remove once https://github.com/pyparsing/pyparsing/issues/208 is resolved. @@ -42,4 +44,30 @@ def fixed_get_attr(self, item): # apply the monkey-patch pyparsing.ParseResults.__getattr__ = fixed_get_attr +_DIAGNOSTIC_RULES = ( + (Type.rule, "type", 10), + (TemplatedType.rule, "templated type", 20), + (Argument.rule, "argument", 100), + (ArgumentList.rule, "argument list", 90), + (ReturnType.rule, "return type", 30), + (Method.rule, "method declaration", 80), + (StaticMethod.rule, "static method declaration", 80), + (Constructor.rule, "constructor declaration", 80), + (Operator.rule, "operator declaration", 80), + (DunderMethod.rule, "dunder method declaration", 80), + (Variable.rule, "variable declaration", 70), + (Enumerator.rule, "enumerator", 100), + (Enum.rule, "enum declaration", 70), + (Template.rule, "template declaration", 60), + (TypedefTemplateInstantiation.rule, "typedef declaration", 60), + (ForwardDeclaration.rule, "forward declaration", 60), + (Include.rule, "include declaration", 60), + (Class.rule, "class declaration", 50), + (Namespace.rule, "namespace declaration", 40), + (GlobalFunction.rule, "global function declaration", 50), +) + +for _rule, _context, _priority in _DIAGNOSTIC_RULES: + _track_rule(_rule, _context, _priority) + pyparsing.ParserElement.enablePackrat() diff --git a/gtwrap/interface_parser/classes.py b/gtwrap/interface_parser/classes.py index 8967bea9..1624607c 100644 --- a/gtwrap/interface_parser/classes.py +++ b/gtwrap/interface_parser/classes.py @@ -23,6 +23,7 @@ from .type import TemplatedType, Typename from .utils import collect_namespaces from .variable import Variable +from .diagnostics import semantic_error class Method: @@ -133,18 +134,23 @@ class Constructor: + ArgumentList.rule("args_list") # + RPAREN # + SEMI_COLON # BR - ).setParseAction(lambda t: Constructor(t.name, t.args_list, t.template)) + ).setParseAction(lambda s, loc, t: Constructor( + t.name, t.args_list, t.template, source=s, location=loc)) def __init__(self, name: str, args: ArgumentList, template: Union[Template, Any], - parent: Union["Class", Any] = ''): + parent: Union["Class", Any] = '', + source: str = '', + location: int = 0): self.name = name self.args = args self.template = template self.parent = parent + self.source = source + self.location = location def __repr__(self) -> str: return "Constructor: {}{}".format(self.name, self.args) @@ -169,8 +175,15 @@ class Overload { + RPAREN # + CONST("is_const") # + SEMI_COLON # BR - ).setParseAction(lambda t: Operator(t.name, t.operator, t.return_type, t. - args_list, t.is_const)) + ).setParseAction(lambda s, loc, t: Operator( + t.name, + t.operator, + t.return_type, + t.args_list, + t.is_const, + source=s, + location=loc, + )) def __init__(self, name: str, @@ -178,7 +191,9 @@ def __init__(self, return_type: ReturnType, args: ArgumentList, is_const: str, - parent: Union["Class", Any] = ''): + parent: Union["Class", Any] = '', + source: str = '', + location: int = 0): self.name = name self.operator = operator self.return_type = return_type @@ -187,21 +202,41 @@ def __init__(self, self.is_unary = len(args) == 0 self.parent = parent + self.source = source + self.location = location # Check for valid unary operators if self.is_unary and self.operator not in ('+', '-'): - raise ValueError("Invalid unary operator {} used for {}".format( - self.operator, self)) + raise semantic_error( + source, + location, + "operator declaration", + f"unsupported unary operator '{self.operator}'", + ) # Check that number of arguments are either 0 or 1 - assert 0 <= len(args) < 2, \ - "Operator overload should be at most 1 argument, " \ - "{} arguments provided".format(len(args)) + if not 0 <= len(args) < 2: + raise semantic_error( + source, + location, + "operator declaration", + "operator overload must have at most one argument; " + f"found {len(args)}", + ) # Check to ensure arg and return type are the same. if len(args) == 1 and self.operator not in ("()", "[]"): - assert args.list()[0].ctype.typename.name == return_type.type1.typename.name, \ - "Mixed type overloading not supported. Both arg and return type must be the same." + argument_type = args.list()[0].ctype.typename.name + return_type_name = return_type.type1.typename.name + if argument_type != return_type_name: + raise semantic_error( + source, + location, + "operator declaration", + "mixed-type operator overloads are unsupported; " + f"argument type '{argument_type}' does not match return " + f"type '{return_type_name}'", + ) def __repr__(self) -> str: return "Operator: {}{}{}({}) {}".format( @@ -345,8 +380,13 @@ def __init__( # Make sure ctors' names and class name are the same. for ctor in self.ctors: if ctor.name != self.name: - raise ValueError("Error in constructor name! {} != {}".format( - ctor.name, self.name)) + raise semantic_error( + ctor.source, + ctor.location, + "constructor declaration", + f"constructor name '{ctor.name}' must match class " + f"name '{self.name}'", + ) for ctor in self.ctors: ctor.parent = self diff --git a/gtwrap/interface_parser/diagnostics.py b/gtwrap/interface_parser/diagnostics.py new file mode 100644 index 00000000..9fe00ce3 --- /dev/null +++ b/gtwrap/interface_parser/diagnostics.py @@ -0,0 +1,290 @@ +""" +Diagnostics for errors in wrapper interface files. + +The interface grammar contains optional and repeated expressions which may +backtrack after a declaration has partially matched. Pyparsing then reports +the error from the enclosing expression, losing the more useful nested +failure. This module records those nested failures during a module parse and +turns the best one into a source-aware exception. +""" + +from __future__ import annotations + +from contextvars import ContextVar, Token +from dataclasses import dataclass +import re +from typing import Optional + +from pyparsing import (ParseBaseException, ParserElement, cpp_style_comment, + quoted_string) + + +@dataclass(frozen=True) +class _Failure: + """A named grammar failure observed during a parse.""" + + context: str + attempted_location: int + exception: ParseBaseException + priority: int + sequence: int + + @property + def location(self) -> int: + """Location at which the underlying expression failed.""" + return self.exception.loc + + @property + def progress(self) -> int: + """Number of characters consumed before the expression failed.""" + return self.location - self.attempted_location + + +class _DiagnosticContext: + """Failure collection state for one module parse.""" + + def __init__(self, source: str, source_name: str): + self.source = source + self.source_name = source_name + self.failures: list[_Failure] = [] + + def record(self, context: str, attempted_location: int, + exception: ParseBaseException, priority: int) -> None: + self.failures.append( + _Failure( + context=context, + attempted_location=attempted_location, + exception=exception, + priority=priority, + sequence=len(self.failures), + )) + + def best_failure(self, fallback: ParseBaseException) -> _Failure: + """Select the farthest, most specific credible parser failure.""" + fallback_failure = _Failure( + context="interface declaration", + attempted_location=fallback.loc, + exception=fallback, + priority=0, + sequence=len(self.failures), + ) + return max( + [*self.failures, fallback_failure], + key=lambda failure: ( + failure.location, + failure.priority, + failure.progress, + failure.sequence, + ), + ) + + +_ACTIVE_CONTEXT: ContextVar[Optional[_DiagnosticContext]] = ContextVar( + "gtwrap_interface_parser_diagnostic_context", default=None) + + +class InterfaceParseError(Exception): + """A source-aware syntax or semantic error in a wrapper interface file.""" + + def __init__( + self, + *, + source_name: str, + source: str, + location: int, + context: str, + expected: str, + hint: Optional[str] = None, + cause: Optional[BaseException] = None, + ): + self.source_name = source_name + self.source = source + self.location = max(0, min(location, len(source))) + self.context = context + self.expected = expected + self.hint = hint + self.cause = cause + + self.line, self.column, self.line_text = _source_location( + source, self.location) + super().__init__(self._summary()) + + def _summary(self) -> str: + return f"parse error in {self.context}: {self.expected}" + + def __str__(self) -> str: + location = ( + f"{self.source_name}:{self.line}:{self.column}: {self._summary()}") + if not self.line_text: + return location + + line_number = str(self.line) + gutter_width = len(line_number) + expanded_line = self.line_text.expandtabs(4) + prefix = self.line_text[:self.column - 1].expandtabs(4) + caret = " " * len(prefix) + "^" + rendered = [ + location, + f" {line_number:>{gutter_width}} | {expanded_line}", + f" {'':>{gutter_width}} | {caret}", + ] + if self.hint: + rendered.append(f"hint: {self.hint}") + return "\n".join(rendered) + + @classmethod + def from_failure(cls, context: _DiagnosticContext, + fallback: ParseBaseException) -> "InterfaceParseError": + """Create an error from the best failure recorded for a parse.""" + failure = context.best_failure(fallback) + failure_context, expected, hint = _readable_expectation( + failure.context, + failure.exception.msg, + context.source, + failure.location, + ) + return cls( + source_name=context.source_name, + source=context.source, + location=failure.location, + context=failure_context, + expected=expected, + hint=hint, + cause=failure.exception, + ) + + +def begin_diagnostics(source: str, + source_name: str) -> tuple[_DiagnosticContext, Token]: + """Start collecting parser failures for a module parse.""" + context = _DiagnosticContext(source, source_name) + return context, _ACTIVE_CONTEXT.set(context) + + +def end_diagnostics(token: Token) -> None: + """Restore the diagnostic state active before a module parse.""" + _ACTIVE_CONTEXT.reset(token) + + +def semantic_error(source: str, + location: int, + context: str, + message: str, + hint: Optional[str] = None) -> InterfaceParseError: + """Create a structured error from a semantic parse-action check.""" + active = _ACTIVE_CONTEXT.get() + source_name = active.source_name if active else "" + return InterfaceParseError( + source_name=source_name, + source=source, + location=location, + context=context, + expected=message, + hint=hint, + ) + + +def track_rule(rule: ParserElement, context: str, priority: int) -> None: + """Record failures of a meaningful grammar rule while parsing a module.""" + + def record_failure(_source: str, location: int, _expression: ParserElement, + exception: ParseBaseException) -> None: + active = _ACTIVE_CONTEXT.get() + if active is not None: + active.record(context, location, exception, priority) + + rule.set_fail_action(record_failure) + + +def _source_location(source: str, location: int) -> tuple[int, int, str]: + """Return one-based line and column plus the containing source line.""" + line_number = source.count("\n", 0, location) + 1 + line_start = source.rfind("\n", 0, location) + 1 + line_end = source.find("\n", location) + if line_end == -1: + line_end = len(source) + return line_number, location - line_start + 1, source[line_start:line_end] + + +def _readable_expectation( + context: str, message: str, source: str, + location: int) -> tuple[str, str, Optional[str]]: + """Convert pyparsing's grammar representation into a user-facing error.""" + found = source[location:location + 1] + previous = source[:location].rstrip() + + if location == len(source): + unclosed = _last_unclosed_delimiter(source) + if unclosed: + opening, closing = unclosed + return ( + "interface declaration", + f"expected '{closing}' to close '{opening}'", + None, + ) + + access_specifier = re.search(r"\b(public|private|protected)\s*$", previous) + if found == ":" and access_specifier: + access = access_specifier.group(1) + return ( + "class member declaration", + f"access specifier '{access}:' is not supported", + "list public wrapper declarations without an access label", + ) + + if context == "argument": + if found in (",", ")"): + return ( + context, + "expected argument name", + "wrapper interface arguments require names, for example " + "'const Pose3& pose'", + ) + return context, "expected an argument type followed by its name", None + + if context == "enumerator": + hint = None + if found == "}" and previous.endswith(","): + hint = "remove the trailing comma after the final enumerator" + return context, "expected enumerator name", hint + + if context == "templated type" and found not in ("", ",", ">"): + return context, "expected ',' or '>' after template argument", None + + expected = message + if expected.startswith("Expected "): + expected = expected[len("Expected "):] + expected = expected.replace("string_end", "end of file") + + if expected.startswith(("{", "[", "Forward:")) or "W:(" in expected: + expected = context + + if expected == "';'": + expected = f"';' after {context}" + + return context, f"expected {expected}", None + + +def _last_unclosed_delimiter(source: str) -> Optional[tuple[str, str]]: + """Return the final unmatched delimiter in source, if there is one.""" + closing_for = {"(": ")", "[": "]", "{": "}"} + opening_for = {closing: opening for opening, closing in closing_for.items()} + ignored_ranges = iter( + (start, end) for _, start, end in + (cpp_style_comment | quoted_string).scan_string(source)) + ignored = next(ignored_ranges, None) + stack: list[str] = [] + for location, character in enumerate(source): + while ignored and location >= ignored[1]: + ignored = next(ignored_ranges, None) + if ignored and ignored[0] <= location < ignored[1]: + continue + if character in closing_for: + stack.append(character) + elif character in opening_for: + if stack and stack[-1] == opening_for[character]: + stack.pop() + if not stack: + return None + opening = stack[-1] + return opening, closing_for[opening] diff --git a/gtwrap/interface_parser/enum.py b/gtwrap/interface_parser/enum.py index 8f875a58..e6c26c13 100644 --- a/gtwrap/interface_parser/enum.py +++ b/gtwrap/interface_parser/enum.py @@ -21,8 +21,8 @@ class Enumerator: """ Rule to parse an enumerator inside an enum. """ - rule = ( - IDENT("enumerator")).setParseAction(lambda t: Enumerator(t.enumerator)) + rule = (IDENT.copy().set_name("enumerator name")("enumerator") + ).setParseAction(lambda t: Enumerator(t.enumerator)) def __init__(self, name): self.name = name diff --git a/gtwrap/interface_parser/function.py b/gtwrap/interface_parser/function.py index b8a97e85..3c54d929 100644 --- a/gtwrap/interface_parser/function.py +++ b/gtwrap/interface_parser/function.py @@ -30,7 +30,7 @@ class Argument: ``` """ rule = ((Type.rule ^ TemplatedType.rule)("ctype") # - + IDENT("name") # + + IDENT.copy().set_name("argument name")("name") # + Optional(EQUAL + DEFAULT_ARG)("default") ).setParseAction(lambda t: Argument( t.ctype, # diff --git a/gtwrap/interface_parser/module.py b/gtwrap/interface_parser/module.py index 7912c40d..5ef0d3a4 100644 --- a/gtwrap/interface_parser/module.py +++ b/gtwrap/interface_parser/module.py @@ -12,7 +12,7 @@ # pylint: disable=unnecessary-lambda, unused-import, expression-not-assigned, no-else-return, protected-access, too-few-public-methods, too-many-arguments -from pyparsing import (ParseResults, ZeroOrMore, # type: ignore +from pyparsing import (ParseBaseException, ParseResults, ZeroOrMore, # type: ignore cppStyleComment, stringEnd) from .classes import Class @@ -51,6 +51,20 @@ class Module: rule.ignore(cppStyleComment) @staticmethod - def parseString(s: str) -> ParseResults: - """Parse the source string and apply the rules.""" - return Module.rule.parseString(s)[0] + def parseString(s: str, source_name: str = "") -> ParseResults: + """Parse source text and report any failure at its best known location.""" + # Imported here to avoid adding the diagnostic machinery to the grammar's + # import cycle. + from .diagnostics import (InterfaceParseError, begin_diagnostics, + end_diagnostics) + + context, token = begin_diagnostics(s, source_name) + try: + return Module.rule.parseString(s)[0] + except InterfaceParseError: + raise + except ParseBaseException as error: + diagnostic = InterfaceParseError.from_failure(context, error) + raise diagnostic from error + finally: + end_diagnostics(token) diff --git a/gtwrap/matlab_wrapper/wrapper.py b/gtwrap/matlab_wrapper/wrapper.py index 49927fc6..79341c26 100755 --- a/gtwrap/matlab_wrapper/wrapper.py +++ b/gtwrap/matlab_wrapper/wrapper.py @@ -1964,7 +1964,9 @@ def wrap(self, files, path): content += f.read() # Parse the contents of the interface file - parsed_result = parser.Module.parseString(content) + source_name = files[0] if len(files) == 1 else ";".join(files) + parsed_result = parser.Module.parseString( + content, source_name=source_name) # Instantiate the module module = instantiator.instantiate_namespace(parsed_result) diff --git a/gtwrap/pybind_wrapper.py b/gtwrap/pybind_wrapper.py index d58ba688..7e969932 100755 --- a/gtwrap/pybind_wrapper.py +++ b/gtwrap/pybind_wrapper.py @@ -726,7 +726,11 @@ def wrap_namespace(self, namespace): return wrapped, includes - def wrap_file(self, content, module_name=None, submodules=None): + def wrap_file(self, + content, + module_name=None, + submodules=None, + source_name=""): """ Wrap the code in the interface file. @@ -734,9 +738,10 @@ def wrap_file(self, content, module_name=None, submodules=None): content: The contents of the interface file. module_name: The name of the module. submodules: List of other interface file names that should be linked to. + source_name: Name of the interface file for parser diagnostics. """ # Parse the contents of the interface file - module = parser.Module.parseString(content) + module = parser.Module.parseString(content, source_name=source_name) # Instantiate all templates module = instantiator.instantiate_namespace(module) @@ -807,7 +812,9 @@ def wrap_submodule(self, source): with open(source, "r", encoding="UTF-8") as f: content = f.read() # Wrap the read-in content - cc_content = self.wrap_file(content, module_name=module_name) + cc_content = self.wrap_file(content, + module_name=module_name, + source_name=source) # Generate the C++ code which Pybind11 will use. with open(filename.replace(".i", ".cpp"), "w", encoding="UTF-8") as f: @@ -834,7 +841,8 @@ def wrap(self, sources, main_module_name): content = f.read() cc_content = self.wrap_file(content, module_name=self.module_name, - submodules=submodules) + submodules=submodules, + source_name=main_module) # Generate the C++ code which Pybind11 will use. with open(main_module_name, "w", encoding="UTF-8") as f: diff --git a/scripts/matlab_wrap.py b/scripts/matlab_wrap.py index 3275667f..036542df 100644 --- a/scripts/matlab_wrap.py +++ b/scripts/matlab_wrap.py @@ -8,6 +8,7 @@ import argparse import sys +from gtwrap.interface_parser import InterfaceParseError from gtwrap.matlab_wrapper import MatlabWrapper if __name__ == "__main__": @@ -63,4 +64,8 @@ use_boost_serialization=args.use_boost_serialization) sources = args.src.split(';') - cc_content = wrapper.wrap(sources, path=args.out) + try: + cc_content = wrapper.wrap(sources, path=args.out) + except InterfaceParseError as error: + print(error, file=sys.stderr) + sys.exit(1) diff --git a/scripts/pybind_wrap.py b/scripts/pybind_wrap.py index 41f39b40..4de783d9 100644 --- a/scripts/pybind_wrap.py +++ b/scripts/pybind_wrap.py @@ -8,7 +8,9 @@ # pylint: disable=import-error import argparse +import sys +from gtwrap.interface_parser import InterfaceParseError from gtwrap.pybind_wrapper import PybindWrapper @@ -86,14 +88,20 @@ def main(): xml_source=args.xml_source, ) - if args.is_submodule: - wrapper.wrap_submodule(args.src) + try: + if args.is_submodule: + wrapper.wrap_submodule(args.src) - else: - # Wrap the code and get back the cpp/cc code. - sources = args.src.split(';') - wrapper.wrap(sources, args.out) + else: + # Wrap the code and get back the cpp/cc code. + sources = args.src.split(';') + wrapper.wrap(sources, args.out) + except InterfaceParseError as error: + print(error, file=sys.stderr) + return 1 + + return 0 if __name__ == "__main__": - main() + sys.exit(main()) diff --git a/tests/test_parser_diagnostics.py b/tests/test_parser_diagnostics.py new file mode 100644 index 00000000..7178bab0 --- /dev/null +++ b/tests/test_parser_diagnostics.py @@ -0,0 +1,204 @@ +"""Tests for source-aware wrapper interface parser diagnostics.""" + +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + +from gtwrap.interface_parser import InterfaceParseError, Module + + +class TestParserDiagnostics(unittest.TestCase): + """Verify syntax and semantic failures identify the useful source token.""" + + def assert_parse_error(self, + source, + *, + line, + column, + context, + expected, + source_name="example.i"): + """Parse source and assert the structured diagnostic fields.""" + with self.assertRaises(InterfaceParseError) as raised: + Module.parseString(source, source_name=source_name) + + error = raised.exception + self.assertEqual(error.source_name, source_name) + self.assertEqual(error.line, line) + self.assertEqual(error.column, column) + self.assertEqual(error.context, context) + self.assertEqual(error.expected, expected) + self.assertIn(f"{source_name}:{line}:{column}", str(error)) + self.assertIn("^", str(error)) + return error + + def test_missing_method_semicolon(self): + self.assert_parse_error( + "class Foo { void bar() } ;", + line=1, + column=24, + context="method declaration", + expected="expected ';' after method declaration", + ) + + def test_missing_argument_name(self): + source = """namespace gtsam { +class Foo { + void project(const Pose3&); +}; +} +""" + error = self.assert_parse_error( + source, + line=3, + column=28, + context="argument", + expected="expected argument name", + ) + self.assertEqual(error.line_text, + " void project(const Pose3&);") + self.assertIn("arguments require names", error.hint) + self.assertIsNotNone(error.cause) + + def test_malformed_template(self): + self.assert_parse_error( + "class Foo { void bar(std::vector