Improve error messages for expressions

dissect/cstruct/exception.py

@@ -27,3 +27,7 @@ class ArraySizeError(Error):
 
 class ExpressionParserError(Error):
     pass
+
+
+class UnknownIdentifierError(ExpressionParserError):
+    pass

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:

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."""

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):

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("

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

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