From f428da737be2c4c22d4e2fdceb4531d33112d776 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Thu, 10 Sep 2026 14:51:28 +0900 Subject: [PATCH 1/2] hrw4u: record spans, comments and operand forms in the AST ast_visitor.py is test-only today, and it cannot reproduce the file it parsed. Four things are dropped, each of which changes either the emitted .conf or the location a diagnostic can name. A span names a file, line and column rather than a line alone. Procedure inlining splices nodes in from a second file, so a line number can no longer say where a diagnostic belongs. Node.line is replaced outright rather than kept as a property over span: nothing outside the tests reads it, and keeping it would let new code go on producing locations that cannot name their own file. Comments, because five .conf goldens carry # comments at exact positions. Parentheses, because a parenthesized factor emits cond %{GROUP}. Set and iprange operands, which until now both became a bare tuple: `in [1.2.3.4]` and `in {1.2.3.4}` produced byte-identical ASTs. They are not interchangeable -- visitor.py emits `lhs (...)` for a set and `lhs {...}` for an iprange, and only the set path is sandbox-checked. Each keeps the source text the emitter already uses, so SetValue.raw matches visitor.py's set_().getText()[1:-1] and IpRangeValue.raw its iprange().getText(). Target.from_dotted guessed a namespace/field split that no caller wanted; every table lookup matches the whole dotted name. --- tools/hrw4u/src/ast_nodes.py | 64 +++++--- tools/hrw4u/src/ast_visitor.py | 97 ++++++----- tools/hrw4u/tests/test_ast_nodes.py | 40 +++-- tools/hrw4u/tests/test_ast_visitor.py | 226 +++++++++++++++++++------- 4 files changed, 281 insertions(+), 146 deletions(-) diff --git a/tools/hrw4u/src/ast_nodes.py b/tools/hrw4u/src/ast_nodes.py index acf5bacccb3..d7bc93c50cf 100644 --- a/tools/hrw4u/src/ast_nodes.py +++ b/tools/hrw4u/src/ast_nodes.py @@ -21,20 +21,24 @@ from typing import Union __all__ = [ + "Span", "LiteralStringValue", "IdentValue", "IPValue", "ParamRef", "RegexValue", + "SetValue", + "IpRangeValue", "ValueExpr", "Node", - "Target", "Assignment", "FunctionCall", "Break", + "Comment", "Comparison", "LogicalOp", "NotOp", + "Group", "BoolLiteral", "IdentCondition", "ElifBranch", @@ -77,33 +81,37 @@ class RegexValue: raw: str -ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int, bool, tuple[IPValue, ...]] +@dataclass(frozen=True, kw_only=True) +class SetValue: + """An `in [...]` operand. Emitted as `(raw)`, so the brackets are stripped but quoting is not.""" + raw: str @dataclass(frozen=True, kw_only=True) -class Node: - line: int +class IpRangeValue: + """An `in {...}` operand. Emitted verbatim, braces included.""" + raw: str -@dataclass(frozen=True) -class Target: - namespace: str | None - field: str +ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int, bool, IpRangeValue] + + +@dataclass(frozen=True, slots=True) +class Span: + """Start position of a node. `line` is 1-based and `column` 0-based, matching ANTLR tokens.""" + file: str + line: int + column: int + - @staticmethod - def from_dotted(name: str) -> Target: - # TODO: the grammar lexes dotted paths as a single IDENT token; - # ideally the grammar would split namespace/field so this - # heuristic isn't needed. - dot = name.rfind(".") - if dot == -1: - return Target(namespace=None, field=name) - return Target(namespace=name[:dot], field=name[dot + 1:]) +@dataclass(frozen=True, kw_only=True) +class Node: + span: Span @dataclass(frozen=True, kw_only=True) class Assignment(Node): - target: Target + name: str operator: str # "=" or "+=" value: ValueExpr @@ -119,11 +127,16 @@ class Break(Node): pass +@dataclass(frozen=True, kw_only=True) +class Comment(Node): + text: str + + @dataclass(frozen=True, kw_only=True) class Comparison(Node): left: IdentValue | FunctionCall operator: str # "==", "!=", ">", "<", "~", "!~", "in", "!in" - right: ValueExpr | RegexValue | tuple[ValueExpr, ...] + right: ValueExpr | RegexValue | SetValue modifiers: tuple[str, ...] @@ -139,6 +152,11 @@ class NotOp(Node): operand: ConditionExpr +@dataclass(frozen=True, kw_only=True) +class Group(Node): + inner: ConditionExpr + + @dataclass(frozen=True, kw_only=True) class BoolLiteral(Node): value: bool @@ -185,7 +203,7 @@ class VarDecl(Node): @dataclass(frozen=True, kw_only=True) class VarSection(Node): scope: str - declarations: tuple[VarDecl, ...] + items: tuple[VarDecl | Comment, ...] @dataclass(frozen=True, kw_only=True) @@ -206,6 +224,6 @@ class HRW4UAST: # Type aliases: must follow all class definitions (evaluated at runtime). -ConditionExpr = Union[Comparison, LogicalOp, NotOp, BoolLiteral, IdentCondition, FunctionCall] -BodyNode = Union[Assignment, FunctionCall, IfBlock, Break] -TopLevelNode = Union[UseDirective, VarSection, ProcedureDecl, Section] +ConditionExpr = Union[Comparison, LogicalOp, NotOp, Group, BoolLiteral, IdentCondition, FunctionCall] +BodyNode = Union[Assignment, FunctionCall, IfBlock, Break, Comment] +TopLevelNode = Union[UseDirective, VarSection, ProcedureDecl, Section, Comment] diff --git a/tools/hrw4u/src/ast_visitor.py b/tools/hrw4u/src/ast_visitor.py index 4a66ec0a710..c61d571d31c 100644 --- a/tools/hrw4u/src/ast_visitor.py +++ b/tools/hrw4u/src/ast_visitor.py @@ -19,10 +19,26 @@ from hrw4u.hrw4uVisitor import hrw4uVisitor from hrw4u.ast_nodes import * +from hrw4u.common import SystemDefaults class ASTVisitor(hrw4uVisitor): - """ANTLR visitor that walks an HRW4U parse tree and produces an AST for HRW4U.""" + """ + ANTLR visitor that walks an HRW4U parse tree and produces an AST for HRW4U. + + Requires a tree that parsed without errors. ANTLR's error recovery can leave a mandatory + child unset, which surfaces here as an AttributeError rather than a diagnostic. + """ + + def __init__(self, filename: str = SystemDefaults.DEFAULT_FILENAME) -> None: + super().__init__() + self.filename = filename + + def _span(self, ctx) -> Span: + return Span(file=self.filename, line=ctx.start.line, column=ctx.start.column) + + def _visit_comment(self, ctx) -> Comment: + return Comment(text=ctx.COMMENT().getText(), span=self._span(ctx)) # Only visitProgram is overridden from the ANTLR visitor interface; # all other traversal uses private _visit_* helpers so that each @@ -39,13 +55,13 @@ def visitProgram(self, ctx) -> HRW4UAST: elif item.section() is not None: items.append(self._visit_section(item.section())) elif item.commentLine() is not None: - pass + items.append(self._visit_comment(item.commentLine())) else: raise ValueError(f"Unhandled programItem alternative at line {item.start.line}") return HRW4UAST(body=tuple(items)) def _visit_use_directive(self, ctx) -> UseDirective: - return UseDirective(spec=ctx.QUALIFIED_IDENT().getText(), line=ctx.start.line) + return UseDirective(spec=ctx.QUALIFIED_IDENT().getText(), span=self._span(ctx)) def _visit_procedure_decl(self, ctx) -> ProcedureDecl: name = ctx.QUALIFIED_IDENT().getText() @@ -53,12 +69,12 @@ def _visit_procedure_decl(self, ctx) -> ProcedureDecl: if ctx.paramList(): params = tuple(self._visit_proc_param(p) for p in ctx.paramList().param()) body = tuple(self._visit_body(ctx.block().blockItem())) - return ProcedureDecl(name=name, params=params, body=body, line=ctx.start.line) + return ProcedureDecl(name=name, params=params, body=body, span=self._span(ctx)) def _visit_proc_param(self, ctx) -> ProcParam: name = ctx.IDENT().getText() default = self._extract_value(ctx.value()) if ctx.value() else None - return ProcParam(name=name, default=default, line=ctx.start.line) + return ProcParam(name=name, default=default, span=self._span(ctx)) def _visit_section(self, ctx) -> VarSection | Section: if ctx.varSection() is not None: @@ -67,22 +83,22 @@ def _visit_section(self, ctx) -> VarSection | Section: return self._visit_var_section(ctx.sessionVarSection(), "session") name = ctx.name.text body = self._visit_body(ctx.sectionBody()) - return Section(type=name, body=tuple(body), line=ctx.start.line) + return Section(type=name, body=tuple(body), span=self._span(ctx)) def _visit_var_section(self, ctx, scope) -> VarSection: - decls = [] + items = [] for var_item in ctx.variables().variablesItem(): if var_item.variableDecl() is not None: - decls.append(self._visit_var_decl(var_item.variableDecl())) + items.append(self._visit_var_decl(var_item.variableDecl())) elif var_item.commentLine() is not None: - pass + items.append(self._visit_comment(var_item.commentLine())) else: raise ValueError(f"Unhandled variablesItem alternative at line {var_item.start.line}") - return VarSection(scope=scope, declarations=tuple(decls), line=ctx.start.line) + return VarSection(scope=scope, items=tuple(items), span=self._span(ctx)) def _visit_var_decl(self, ctx) -> VarDecl: return VarDecl( - name=ctx.name.text, type_name=ctx.typeName.text, slot=int(ctx.slot.text) if ctx.slot else None, line=ctx.start.line) + name=ctx.name.text, type_name=ctx.typeName.text, slot=int(ctx.slot.text) if ctx.slot else None, span=self._span(ctx)) def _visit_body(self, items) -> list[BodyNode]: """Shared helper for sectionBody and blockItem lists.""" @@ -93,35 +109,30 @@ def _visit_body(self, items) -> list[BodyNode]: elif item.conditional() is not None: result.append(self._visit_conditional(item.conditional())) elif item.commentLine() is not None: - pass + result.append(self._visit_comment(item.commentLine())) else: raise ValueError(f"Unhandled body item alternative at line {item.start.line}") return result def _visit_statement(self, ctx) -> BodyNode: - line = ctx.start.line if ctx.BREAK(): - return Break(line=line) + return Break(span=self._span(ctx)) if ctx.functionCall(): return self._visit_function_call(ctx.functionCall()) if ctx.EQUAL(): - target = Target.from_dotted(ctx.lhs.text) - value = self._extract_value(ctx.value()) - return Assignment(target=target, operator="=", value=value, line=line) + return Assignment(name=ctx.lhs.text, operator="=", value=self._extract_value(ctx.value()), span=self._span(ctx)) if ctx.PLUSEQUAL(): - target = Target.from_dotted(ctx.lhs.text) - value = self._extract_value(ctx.value()) - return Assignment(target=target, operator="+=", value=value, line=line) + return Assignment(name=ctx.lhs.text, operator="+=", value=self._extract_value(ctx.value()), span=self._span(ctx)) if ctx.op: - return FunctionCall(name=ctx.op.text, args=(), line=line) - raise ValueError(f"Unhandled statement alternative at line {line}") + return FunctionCall(name=ctx.op.text, args=(), span=self._span(ctx)) + raise ValueError(f"Unhandled statement alternative at line {ctx.start.line}") def _visit_function_call(self, ctx) -> FunctionCall: name = ctx.funcName.text args = () if ctx.argumentList(): args = tuple(self._extract_value(v) for v in ctx.argumentList().value()) - return FunctionCall(name=name, args=args, line=ctx.start.line) + return FunctionCall(name=name, args=args, span=self._span(ctx)) def _extract_value(self, ctx) -> ValueExpr: if ctx.number is not None: @@ -137,7 +148,7 @@ def _extract_value(self, ctx) -> ValueExpr: if ctx.ip(): return IPValue(raw=ctx.ip().getText()) if ctx.iprange(): - return tuple(IPValue(raw=ip.getText()) for ip in ctx.iprange().ip()) + return IpRangeValue(raw=ctx.iprange().getText()) if ctx.paramRef(): return ParamRef(raw=ctx.paramRef().IDENT().getText()) raise ValueError(f"Unhandled value alternative at line {ctx.start.line}") @@ -145,23 +156,20 @@ def _extract_value(self, ctx) -> ValueExpr: def _visit_conditional(self, ctx) -> IfBlock: if_stmt = ctx.ifStatement() condition = self._visit_condition(if_stmt.condition()) - block = if_stmt.block() - body = tuple(self._visit_body(block.blockItem())) if block else () + body = tuple(self._visit_body(if_stmt.block().blockItem())) elif_branches = [] for elif_ctx in ctx.elifClause(): elif_cond = self._visit_condition(elif_ctx.condition()) - elif_block = elif_ctx.block() - elif_body = tuple(self._visit_body(elif_block.blockItem())) if elif_block else () - elif_branches.append(ElifBranch(condition=elif_cond, body=elif_body, line=elif_ctx.start.line)) + elif_body = tuple(self._visit_body(elif_ctx.block().blockItem())) + elif_branches.append(ElifBranch(condition=elif_cond, body=elif_body, span=self._span(elif_ctx))) else_body = () if ctx.elseClause(): - else_block = ctx.elseClause().block() - if else_block: - else_body = tuple(self._visit_body(else_block.blockItem())) + else_body = tuple(self._visit_body(ctx.elseClause().block().blockItem())) - return IfBlock(condition=condition, body=body, elif_branches=tuple(elif_branches), else_body=else_body, line=ctx.start.line) + return IfBlock( + condition=condition, body=body, elif_branches=tuple(elif_branches), else_body=else_body, span=self._span(ctx)) def _visit_condition(self, ctx) -> ConditionExpr: return self._visit_expression(ctx.expression()) @@ -170,35 +178,34 @@ def _visit_expression(self, ctx) -> ConditionExpr: if ctx.OR(): left = self._visit_expression(ctx.expression()) right = self._visit_term(ctx.term()) - return LogicalOp(operator="||", left=left, right=right, line=ctx.start.line) + return LogicalOp(operator="||", left=left, right=right, span=self._span(ctx)) return self._visit_term(ctx.term()) def _visit_term(self, ctx) -> ConditionExpr: if ctx.AND(): left = self._visit_term(ctx.term()) right = self._visit_factor(ctx.factor()) - return LogicalOp(operator="&&", left=left, right=right, line=ctx.start.line) + return LogicalOp(operator="&&", left=left, right=right, span=self._span(ctx)) return self._visit_factor(ctx.factor()) def _visit_factor(self, ctx) -> ConditionExpr: if ctx.getChildCount() == 2 and ctx.getChild(0).getText() == "!": - return NotOp(operand=self._visit_factor(ctx.factor()), line=ctx.start.line) + return NotOp(operand=self._visit_factor(ctx.factor()), span=self._span(ctx)) if ctx.LPAREN(): - return self._visit_expression(ctx.expression()) + return Group(inner=self._visit_expression(ctx.expression()), span=self._span(ctx)) if ctx.functionCall(): return self._visit_function_call(ctx.functionCall()) if ctx.comparison(): return self._visit_comparison(ctx.comparison()) if ctx.ident is not None: - return IdentCondition(name=ctx.ident.text, line=ctx.start.line) + return IdentCondition(name=ctx.ident.text, span=self._span(ctx)) if ctx.TRUE(): - return BoolLiteral(value=True, line=ctx.start.line) + return BoolLiteral(value=True, span=self._span(ctx)) if ctx.FALSE(): - return BoolLiteral(value=False, line=ctx.start.line) + return BoolLiteral(value=False, span=self._span(ctx)) raise ValueError(f"Unhandled factor alternative at line {ctx.start.line}") def _visit_comparison(self, ctx) -> Comparison: - line = ctx.start.line comp = ctx.comparable() if comp.ident is not None: left = IdentValue(raw=comp.ident.text) @@ -209,7 +216,7 @@ def _visit_comparison(self, ctx) -> Comparison: right = self._extract_comparison_rhs(ctx, operator) modifiers = self._extract_modifiers(ctx) - return Comparison(left=left, operator=operator, right=right, modifiers=modifiers, line=line) + return Comparison(left=left, operator=operator, right=right, modifiers=modifiers, span=self._span(ctx)) def _detect_comparison_operator(self, ctx) -> str: if ctx.EQUALS(): @@ -231,14 +238,14 @@ def _detect_comparison_operator(self, ctx) -> str: return "in" raise ValueError(f"Unhandled comparison operator at line {ctx.start.line}") - def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | tuple[ValueExpr, ...]: + def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | SetValue: if operator in ("~", "!~"): return RegexValue(raw=ctx.regex().getText()[1:-1]) if operator in ("in", "!in"): if ctx.set_(): - return tuple(self._extract_value(v) for v in ctx.set_().value()) + return SetValue(raw=ctx.set_().getText()[1:-1]) if ctx.iprange(): - return tuple(IPValue(raw=ip.getText()) for ip in ctx.iprange().ip()) + return IpRangeValue(raw=ctx.iprange().getText()) if ctx.value(): return self._extract_value(ctx.value()) raise ValueError(f"Unhandled comparison RHS at line {ctx.start.line}") diff --git a/tools/hrw4u/tests/test_ast_nodes.py b/tools/hrw4u/tests/test_ast_nodes.py index d76d4a89b26..245e1f7b0a3 100644 --- a/tools/hrw4u/tests/test_ast_nodes.py +++ b/tools/hrw4u/tests/test_ast_nodes.py @@ -15,27 +15,31 @@ # See the License for the specific language governing permissions and # limitations under the License. -from hrw4u.ast_nodes import Target +import pytest +from hrw4u.ast_nodes import Span -class TestTarget: - def test_dotted_path(self): - t = Target.from_dotted("inbound.req.X-Foo") - assert t.namespace == "inbound.req" - assert t.field == "X-Foo" +class TestSpan: - def test_two_segments(self): - t = Target.from_dotted("inbound.ip") - assert t.namespace == "inbound" - assert t.field == "ip" + def test_equality_is_by_value(self): + assert Span(file="a", line=1, column=0) == Span(file="a", line=1, column=0) + assert Span(file="a", line=1, column=0) != Span(file="b", line=1, column=0) - def test_no_dots(self): - t = Target.from_dotted("bool_0") - assert t.namespace is None - assert t.field == "bool_0" + def test_is_hashable_so_it_can_key_a_span_index(self): + first = Span(file="a", line=1, column=0) + assert {first: "node"}[Span(file="a", line=1, column=0)] == "node" - def test_deep_namespace(self): - t = Target.from_dotted("http.cntl.TXN_DEBUG") - assert t.namespace == "http.cntl" - assert t.field == "TXN_DEBUG" + def test_is_immutable(self): + span = Span(file="a", line=1, column=0) + with pytest.raises(Exception): + span.line = 2 + + def test_rejects_unknown_attributes(self): + # slots=True: a typo'd field must fail loudly rather than sit unread on the instance. + with pytest.raises(AttributeError): + Span(file="a", line=1, column=0).lineno = 2 + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/tools/hrw4u/tests/test_ast_visitor.py b/tools/hrw4u/tests/test_ast_visitor.py index ec919d1f060..506b8255461 100644 --- a/tools/hrw4u/tests/test_ast_visitor.py +++ b/tools/hrw4u/tests/test_ast_visitor.py @@ -15,14 +15,25 @@ # See the License for the specific language governing permissions and # limitations under the License. +from antlr4 import InputStream, CommonTokenStream + from hrw4u.ast_nodes import * -from utils import parse_input_text from hrw4u.ast_visitor import ASTVisitor +from hrw4u.common import SystemDefaults +from hrw4u.errors import ThrowingErrorListener +from hrw4u.hrw4uLexer import hrw4uLexer +from hrw4u.hrw4uParser import hrw4uParser -def _build(source: str) -> HRW4UAST: - _, tree = parse_input_text(source) - return ASTVisitor().visit(tree) +def _build(source: str, filename: str = SystemDefaults.DEFAULT_FILENAME) -> HRW4UAST: + """Parse strictly, so a test never asserts against an error-recovered tree.""" + lexer = hrw4uLexer(InputStream(source)) + lexer.removeErrorListeners() + lexer.addErrorListener(ThrowingErrorListener(filename)) + parser = hrw4uParser(CommonTokenStream(lexer)) + parser.removeErrorListeners() + parser.addErrorListener(ThrowingErrorListener(filename)) + return ASTVisitor(filename=filename).visit(parser.program()) class TestAssignments: @@ -31,7 +42,7 @@ def test_simple_assignment(self): ast = _build('REMAP {\n inbound.req.X-Foo = "test";\n}') a = ast.body[0].body[0] assert isinstance(a, Assignment) - assert a.target == Target.from_dotted("inbound.req.X-Foo") + assert a.name == "inbound.req.X-Foo" assert a.operator == "=" assert a.value == LiteralStringValue(raw="test") @@ -46,6 +57,11 @@ def test_int_value(self): a = ast.body[0].body[0] assert a.value == 1 + def test_undotted_lvalue_keeps_its_name(self): + ast = _build('VARS {\n flag: bool;\n}\nREMAP {\n flag = true;\n}') + a = ast.body[1].body[0] + assert a.name == "flag" + def test_plus_equals(self): ast = _build('REMAP {\n inbound.req.X-Foo += "extra";\n}') a = ast.body[0].body[0] @@ -95,15 +111,15 @@ def test_break(self): class TestSections: - def test_comments_in_section_body_skipped(self): + def test_comments_in_section_body_are_kept_in_order(self): src = 'REMAP {\n # a comment\n set-debug();\n # another comment\n}' ast = _build(src) - assert len(ast.body[0].body) == 1 + assert len(ast.body[0].body) == 3 - def test_comments_in_block_skipped(self): + def test_comments_in_block_are_kept_in_order(self): src = 'REMAP {\n if true {\n # comment\n set-debug();\n }\n}' ast = _build(src) - assert len(ast.body[0].body[0].body) == 1 + assert len(ast.body[0].body[0].body) == 2 def test_section_type(self): ast = _build('REMAP {\n set-debug();\n}') @@ -138,12 +154,12 @@ def test_item_ordering(self): class TestVarSections: - def test_comments_in_var_section_skipped(self): + def test_comments_in_var_section_are_kept_in_order(self): src = 'VARS {\n # comment\n x: bool;\n # another\n y: int;\n}\nREMAP {\n set-debug();\n}' ast = _build(src) vs = ast.body[0] assert isinstance(vs, VarSection) - assert len(vs.declarations) == 2 + assert len(vs.items) == 4 def test_txn_scope(self): src = 'VARS {\n flag: bool;\n}\nREMAP {\n set-debug();\n}' @@ -151,10 +167,10 @@ def test_txn_scope(self): vs = ast.body[0] assert isinstance(vs, VarSection) assert vs.scope == "txn" - assert len(vs.declarations) == 1 - assert vs.declarations[0].name == "flag" - assert vs.declarations[0].type_name == "bool" - assert vs.declarations[0].slot is None + assert len(vs.items) == 1 + assert vs.items[0].name == "flag" + assert vs.items[0].type_name == "bool" + assert vs.items[0].slot is None def test_session_scope(self): src = 'SESSION_VARS {\n counter: int;\n}\nREMAP {\n set-debug();\n}' @@ -162,24 +178,24 @@ def test_session_scope(self): vs = ast.body[0] assert isinstance(vs, VarSection) assert vs.scope == "session" - assert vs.declarations[0].name == "counter" + assert vs.items[0].name == "counter" def test_slot(self): src = 'VARS {\n x: int @3;\n}\nREMAP {\n set-debug();\n}' ast = _build(src) vs = ast.body[0] assert isinstance(vs, VarSection) - assert vs.declarations[0].slot == 3 + assert vs.items[0].slot == 3 def test_multiple_declarations(self): src = 'VARS {\n a: bool;\n b: int;\n c: string;\n}\nREMAP {\n set-debug();\n}' ast = _build(src) vs = ast.body[0] assert isinstance(vs, VarSection) - assert len(vs.declarations) == 3 - assert vs.declarations[0].name == "a" - assert vs.declarations[1].name == "b" - assert vs.declarations[2].name == "c" + assert len(vs.items) == 3 + assert vs.items[0].name == "a" + assert vs.items[1].name == "b" + assert vs.items[2].name == "c" class TestProcedures: @@ -237,7 +253,7 @@ def test_in_set(self): cond = self._first_condition('REMAP {\n if inbound.url.path in ["a", "b"] {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (LiteralStringValue(raw="a"), LiteralStringValue(raw="b")) + assert cond.right == SetValue(raw='"a","b"') def test_not_in_set(self): cond = self._first_condition('REMAP {\n if inbound.url.path !in ["a"] {\n set-debug();\n }\n}') @@ -248,7 +264,15 @@ def test_in_iprange(self): cond = self._first_condition('REMAP {\n if inbound.ip in {10.0.0.0/8} {\n set-debug();\n }\n}') assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (IPValue(raw="10.0.0.0/8"),) + assert cond.right == IpRangeValue(raw='{10.0.0.0/8}') + + def test_set_and_iprange_of_the_same_ip_stay_distinct(self): + # visitor.py emits `lhs (...)` for a set but `lhs {...}` for an iprange, and only the set + # path is sandbox-checked, so the AST has to tell one from the other. + as_set = self._first_condition('REMAP {\n if inbound.ip in [1.2.3.4] {\n set-debug();\n }\n}') + as_range = self._first_condition('REMAP {\n if inbound.ip in {1.2.3.4} {\n set-debug();\n }\n}') + assert as_set.right == SetValue(raw='1.2.3.4') + assert as_range.right == IpRangeValue(raw='{1.2.3.4}') def test_modifiers(self): cond = self._first_condition('REMAP {\n if inbound.req.X-Foo == "bar" with NOCASE {\n set-debug();\n }\n}') @@ -328,9 +352,10 @@ def test_neq_comparison(self): def test_parenthesized_condition(self): cond = self._first_condition('REMAP {\n if (inbound.req.X-Foo == "bar") {\n set-debug();\n }\n}') - assert isinstance(cond, Comparison) - assert cond.operator == "==" - assert cond.right == LiteralStringValue(raw="bar") + assert isinstance(cond, Group) + assert isinstance(cond.inner, Comparison) + assert cond.inner.operator == "==" + assert cond.inner.right == LiteralStringValue(raw="bar") def test_and_binds_tighter_than_or(self): # a || b && c should parse as a || (b && c) @@ -370,9 +395,10 @@ def test_not_comparison_with_or(self): assert isinstance(cond, LogicalOp) assert cond.operator == "||" assert isinstance(cond.left, NotOp) - assert isinstance(cond.left.operand, Comparison) - assert cond.left.operand.left == IdentValue(raw="inbound.req.X-A") - assert cond.left.operand.right == LiteralStringValue(raw="x") + assert isinstance(cond.left.operand, Group) + assert isinstance(cond.left.operand.inner, Comparison) + assert cond.left.operand.inner.left == IdentValue(raw="inbound.req.X-A") + assert cond.left.operand.inner.right == LiteralStringValue(raw="x") assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-B") @@ -397,10 +423,11 @@ def test_parens_override_precedence(self): ' set-debug();\n }\n}') assert isinstance(cond, LogicalOp) assert cond.operator == "&&" - assert isinstance(cond.left, LogicalOp) - assert cond.left.operator == "||" - assert cond.left.left.left == IdentValue(raw="inbound.req.X-A") - assert cond.left.right.left == IdentValue(raw="inbound.req.X-B") + assert isinstance(cond.left, Group) + assert isinstance(cond.left.inner, LogicalOp) + assert cond.left.inner.operator == "||" + assert cond.left.inner.left.left == IdentValue(raw="inbound.req.X-A") + assert cond.left.inner.right.left == IdentValue(raw="inbound.req.X-B") assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-C") @@ -413,8 +440,9 @@ def test_nested_parens_with_not(self): assert isinstance(cond, LogicalOp) assert cond.operator == "&&" assert isinstance(cond.left, NotOp) - assert isinstance(cond.left.operand, LogicalOp) - assert cond.left.operand.operator == "||" + assert isinstance(cond.left.operand, Group) + assert isinstance(cond.left.operand.inner, LogicalOp) + assert cond.left.operand.inner.operator == "||" assert isinstance(cond.right, Comparison) assert cond.right.left == IdentValue(raw="inbound.req.X-C") @@ -482,7 +510,7 @@ def test_mixed_body(self): assert isinstance(body[2], Assignment) -class TestLineNumbers: +class TestSpanLines: SRC = ( "use test::helper\n" # line 1 "VARS {\n" # line 2 @@ -523,97 +551,97 @@ def setup_method(self): def test_use_directive(self): u = self.ast.body[0] assert isinstance(u, UseDirective) - assert u.line == 1 + assert u.span.line == 1 def test_var_section(self): vs = self.ast.body[1] assert isinstance(vs, VarSection) - assert vs.line == 2 + assert vs.span.line == 2 def test_var_decl(self): - vd = self.ast.body[1].declarations[0] + vd = self.ast.body[1].items[0] assert isinstance(vd, VarDecl) - assert vd.line == 3 + assert vd.span.line == 3 def test_procedure_decl(self): pd = self.ast.body[2] assert isinstance(pd, ProcedureDecl) - assert pd.line == 5 + assert pd.span.line == 5 def test_proc_param(self): pp = self.ast.body[2].params[0] assert isinstance(pp, ProcParam) - assert pp.line == 5 + assert pp.span.line == 5 def test_procedure_body_assignment(self): a = self.ast.body[2].body[0] assert isinstance(a, Assignment) - assert a.line == 6 + assert a.span.line == 6 def test_section(self): s = self.ast.body[3] assert isinstance(s, Section) - assert s.line == 8 + assert s.span.line == 8 def test_assignment(self): a = self.ast.body[3].body[0] assert isinstance(a, Assignment) - assert a.line == 9 + assert a.span.line == 9 def test_function_call(self): fc = self.ast.body[3].body[1] assert isinstance(fc, FunctionCall) - assert fc.line == 10 + assert fc.span.line == 10 def test_standalone_operator(self): fc = self.ast.body[3].body[2] assert isinstance(fc, FunctionCall) - assert fc.line == 11 + assert fc.span.line == 11 def test_if_block(self): ib = self.ast.body[3].body[3] assert isinstance(ib, IfBlock) - assert ib.line == 12 + assert ib.span.line == 12 def test_comparison_in_condition(self): cond = self.ast.body[3].body[3].condition assert isinstance(cond, Comparison) - assert cond.line == 12 + assert cond.span.line == 12 def test_break(self): brk = self.ast.body[3].body[3].body[0] assert isinstance(brk, Break) - assert brk.line == 13 + assert brk.span.line == 13 def test_elif_branch(self): eb = self.ast.body[3].body[3].elif_branches[0] assert isinstance(eb, ElifBranch) - assert eb.line == 14 + assert eb.span.line == 14 def test_elif_condition(self): cond = self.ast.body[3].body[3].elif_branches[0].condition assert isinstance(cond, Comparison) - assert cond.line == 14 + assert cond.span.line == 14 def test_logical_op(self): cond = self.ast.body[3].body[4].condition assert isinstance(cond, LogicalOp) - assert cond.line == 19 + assert cond.span.line == 19 def test_not_op(self): cond = self.ast.body[3].body[5].condition assert isinstance(cond, NotOp) - assert cond.line == 22 + assert cond.span.line == 22 def test_bool_literal(self): cond = self.ast.body[3].body[6].condition assert isinstance(cond, BoolLiteral) - assert cond.line == 25 + assert cond.span.line == 25 def test_ident_condition(self): cond = self.ast.body[3].body[7].condition assert isinstance(cond, IdentCondition) - assert cond.line == 28 + assert cond.span.line == 28 class TestRealConfigs: @@ -700,7 +728,7 @@ def test_ip_range_condition(self): cond = ast.body[0].body[0].condition assert isinstance(cond, Comparison) assert cond.operator == "in" - assert len(cond.right) == 2 + assert cond.right == IpRangeValue(raw='{192.168.0.0/16,10.0.0.0/8}') def test_set_membership_with_modifier(self): """From tests/data/conds/in-sets.input.txt.""" @@ -713,7 +741,7 @@ def test_set_membership_with_modifier(self): cond = ast.body[0].body[0].condition assert isinstance(cond, Comparison) assert cond.operator == "in" - assert cond.right == (LiteralStringValue(raw="php"), LiteralStringValue(raw="php3"), LiteralStringValue(raw="php4")) + assert cond.right == SetValue(raw='"php","php3","php4"') assert cond.modifiers == ("EXT",) def test_debug_pattern_for_lint_rules(self): @@ -732,9 +760,87 @@ def test_debug_pattern_for_lint_rules(self): # TXN_DEBUG assignment with True assert isinstance(body[1], Assignment) - assert body[1].target == Target.from_dotted("http.cntl.TXN_DEBUG") + assert body[1].name == "http.cntl.TXN_DEBUG" assert body[1].value is True # Regular assignment (not flagged) assert isinstance(body[2], Assignment) - assert body[2].target.namespace == "inbound.req" + assert body[2].name == "inbound.req.X-Foo" + + +class TestSpans: + + def test_span_carries_file_line_and_column(self): + ast = _build('REMAP {\n inbound.req.X-Foo = "test";\n}', filename="demo.hrw4u") + stmt = ast.body[0].body[0] + assert stmt.span == Span(file="demo.hrw4u", line=2, column=4) + + def test_default_filename(self): + ast = _build('REMAP {\n set-debug();\n}') + assert ast.body[0].span.file == SystemDefaults.DEFAULT_FILENAME + + def test_nested_nodes_carry_their_own_column(self): + src = 'REMAP {\n if !(inbound.status > 399) {\n set-debug();\n }\n}' + cond = _build(src, filename="d").body[0].body[0].condition + assert cond.span == Span(file="d", line=2, column=7) + assert cond.operand.span == Span(file="d", line=2, column=8) + assert cond.operand.inner.span == Span(file="d", line=2, column=9) + + def test_elif_and_decl_columns(self): + src = 'VARS {\n flag: bool;\n}\nREMAP {\n if flag {\n break;\n } elif true {\n break;\n }\n}' + ast = _build(src, filename="d") + assert ast.body[0].items[0].span == Span(file="d", line=2, column=6) + assert ast.body[1].body[0].elif_branches[0].span == Span(file="d", line=7, column=6) + + +class TestComments: + + def test_top_level_comment_preserved(self): + ast = _build('# hello\nREMAP {\n set-debug();\n}') + assert ast.body[0] == Comment(text="# hello", span=Span(file=SystemDefaults.DEFAULT_FILENAME, line=1, column=0)) + + def test_indented_comment_keeps_its_column(self): + ast = _build('REMAP {\n # deep\n set-debug();\n}', filename="d") + assert ast.body[0].body[0] == Comment(text="# deep", span=Span(file="d", line=2, column=8)) + + def test_comment_in_section_body_keeps_position(self): + ast = _build('REMAP {\n # first\n set-debug();\n}') + body = ast.body[0].body + assert isinstance(body[0], Comment) + assert body[0].text == "# first" + assert isinstance(body[1], FunctionCall) + + def test_comment_in_block(self): + ast = _build('REMAP {\n if inbound.status > 399 {\n # why\n set-debug();\n }\n}') + assert isinstance(ast.body[0].body[0].body[0], Comment) + + def test_comment_in_else_block(self): + src = 'REMAP {\n if true {\n break;\n } else {\n # otherwise\n set-debug();\n }\n}' + assert isinstance(_build(src).body[0].body[0].else_body[0], Comment) + + def test_comment_in_procedure_body(self): + src = 'procedure local::p() {\n # note\n set-debug();\n}\nREMAP {\n set-debug();\n}' + assert isinstance(_build(src).body[0].body[0], Comment) + + def test_comment_in_vars_section(self): + ast = _build('VARS {\n # a counter\n hits: int8;\n}') + items = ast.body[0].items + assert isinstance(items[0], Comment) + assert isinstance(items[1], VarDecl) + + def test_comment_in_session_vars_section(self): + ast = _build('SESSION_VARS {\n # a counter\n hits: int8;\n}\nREMAP {\n set-debug();\n}') + assert isinstance(ast.body[0].items[0], Comment) + + +class TestGrouping: + + def test_negated_group(self): + ast = _build('REMAP {\n if !(inbound.status > 399) {\n set-debug();\n }\n}') + cond = ast.body[0].body[0].condition + assert isinstance(cond, NotOp) + assert isinstance(cond.operand, Group) + + def test_no_parens_no_group(self): + ast = _build('REMAP {\n if inbound.status > 399 {\n set-debug();\n }\n}') + assert isinstance(ast.body[0].body[0].condition, Comparison) From 04133e312884032376e56d402ca602ebb784fe9e Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Thu, 10 Sep 2026 16:53:25 +0900 Subject: [PATCH 2/2] hrw4u: report AST visitor errors with file and column The eight "Unhandled ... at line N" raises predate the span and cannot name the file, which is the case procedure inlining creates. They go through one _unhandled() helper now. test_ast_nodes.py asserted AttributeError for assignment to an unknown Span field, which proved nothing: FrozenInstanceError subclasses AttributeError, so frozen intercepted the write and slots was never exercised. Check the layout instead. ValueExpr admits IpRangeValue but not SetValue because the grammar's value rule admits iprange and not set_; say so where the alias is defined. --- tools/hrw4u/src/ast_nodes.py | 2 ++ tools/hrw4u/src/ast_visitor.py | 20 ++++++++++++-------- tools/hrw4u/tests/test_ast_nodes.py | 15 ++++++--------- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/tools/hrw4u/src/ast_nodes.py b/tools/hrw4u/src/ast_nodes.py index d7bc93c50cf..caca5babb7d 100644 --- a/tools/hrw4u/src/ast_nodes.py +++ b/tools/hrw4u/src/ast_nodes.py @@ -93,6 +93,8 @@ class IpRangeValue: raw: str +# IpRangeValue is a ValueExpr but SetValue is not, because the grammar's `value` rule admits +# `iprange` and not `set_`: an iprange is legal anywhere a value is, a set only after `in`. ValueExpr = Union[LiteralStringValue, IdentValue, IPValue, ParamRef, int, bool, IpRangeValue] diff --git a/tools/hrw4u/src/ast_visitor.py b/tools/hrw4u/src/ast_visitor.py index c61d571d31c..58f3e23f791 100644 --- a/tools/hrw4u/src/ast_visitor.py +++ b/tools/hrw4u/src/ast_visitor.py @@ -37,6 +37,10 @@ def __init__(self, filename: str = SystemDefaults.DEFAULT_FILENAME) -> None: def _span(self, ctx) -> Span: return Span(file=self.filename, line=ctx.start.line, column=ctx.start.column) + def _unhandled(self, what: str, ctx) -> ValueError: + span = self._span(ctx) + return ValueError(f"Unhandled {what} at {span.file}:{span.line}:{span.column}") + def _visit_comment(self, ctx) -> Comment: return Comment(text=ctx.COMMENT().getText(), span=self._span(ctx)) @@ -57,7 +61,7 @@ def visitProgram(self, ctx) -> HRW4UAST: elif item.commentLine() is not None: items.append(self._visit_comment(item.commentLine())) else: - raise ValueError(f"Unhandled programItem alternative at line {item.start.line}") + raise self._unhandled("programItem alternative", item) return HRW4UAST(body=tuple(items)) def _visit_use_directive(self, ctx) -> UseDirective: @@ -93,7 +97,7 @@ def _visit_var_section(self, ctx, scope) -> VarSection: elif var_item.commentLine() is not None: items.append(self._visit_comment(var_item.commentLine())) else: - raise ValueError(f"Unhandled variablesItem alternative at line {var_item.start.line}") + raise self._unhandled("variablesItem alternative", var_item) return VarSection(scope=scope, items=tuple(items), span=self._span(ctx)) def _visit_var_decl(self, ctx) -> VarDecl: @@ -111,7 +115,7 @@ def _visit_body(self, items) -> list[BodyNode]: elif item.commentLine() is not None: result.append(self._visit_comment(item.commentLine())) else: - raise ValueError(f"Unhandled body item alternative at line {item.start.line}") + raise self._unhandled("body item alternative", item) return result def _visit_statement(self, ctx) -> BodyNode: @@ -125,7 +129,7 @@ def _visit_statement(self, ctx) -> BodyNode: return Assignment(name=ctx.lhs.text, operator="+=", value=self._extract_value(ctx.value()), span=self._span(ctx)) if ctx.op: return FunctionCall(name=ctx.op.text, args=(), span=self._span(ctx)) - raise ValueError(f"Unhandled statement alternative at line {ctx.start.line}") + raise self._unhandled("statement alternative", ctx) def _visit_function_call(self, ctx) -> FunctionCall: name = ctx.funcName.text @@ -151,7 +155,7 @@ def _extract_value(self, ctx) -> ValueExpr: return IpRangeValue(raw=ctx.iprange().getText()) if ctx.paramRef(): return ParamRef(raw=ctx.paramRef().IDENT().getText()) - raise ValueError(f"Unhandled value alternative at line {ctx.start.line}") + raise self._unhandled("value alternative", ctx) def _visit_conditional(self, ctx) -> IfBlock: if_stmt = ctx.ifStatement() @@ -203,7 +207,7 @@ def _visit_factor(self, ctx) -> ConditionExpr: return BoolLiteral(value=True, span=self._span(ctx)) if ctx.FALSE(): return BoolLiteral(value=False, span=self._span(ctx)) - raise ValueError(f"Unhandled factor alternative at line {ctx.start.line}") + raise self._unhandled("factor alternative", ctx) def _visit_comparison(self, ctx) -> Comparison: comp = ctx.comparable() @@ -236,7 +240,7 @@ def _detect_comparison_operator(self, ctx) -> str: if hasattr(child, "getText") and child.getText() == "!": return "!in" return "in" - raise ValueError(f"Unhandled comparison operator at line {ctx.start.line}") + raise self._unhandled("comparison operator", ctx) def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | SetValue: if operator in ("~", "!~"): @@ -248,7 +252,7 @@ def _extract_comparison_rhs(self, ctx, operator) -> ValueExpr | RegexValue | Set return IpRangeValue(raw=ctx.iprange().getText()) if ctx.value(): return self._extract_value(ctx.value()) - raise ValueError(f"Unhandled comparison RHS at line {ctx.start.line}") + raise self._unhandled("comparison RHS", ctx) def _extract_modifiers(self, ctx) -> tuple[str, ...]: if ctx.modifier(): diff --git a/tools/hrw4u/tests/test_ast_nodes.py b/tools/hrw4u/tests/test_ast_nodes.py index 245e1f7b0a3..c849d4f92c0 100644 --- a/tools/hrw4u/tests/test_ast_nodes.py +++ b/tools/hrw4u/tests/test_ast_nodes.py @@ -15,6 +15,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import dataclasses + import pytest from hrw4u.ast_nodes import Span @@ -32,14 +34,9 @@ def test_is_hashable_so_it_can_key_a_span_index(self): def test_is_immutable(self): span = Span(file="a", line=1, column=0) - with pytest.raises(Exception): + with pytest.raises(dataclasses.FrozenInstanceError): span.line = 2 - def test_rejects_unknown_attributes(self): - # slots=True: a typo'd field must fail loudly rather than sit unread on the instance. - with pytest.raises(AttributeError): - Span(file="a", line=1, column=0).lineno = 2 - - -if __name__ == "__main__": - pytest.main([__file__, "-v"]) + def test_carries_no_instance_dict(self): + # slots=True: frozen already blocks assignment, so check the layout itself. + assert not hasattr(Span(file="a", line=1, column=0), "__dict__")