From c5b0ded0fbab1852a06384d8244e75ef6d5fe482 Mon Sep 17 00:00:00 2001 From: Schamper <1254028+Schamper@users.noreply.github.com> Date: Thu, 5 Nov 2026 23:25:37 +0100 Subject: [PATCH] Improve error messages for expressions --- dissect/cstruct/exception.py | 4 ++ dissect/cstruct/expression.py | 32 +++++++--------- dissect/cstruct/lexer.py | 15 +++++--- dissect/cstruct/parser.py | 32 ++++++++++------ tests/test_compiler.py | 4 +- tests/test_expression.py | 18 ++++----- tests/test_parser.py | 72 +++++++++++++++++++++++++++++++++++ 7 files changed, 132 insertions(+), 45 deletions(-) diff --git a/dissect/cstruct/exception.py b/dissect/cstruct/exception.py index f837421..0e5508f 100644 --- a/dissect/cstruct/exception.py +++ b/dissect/cstruct/exception.py @@ -27,3 +27,7 @@ class ArraySizeError(Error): class ExpressionParserError(Error): pass + + +class UnknownIdentifierError(ExpressionParserError): + pass diff --git a/dissect/cstruct/expression.py b/dissect/cstruct/expression.py index 72c372b..28ab24d 100644 --- a/dissect/cstruct/expression.py +++ b/dissect/cstruct/expression.py @@ -2,8 +2,8 @@ from typing import TYPE_CHECKING, Any -from dissect.cstruct.exception import ExpressionParserError -from dissect.cstruct.lexer import IDENTIFIER_TYPES, Lexer, TokenCursor, TokenType +from dissect.cstruct.exception import ExpressionParserError, UnknownIdentifierError +from dissect.cstruct.lexer import IDENTIFIER_TYPES, Lexer, TokenCursor, TokenType, join_tokens from dissect.cstruct.util import offsetof, sizeof if TYPE_CHECKING: @@ -87,11 +87,7 @@ def _offsetof(cs: cstruct, name: str, field: str) -> int: } -def _error(msg: str, line: int) -> ExpressionParserError: - return ExpressionParserError(f"line {line}: {msg}") - - -def _identifier(cs: cstruct, context: dict[str, Any], ident: str, line: int) -> int: +def _identifier(cs: cstruct, context: dict[str, Any], ident: str) -> int: """Resolve an identifier to an integer value.""" obj = None for i, part in enumerate(ident.split(".")): @@ -103,16 +99,16 @@ def _identifier(cs: cstruct, context: dict[str, Any], ident: str, line: int) -> elif part in cs.types: obj = cs.types[part] else: - raise _error(f"Unknown identifier: '{ident}'", line) + raise UnknownIdentifierError(f"Unknown identifier: '{ident}'") else: obj = obj.get(part, _MISSING) if isinstance(obj, dict) else getattr(obj, part, _MISSING) if obj is _MISSING: - raise _error(f"Unknown identifier: '{ident}'", line) + raise UnknownIdentifierError(f"Unknown identifier: '{ident}'") try: return int(obj) except (ValueError, TypeError): - raise _error(f"Identifier '{ident}' does not resolve to an integer", line) + raise ExpressionParserError(f"Identifier '{ident}' does not resolve to an integer") # Globals used by the source code of an expression @@ -129,8 +125,8 @@ def __init__(self, cs: cstruct, context: dict[str, Any]): def number(self, value: int) -> int: return value - def identifier(self, ident: str, token: Token) -> int: - return _identifier(self.cs, self.context, ident, token.line) + def identifier(self, ident: str) -> int: + return _identifier(self.cs, self.context, ident) def unary(self, operator: TokenType, value: int) -> int: return UNARY_OPERATORS[operator](value) @@ -152,10 +148,10 @@ def __init__(self, variables: dict[str, str], context: str): def number(self, value: int) -> str: return repr(value) - def identifier(self, ident: str, token: Token) -> str: + def identifier(self, ident: str) -> str: if ident in self.variables: return self.variables[ident] - return f"_identifier(cs, {self.context}, {ident!r}, {token.line})" + return f"_identifier(cs, {self.context}, {ident!r})" def unary(self, operator: TokenType, value: str) -> str: return f"({OPERATOR_SOURCE[operator]}{value})" @@ -195,7 +191,7 @@ def _reset(self) -> None: self._queue = [] def _error(self, msg: str, *, token: Token | None = None) -> ExpressionParserError: - return _error(msg, (token if token is not None else self._current()).line) + return ExpressionParserError(msg) def _evaluate_expression(self, backend: _Evaluator | _SourceBuilder) -> None: operator = self._stack.pop(-1) @@ -266,18 +262,18 @@ def _evaluate(self, backend: _Evaluator | _SourceBuilder) -> int | str: num_args = FUNCTION_TOKENS[func] while num_args > 1: - self._queue.append(self._collect_until(TokenType.COMMA)) + self._queue.append(join_tokens(self._collect_until(TokenType.COMMA))) self._expect(TokenType.COMMA) num_args -= 1 - self._queue.append(self._collect_until(TokenType.RPAREN)) + self._queue.append(join_tokens(self._collect_until(TokenType.RPAREN))) self._expect(TokenType.RPAREN) # Evaluate immediately self._evaluate_expression(backend) elif token.type in IDENTIFIER_TYPES: - self._queue.append(backend.identifier(self._take().value, token)) + self._queue.append(backend.identifier(self._take().value)) elif token.type == TokenType.LPAREN: if self._previous().type == TokenType.NUMBER: diff --git a/dissect/cstruct/lexer.py b/dissect/cstruct/lexer.py index 0f0a492..baa500e 100644 --- a/dissect/cstruct/lexer.py +++ b/dissect/cstruct/lexer.py @@ -143,6 +143,11 @@ def format_error_context(data: str, lineno: int, window: int = 1) -> str: return "\n".join(f"{start + i + 1:>{no_len}}: {line}" for i, line in enumerate(context)) +def join_tokens(tokens: list[Token]) -> str: + """Join the values of the tokens with spaces, e.g. to evaluate them as an expression.""" + return " ".join(token.value for token in tokens) + + class Lexer: """Lexer compatible with C-like syntax for struct definitions and preprocessor directives.""" @@ -566,11 +571,11 @@ def _expect(self, *types: TokenType) -> Token: raise self._error(f"expected {expected}, got {actual}") return self._take() - def _collect_until(self, *terminators: TokenType) -> str: - """Collect token values until one of the terminators is reached, tracking nesting.""" + def _collect_until(self, *terminators: TokenType) -> list[Token]: + """Collect tokens until one of the terminators is reached, tracking nesting.""" terminators_set = set(terminators) start = self._current() - parts: list[str] = [] + tokens: list[Token] = [] depth = 0 while (token := self._current()).type != TokenType.EOF: @@ -584,13 +589,13 @@ def _collect_until(self, *terminators: TokenType) -> str: if depth < 0: raise self._error("unmatched closing bracket", token=token) - parts.append(token.value) + tokens.append(token) self._pos += 1 if depth > 0: raise self._error("unclosed opening bracket", token=start) - return " ".join(parts) + return tokens def _error(self, msg: str, *, token: Token | None = None) -> Exception: """Subclasses should override to return an appropriate exception.""" diff --git a/dissect/cstruct/parser.py b/dissect/cstruct/parser.py index bd27fa5..96dc803 100644 --- a/dissect/cstruct/parser.py +++ b/dissect/cstruct/parser.py @@ -9,9 +9,10 @@ ExpressionParserError, LexerError, ParserError, + UnknownIdentifierError, ) from dissect.cstruct.expression import Expression -from dissect.cstruct.lexer import IDENTIFIER_TYPES, TokenCursor, TokenType, format_error_context, tokenize +from dissect.cstruct.lexer import IDENTIFIER_TYPES, TokenCursor, TokenType, format_error_context, join_tokens, tokenize from dissect.cstruct.types import BaseArray, BaseType, Field, Structure if TYPE_CHECKING: @@ -422,8 +423,13 @@ def _parse_enum_or_flag(self) -> type[Enum | Flag]: member_name = self._expect(TokenType.IDENTIFIER).value if self._match(TokenType.EQUALS): - expression = self._collect_until(TokenType.COMMA, TokenType.RBRACE) - value = Expression(expression).evaluate(self.cs, values) + if not (tokens := self._collect_until(TokenType.COMMA, TokenType.RBRACE)): + raise self._error("expected an expression") + + try: + value = Expression(join_tokens(tokens)).evaluate(self.cs, values) + except ExpressionParserError as e: + raise self._error(str(e), token=tokens[0]) from None else: value = next_value @@ -508,17 +514,21 @@ def _parse_array_dimensions(self, base_type: type[BaseType]) -> type[BaseType]: dimensions: list[int | Expression] = [] while self._match(TokenType.LBRACKET): - if self._at(TokenType.RBRACKET): - dimensions.append(None) - else: - expression = self._collect_until(TokenType.RBRACKET) - count = Expression(expression) + tokens = self._collect_until(TokenType.RBRACKET) + self._expect(TokenType.RBRACKET) + + count = None + if tokens: + count = Expression(join_tokens(tokens)) try: count = count.evaluate(self.cs) - except Exception: + except UnknownIdentifierError: + # The size refers to a field, so it's evaluated when reading the structure pass - dimensions.append(count) - self._expect(TokenType.RBRACKET) + except ExpressionParserError as e: + raise self._error(str(e), token=tokens[0]) from None + + dimensions.append(count) type_ = base_type for count in reversed(dimensions): diff --git a/tests/test_compiler.py b/tests/test_compiler.py index 16280f7..05c3920 100644 --- a/tests/test_compiler.py +++ b/tests/test_compiler.py @@ -423,11 +423,11 @@ def test_compiled_dynamic_array_sizes() -> None: assert [line for line in lines if line.startswith("_count = ") or "._read" in line] == [ "_count = max(0, int(_value0))", "_value3 = values['a'] = _type_call(_type3, _type4._read_array(stream, _count, context=values, endian=endian))", - "_count = max(0, (int(_value0) - _identifier(cs, values, 'HDR', 1)))", + "_count = max(0, (int(_value0) - _identifier(cs, values, 'HDR')))", "_value4 = values['b'] = _type_call(_type5, _type6._read_array(stream, _count, context=values, endian=endian))", "_count = max(0, (int(_value1) * 2))", "_value5 = values['c'] = _type_call(_type7, _type8._read_array(stream, _count, context=values, endian=endian))", - "_count = max(0, _identifier(cs, values, 'hdr.size', 1))", + "_count = max(0, _identifier(cs, values, 'hdr.size'))", "_value6 = values['d'] = _type_call(_type9, _type0._read_array(stream, _count, context=values, endian=endian))", "_count = max(0, (int(_value0) + _sizeof(cs, 'inner')))", "_value7 = values['e'] = _type_call(_type10, _type2._read_array(" diff --git a/tests/test_expression.py b/tests/test_expression.py index 46fba0c..94e6977 100644 --- a/tests/test_expression.py +++ b/tests/test_expression.py @@ -4,7 +4,7 @@ import pytest -from dissect.cstruct.exception import ExpressionParserError, LexerError, ResolveError +from dissect.cstruct.exception import ExpressionParserError, LexerError, ResolveError, UnknownIdentifierError from dissect.cstruct.expression import SOURCE_GLOBALS, Expression if TYPE_CHECKING: @@ -174,13 +174,13 @@ def test_expression(cs_with_consts: cstruct, expression: str, answer: int) -> No ), pytest.param( "UNKNOWN", - ExpressionParserError, + UnknownIdentifierError, "Unknown identifier: 'UNKNOWN'", id="unknown-identifier", ), pytest.param( "UNKNOWN.UNKNOWN", - ExpressionParserError, + UnknownIdentifierError, "Unknown identifier: 'UNKNOWN.UNKNOWN'", id="unknown-identifier", ), @@ -261,7 +261,7 @@ def test_dotted_identifier_unknown_member(cs: cstruct) -> None: }; """) - with pytest.raises(ExpressionParserError, match="Unknown identifier: 'color\\.NOTAMEMBER'"): + with pytest.raises(UnknownIdentifierError, match="Unknown identifier: 'color\\.NOTAMEMBER'"): Expression("color.NOTAMEMBER").evaluate(cs) @@ -354,9 +354,9 @@ def test_expression_source_invalid(cs: cstruct, source: str) -> None: # Numbers in other bases ("0x10 + 0b101 - 0o7", "((16 + 5) - 7)"), # Functions, and identifiers that are resolved from the context - ("sizeof(test) * X - -1", "((_sizeof(cs, 'test') * _identifier(cs, context, 'X', 1)) - (-1))"), + ("sizeof(test) * X - -1", "((_sizeof(cs, 'test') * _identifier(cs, context, 'X')) - (-1))"), ("offsetof(test, b) + a", "(_offsetof(cs, 'test', 'b') + a)"), - ("hdr.size * 4", "(_identifier(cs, context, 'hdr.size', 1) * 4)"), + ("hdr.size * 4", "(_identifier(cs, context, 'hdr.size') * 4)"), ], ) def test_expression_source_code(cs: cstruct, expression: str, source: str) -> None: @@ -380,6 +380,6 @@ def test_expression_source_code(cs: cstruct, expression: str, source: str) -> No def test_expression_source_context() -> None: - """Test that identifiers are resolved from the context with the given name, with their line number.""" - expected = "(_identifier(cs, values, 'X', 1) + _identifier(cs, values, 'b', 2))" - assert Expression("X +\nb").source(context="values") == expected + """Test that identifiers are resolved from the context with the given name.""" + expected = "(_identifier(cs, values, 'X') + _identifier(cs, values, 'b'))" + assert Expression("X + b").source(context="values") == expected diff --git a/tests/test_parser.py b/tests/test_parser.py index bbebb77..f482a6a 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -630,6 +630,78 @@ def test_preprocessor_define_from_enum_in_struct(cs: cstruct) -> None: assert cs.packet.__members__["payload"].type.__num_entries__ == 20 +@pytest.mark.parametrize( + ("cdef", "message"), + [ + ( + """ + struct test { + uint8 a; + char b[N]; + }; + """, + """\ + line 4: Identifier 'N' does not resolve to an integer + 2: struct test { + 3: uint8 a; + 4: char b[N]; + """, + ), + ( + """ + struct test { + uint8 a; + char b[4 4]; + }; + """, + """\ + line 4: Invalid expression: too many operands + 2: struct test { + 3: uint8 a; + 4: char b[4 4]; + """, + ), + ( + """ + enum test : uint8 { + A = 1, + B = A +, + C + }; + """, + """\ + line 4: Invalid expression: not enough operands + 2: enum test : uint8 { + 3: A = 1, + 4: B = A +, + """, + ), + ( + """ + enum test : uint8 { + A = 1, + B = , + C + }; + """, + """\ + line 4: expected an expression + 2: enum test : uint8 { + 3: A = 1, + 4: B = , + """, + ), + ], +) +def test_expression_invalid(cs: cstruct, cdef: str, message: str) -> None: + """Test that an array size or enum value that is missing or can't be evaluated is an error at its line.""" + cs.load("#define N LENGTH") + + with pytest.raises(ParserError) as exc_info: + cs.load(textwrap.dedent(cdef)) + assert str(exc_info.value) == textwrap.dedent(message.rstrip()) + + def test_error_context(cs: cstruct) -> None: """Test the context window around errors: 1 line before, the error line, up to 2 lines after.""" # Error on line 1: no preceding lines available; shows lines 1, 2, 3