Impl/topic09

[FeelTheBeats]

No description provided.

[github-actions]

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 3 个文件超过上限（最多 10 个）未审查

📁 .github/workflows/ci.yml

This is a clean, consistent addition that follows existing patterns. No blockers.

💭 **Consistency**: The new test step (lines 110-115) is placed right after `test_pr37_regression.py` but has no comment header like the benchmark sections (e.g., `# ── 3.1.2 课题09 ... ──`). Minor inconsistency in style — consider adding a comment header to match the benchmark section pattern above.

💭 **Test granularity**: Three test files are bundled into one step. If `test_dsl_errors_integration.py` fails, the output for `test_dsl_errors.py` results may be harder to isolate in CI logs. Consider splitting into separate steps for better failure localization, matching the pattern used by `test_pr37_regression.py` (which runs as a standalone step).

No correctness, security, or maintainability concerns. The mkdir -p, --tb=short, -f file guard, and GITHUB_STEP_SUMMARY append all match existing conventions perfectly.

---

📁 benchmarks/run_topic09_errors_case.py

🔴 Bug: strict-mode error-code compared against wrong code space — evaluate unpacks EXPECTED_COMPILER_DIAGNOSTICS[0] (line 5, col 5, "E200") and compares to strict["error_code"]. But strict comes from ExtendedDSLParser without a collector, which emits rich-code-space values (ErrorCode.SEM_UNKNOWN_OP = E301). The check will always fail. Use EXPECTED_RICH_DIAGNOSTICS[0][2] (or the enum) for that comparison.

🔴 Bug: rich_positions_match_case compares mixed types — EXPECTED_RICH_DIAGNOSTICS is written with ErrorCode.SEM_UNKNOWN_OP / ErrorCode.SYN_MISSING_TERMINATOR (enum members) while EXPECTED_COMPILER_DIAGNOSTICS uses bare "E200" strings. Whatever err.error_code returns from diagnostic_payload, one side of rich_positions == EXPECTED_RICH_DIAGNOSTICS won't match unless ErrorCode.__eq__ is customised to compare against its string value. Either normalise to strings ("E301" etc.) or convert the collected side to the same enum type — pick one.

🔴 Bug: has_marker requires a | on the caret line — diagnostic_payload sets "has_marker": "^" in marker and "|" in marker, and all_rich_diagnostics_render_markers iterates over every rich diagnostic. In a typical gcc/clang gutter render the | sits on the source-display line (line[1]), not on the caret line (line[2]). If render_error puts the gutter separator on the source line, this check is always False and the whole report is red. Verify against actual output and drop the | clause, or read the separator from line[1].

🟡 Fragile token lookup in render_via_api — token_col = source_display.find("retrun") hard-codes the misspelled token and -1 on miss silently disables the alignment check. Derive the token from err.message or pass the expected token in as a parameter so the check doesn't silently no-op if the fixture or the spelling error changes.

🟡 Hard-coded caret width — EXPECTED_COLUMN_MARKER = "^~~~~~" (6 tildes) pins the token width. Any edit to the retrun token (or a different token picked for the render sample) breaks the check. Compute the expected caret length from the actual token length (or err.end_col - err.col) instead.

🟡 Positional indexing of rendered_lines — rendered_lines[1], [2], [3] in diagnostic_payload and render_via_api assume a fixed 4-line render layout. If format_error/render_error adds a | gutter header or wraps long messages, this mis-parses silently. Consider scanning for the first line containing ^ for the marker and the line above it for the source display.

🟡 check_collector_limit assumes one error per generated line — expected_suppressed = LIMIT_CASE_LINES - LIMIT_CASE_MAX_ERRORS presumes retrun(x, i) yields exactly one diagnostic per line. If the parser ever also reports an arity mismatch on retrun, this assertion is wrong. Compute the expected suppressed count from collector.error_count after parsing, or use a simpler generated input (e.g. repeated retrun).

🟡 Silent fall-through in spelling_err render sample — when no SEM_UNKNOWN_OP diagnostic exists, render falls back to a hand-built dict with every field empty/False. That will make the downstream hard checks fail with unhelpful values. Either raise early if the fixture invariant is missing, or use pytest.skip-style skip semantics in the return payload so the failure reason is explicit.

🟡 compiler["diagnostics"] vs result.errors — you collect result.errors (strings) alongside structured result.diagnostics. If a diagnostic has no DSLSyntaxError in diagnostics, the hard checks (all_diagnostics_have_positions, compiler_positions_match_case) will miss it. Assert len(result.errors) == len(result.diagnostics) as a sanity check.

💭 Nit: Any overuse — the payloads returned by diagnostic_payload, compile_case, render_via_api, check_collector_limit, check_strict_mode all have concrete shapes. Tightening to TypedDict (or a small dataclass) would make the schema auditable from the type signatures instead of only from the emitted JSON, and catch the enum/string mismatch above at compile time.

💭 Nit: # pragma: no cover - defensive on the broad except Exception — the branch is unreachable in normal operation but is the exact place a real regression would surface. Add a logger.warning (or print to stderr) so the report explains why strict mode raised a non-DSL error.

💭 Nit: Markdown table cell escaping — d["message"] and d["suggestion"] are interpolated into a |-delimited table with no escaping. If a message ever contains |, the table renders broken. Escape | → \| in the values.

💭 Nit: DEFAULT_CASE uses .parents[1] — fine from benchmarks/, but if this script is ever moved to benchmarks/subdir/ it silently points at the wrong repo root. Path(__file__).resolve().parents[N] with a comment, or walk up to a marker file (pyproject.toml), is more robust.

---

📁 scratchv/compiler.py

🔴 Import hoisting risk — Line 26: from scratchv.frontend.dsl_errors import DSLParseError, DSLSyntaxError moved to module top-level. The original code deliberately lazy-imported it inside the try block — likely to avoid a circular import (compiler.py ↔ frontend/*). Verify there's no cycle; if there is, this will fail at import time, not at first use, which is worse. Keep the lazy import unless you've confirmed no cycle.

🔴 source can be None — _parse() now passes source directly to ExtendedDSLParser().parse(source, ...) after removing source or "". When both dsl_source and input_path are None, source is None and the parser receives None instead of "". Either preserve the or "" guard or validate source upfront.

🟡 filename can be None — Original used filename=input_path or "<dsl>". New code leaves filename=None when dsl_source is supplied without an input_path, so error diagnostics lose the <dsl> label. Set filename = input_path or "<dsl>" before the try block.

🟡 Lazy import inconsistency — from scratchv.frontend.dsl_parser import DSLParser is still deferred inside the except DSLParseError branch while dsl_errors is now top-level. Pick one style. If you keep top-level for dsl_errors, consider doing the same for dsl_parser (subject to the cycle check above).

🟡 Silent fallback masks bugs — except DSLParseError: fall back to DSLParser swallows any non-syntax parse error from ExtendedDSLParser and silently retries with the simpler parser. That can hide genuine parser regressions. Consider logging a warning/debug when the fallback is taken, or gating it behind a flag, so test failures don't disappear.

🟡 Behavior change in compile() for DSLParseError — Only DSLSyntaxError is converted to CompileResult; DSLParseError propagates as an uncaught exception. The old code did the same, but worth confirming this is intentional now that DSLParseError is an imported symbol — readers will expect symmetric handling.

💭 Exception ordering assumption — except DSLSyntaxError: raise before except DSLParseError: fall back is only correct if DSLSyntaxError is a subclass of DSLParseError. If they're independent classes the order is fine but the comment ("Precise, positioned error") is misleading. Document the class hierarchy or add an assertion.

---

📁 scratchv/frontend/__init__.py

No issues found — the diff is clean.

💭 Nit: __all__ ordering — The list mixes alphabetical grouping with category grouping. The first block (ONNXParser → suggest_spelling) is roughly alphabetical, but DSLValidator, OP_SIGNATURES, SourceBuffer are appended at the end, breaking the sort (e.g. DSLValidator < ExtendedDSLParser). This was already the case before the diff, so it's not a regression — just worth a comment noting the intended grouping if it's deliberate.

---

📁 scratchv/frontend/dsl_errors.py

🔴 Regression: matmul arity hint — _ARITY_HINTS["matmul"] changed from "requires rows:, cols:, inner: kwargs (e.g., m:2, n:2, k:2)" to "requires exactly 2 arguments". If matmul is genuinely keyword-driven (as the old hint and SEM_UNKNOWN_KWARG suggest), this misinforms users and makes the SEM_UNKNOWN_KWARG/kwarg error path inconsistent with the arity path. Verify the real signature; if both forms exist, keep the richer hint.

🔴 SEM_UNKNOWN_OP = "E301" conflicts with docstring example — The module docstring shows error[E301]: unexpected token 'retrun', which is a spelling/token-level issue, but E3xx is documented as semantic. Either fix the example (use E1xx/E2xx for misspellings) or reclassify SEM_UNKNOWN_OP. As written, reading the docstring teaches a wrong convention.

🟡 dataclasses.replace/asdict compatibility — Moving from @dataclass (generated __init__) to @dataclass(init=False) + hand-written __init__ breaks any caller using dataclasses.replace(err, ...) (it requires generated __init__) and changes field ordering of dataclasses.fields(). If any test/fixture reconstructs errors this way, it will silently fail. If none do, consider init=True + custom __post_init__, or document the constraint.

🟡 fix_hint vs suggestion silent precedence — if fix_hint is None: fix_hint = suggestion means if a caller passes both, suggestion is silently discarded. Prefer raising or explicitly documenting "fix_hint wins". At minimum, the docstring for suggestion should say so.

🟡 errors property re-sorts every access — sorted(...) on each property read; report() iterates it once, but has_errors/error_count don't need it. Cache or expose unsorted internally to avoid O(n log n) per access.

🟡 ErrorCode gap at E303 — Jumps E302 → E304. Either reserve E303 with a comment (# E303 reserved) or fill the sequence; gaps in stable codes invite future collision.

🟡 ErrorCollector.add mutates its argument — err.filename = self.filename mutates a caller-owned object. If the same DSLSyntaxError is re-used (cached, logged elsewhere), it now carries a stale filename. Copy instead, or document the mutation contract.

🟡 context_lines silently ignored when source is None — Old code attempted context rendering regardless; new code requires source and otherwise no-ops. This is a behavior change that callers of format_error(err, context_lines=2) (without source) will hit silently. Emit a debug warning or assert, or fall back to repeating err.source_line.

🟡 _compute_suggestion arity regex is brittle — r"(\w+)\(\)\s+expects\s+" requires the exact phrase op() expects N. Any parser that emits arity mismatch for mul: got 3, want 2 will not match. Consider matching the arity hint from the caller (pass op/arg_count explicitly) instead of parsing the message.

💭 suppressed_count semantics — _suppressed counts only errors arriving after the limit, not dedup-suppressed ones. Name/comment should clarify (e.g., over_limit_count) so callers don't assume it's total dropped.

💭 report() header — "--- N error(s) found ---" is verbose and now appears in colored bold; consider a plainer f"{N} error(s) in {filename}" matching modern compilers.

---

📁 scratchv/frontend/dsl_extended.py

🔴 Breaking base contract: super()._parse_line(line, line_no) — Line in _parse_line: the base class DSLParser._parse_line must now accept a second line_no argument. This diff alone breaks if the base class wasn't updated in a companion change. Verify the base signature was updated, or guard with inspect.signature.

🟡 Off-by-one risk: _line_indent vs 0-based _raw_lines — line_no is consistently 1-based (idx + 1, start_idx + 1), and _raw_lines is 0-indexed. If _line_indent(n) does self._raw_lines[n] instead of self._raw_lines[n - 1], every diagnostic column will be wrong (and line 0 would hit the off-by-one crash). Add an assertion or comment to lock the contract.

🟡 _recover_after_bad_header depth tracking is ambiguous — Lines that match endif or endwhile are both treated as depth decrements regardless of which opener they actually match. Example:

while bad:
    while good:
        ...
    endif        # stray — but decrements depth for the good while
endwhile         # now consumed as the bad header's terminator

This mis-consumes the endwhile that belongs to the outer (correct) block. For a recovery-only path this may be acceptable, but document the limitation — or track opener kinds in a stack instead of a flat depth counter.

🟡 unclosed is captured-then-ignored pattern — unclosed is computed before _report_unclosed_for() and _report_unclosed_while() clear the stacks, then used to skip the auto-return. This works but is fragile: if a future refactor removes one of the report calls, the guard silently breaks. Consider checking _loop_stack / _while_stack after the reports and replacing unclosed with a clearer name like _had_unclosed_blocks.

🟡 _parse_block swallows endfor via _parse_line which swallows if/while — The extended _parse_line returns early for if/while keywords (they're handled at block level), but _parse_block already dispatches those before reaching _parse_line. If someone adds a new block keyword to _parse_block but forgets to add it to _parse_line's guard, they get a silent double-parse. Consider extracting the set of block keywords into a module-level constant shared by both methods.

💭 __all__ re-exports DSLParseError / DSLSyntaxError — These come from dsl_errors, not this module. Re-exporting is fine for convenience but encourages from scratchv.frontend.dsl_extended import DSLSyntaxError instead of the canonical from scratchv.frontend.dsl_errors import .... Either remove them from __all__ or add a comment explaining the re-export intent.

💭 Optional[str] vs str | None mixed in same file — Method signatures use Optional[str] but _while_stack annotation uses str | None. The file has from __future__ import annotations so both work at runtime, but pick one style for consistency.

---

📁 scratchv/frontend/dsl_parser.py

🔴 Bug: silent arithmetic in argument expressions — Line ~280: the illegal-char whitelist includes * and /, so x = mul(1 * 2) passes all checks. _parse_value("1 * 2") fails numeric parse → returns string → _resolve creates a phantom variable. The DSL has no arithmetic operators, so these chars should be in the blacklist (or removed from the whitelist).

🟡 Dead code in _dispatch_op — Line ~398: resolved: list[Value] = [] is immediately overwritten by resolved = [self._resolve(a) for a in plain]. Remove the initializer.

🟡 Per-call assert — Line ~429: assert set(handlers) == self.supported_operations() rebuilds a set and compares on every _dispatch_op invocation. Hoist to __init__ or module-level.

🟡 Redundant guard — Line ~212: if not self._loop_stack and not unclosed_loop — _report_unclosed_for() already empties _loop_stack, so not self._loop_stack is always True. Simplify to if not unclosed_loop.

🟡 Data duplication — _ARITY (line ~50) and _NUMBER (line ~55) duplicate info already in OP_SIGNATURES and the validator's own _NUMBER. If they diverge, arity checks and numeric validation silently disagree. Consider deriving _ARITY from OP_SIGNATURES and importing _NUMBER from the validator.

🟡 Incomplete kwargs validation — Line ~362: when signature is None (op not in OP_SIGNATURES), allowed = frozenset(), so all kwargs are rejected with E304. If OP_SIGNATURES is incomplete, valid kwargs get false-rejected. Add a warning or skip kwarg validation when signature is None.

🟡 Inline comment edge case — Line ~224: line.split(" #", 1)[0] requires a space before #. x = mul(1,2)#comment isn't stripped → 2)#comment becomes a phantom variable. Consider re.sub(r"\s*#.*$", "", line) or at least line.split("#", 1)[0] (safe here since # has no other meaning).

🟡 _for_positions / _loop_stack sync fragility — Line ~120: _report_unclosed_for pops _loop_stack unconditionally but _for_positions conditionally. If they ever desync (e.g., a future code path pushes one but not the other), error positions silently drift. Consider asserting length equality after parse.

---

📁 scratchv/frontend/dsl_validator.py

🔴 Unclosed-block errors may flood output — Line 147: Removing the limit check from the stack-unwinding loop means a deeply nested file with early errors will report all unclosed blocks on top of the already-limited inline errors. If the intent is to cap total errors, these two changes need coordination: either add the unclosed-block checks into the main loop's budget, or gate the unwinding on a separate quota. As-is, a file with 50 unclosed if blocks would dump 50 E111 errors regardless of limit.

🟡 Wasted parsing cycles — Line 101: The main loop now calls self._statement() and full validation on every line even after limit_reached. On large files with early syntax errors this is wasted work. Consider keeping the early break but moving it after unclosed-block detection, or add the break back but skip only when there are no structural checks remaining:

if collector.limit_reached and not stack:
    break

💭 No test for the removed behavior — Was there a test asserting the validator stops early? If not, add one to confirm that (a) all errors in a file are reported, and (b) performance is acceptable on a large file with many errors.

---

📁 tests/data/dsl_golden_ir.json

🔴 SSA dominance violation in if/else merge blocks — Cases 016, 018, 021: The merge block (if_end3) returns a variable defined only in one branch. E.g., case 016 returns $v_2 but it only exists in if_else2; if if_then1 is taken, $v_2 is undefined. There are no phi nodes at merge points. Either the IR format needs phi instructions, or the compiler is emitting semantically invalid IR that these golden files will silently validate as correct.

🔴 Infinite while loops in golden data — Cases 017, 022: The loop condition checks $i < N but $i is never incremented. Compare with examples/cfg/while_loop.dsl which correctly does $v_4 = add $i $v_3. These golden files encode loops that never terminate — if they're meant to test the compiler output, the compiler is also generating broken IR here.

🟡 Float-typed loop bounds — Cases 017, 022, and examples/cfg/while_loop.dsl: Loop counters and bounds use value=10.0, value=5.0, value=1.0. Integer loop variables are more conventional and avoid floating-point comparison edge cases. Is this a deliberate IR design choice? If so, it should be documented.

🟡 matmul placeholder parameters — Cases 007 and examples/matmul_test.dsl: Parameters [m=2] [n=2] [k=2] look like hardcoded defaults rather than derived from actual tensor shapes. If the DSL source specifies shapes differently, this golden data won't catch regressions.

💭 Redundant golden entries: 001_simple_add ↔ examples/simple_add, 008_dot ↔ examples/dot_product, 007_matmul ↔ examples/matmul_test produce identical IR. Fine for separate test suites, but consider deduplicating or adding a comment explaining the split.

💭 File ordering: Keys are not alphabetically sorted (e.g., examples/cfg/ entries appear before examples/dot_product.dsl). Trivial, but alphabetical ordering makes diffs and manual inspection easier.

---

📁 tests/data/topic09_dsl_errors_feature.dsl

🔴 Comment misleading — Line 2 says "asserts the exact diagnostics below" but no diagnostics are listed below. The reader cannot tell what errors CI expects. Consider listing expected error codes/messages inline (e.g., # triggers: undefined_name('retrun')) or link to the benchmark script's assertion table.

🟡 Scope leak ambiguity — Line 10 return d references d, which is only assigned inside the if block (line 5–6). Whether this counts as an extra error depends on whether the DSL has block scoping. If it does, the file triggers an unintended 4th error (undefined_variable/unbound), and CI's "exact diagnostics" assertion will fail silently on any scope-model change. Either annotate the intent (# d is intentionally out-of-scope or # d is intentionally in scope) or move the return inside the if.

🟡 Parser recovery assumption — Line 4 b = retrun(a, 1): is retrun an undefined-name error (runtime) or a parse-time error (reserved-word-ish typo)? If the parser halts on this error and never reaches line 8, the endwhile error won't fire. Worth confirming the parser has error-recovery so all expected diagnostics actually surface — otherwise CI is flaky on parser-implementation changes.

🟡 No test-side comment per line — A one-line annotation next to each intentional error (e.g., b = retrun(a, 1) # expect: undefined_function) would make future parser refactors safer and eliminate the "do not fix" guesswork.

💭 Style nit — Mixing if (...): (colon) with endwhile (no colon) is fine for an error fixture, but a single header comment noting "syntax mix is intentional to probe both branches of the tokenizer" prevents well-meaning cleanup PRs.

---

---

⚠️ 未审查的文件

• tests/test_dsl_errors.py
• tests/test_dsl_errors_integration.py
• tests/test_topic09_errors_case_report.py