Skip to content

Commit 486df49

Browse files
committed
Address Copilot review comments
- Validate that required params precede optional params in procedure declarations; raises a clear error at declaration time. - Extract source line from input stream in _get_value_text errors so the error output includes the offending line and caret position. - Use SystemDefaults.INDENT_SPACES instead of hardcoded 4-space literals in flatten() and _flatten_conditional(). - Remove dead _collect_errors() in TestParseErrorMessages; _first_error() is the working equivalent used by all tests.
1 parent 91523d3 commit 486df49

2 files changed

Lines changed: 15 additions & 11 deletions

File tree

tools/hrw4u/src/visitor.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,12 @@ def _get_value_text(self, val_ctx) -> str:
237237
if val_ctx.paramRef():
238238
name = val_ctx.paramRef().IDENT().getText()
239239
if name not in self._proc_bindings:
240+
try:
241+
source_line = val_ctx.start.getInputStream().strdata.splitlines()[val_ctx.start.line - 1]
242+
except Exception:
243+
source_line = ""
240244
raise Hrw4uSyntaxError(
241-
self.filename, val_ctx.start.line, val_ctx.start.column, f"'${name}' used outside procedure context", "")
245+
self.filename, val_ctx.start.line, val_ctx.start.column, f"'${name}' used outside procedure context", source_line)
242246
return self._proc_bindings[name]
243247
return val_ctx.getText()
244248

@@ -547,7 +551,7 @@ def _flatten_items(self, items, indent: str, source_text: str, bindings: dict[st
547551
def _flatten_conditional(self, cond_ctx, indent: str, source_text: str, bindings: dict[str, str]) -> list[str]:
548552
"""Flatten a conditional block, expanding proc calls within its branches."""
549553
lines: list[str] = []
550-
inner_indent = indent + " "
554+
inner_indent = indent + (" " * SystemDefaults.INDENT_SPACES)
551555

552556
if_ctx = cond_ctx.ifStatement()
553557
cond_text = self._get_source_text(if_ctx.condition(), source_text)
@@ -573,7 +577,7 @@ def flatten(self, ctx, source_text: str = "") -> list[str]:
573577
if not source_text:
574578
source_text = ctx.start.source[1].getText(0, ctx.start.source[1].size - 1)
575579
self._source_text = source_text
576-
indent = " " * 4
580+
indent = " " * SystemDefaults.INDENT_SPACES
577581

578582
# Phase 1: Load all procedures (use directives + local procedure declarations)
579583
for item in ctx.programItem():
@@ -640,6 +644,14 @@ def visitProcedureDecl(self, ctx) -> None:
640644
self.filename, ctx.start.line, ctx.start.column, f"procedure '{name}' already declared in {existing.source_file}",
641645
"")
642646
params = self._collect_proc_params(ctx.paramList()) if ctx.paramList() else []
647+
seen_default = False
648+
for p in params:
649+
if p.default_ctx is None and seen_default:
650+
raise Hrw4uSyntaxError(
651+
self.filename, ctx.start.line, ctx.start.column,
652+
f"procedure '{name}': required parameter '${p.name}' must not follow an optional parameter", "")
653+
if p.default_ctx is not None:
654+
seen_default = True
643655
self._proc_registry[name] = ProcSig(name, params, ctx.block(), self.filename, self._source_text)
644656

645657
def visitProgram(self, ctx) -> list[str]:

tools/hrw4u/tests/test_units.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -286,14 +286,6 @@ def test_no_partial_word_replacement(self):
286286
class TestParseErrorMessages:
287287
"""Verify that ANTLR parse errors use human-friendly token names."""
288288

289-
def _collect_errors(self, text: str) -> list[str]:
290-
collector = ErrorCollector(max_errors=10)
291-
try:
292-
create_parse_tree(text, "<test>", hrw4uLexer, hrw4uParser, "test", collect_errors=True, max_errors=10)
293-
except Exception:
294-
pass
295-
return [str(e) for e in collector.errors]
296-
297289
def _first_error(self, text: str) -> str:
298290
_, _, collector = create_parse_tree(text, "<test>", hrw4uLexer, hrw4uParser, "test", collect_errors=True, max_errors=10)
299291
assert collector and collector.has_errors(), f"Expected parse errors for: {text!r}"

0 commit comments

Comments
 (0)