visitor.py LOC cleanup - #348
Conversation
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 Hey there! It looks like the changelog might need an update. Please take a moment to edit the
|
TheGupta2012
left a comment
TheGupta2012
left a comment
There was a problem hiding this comment.
Thanks @micpap25 for working on this! Can you please resolve the comments and continue the work for refactoring the visitor? This seems like a good start!
Function is simple but has unintuitive logic; the "return None" approach used here should be removed if we decide not to use this refactoring.
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Thanks for picking up #188 — consolidating the repeated if self._check_only: return [] tail into a decorator is the right instinct, and 26 net lines out of visitor.py is real progress. The _handle_function_init_expression inlining is behaviour-preserving as far as I can tell, and mypy, black and isort are all clean.
The decorator itself has two bugs that need fixing before this can go further, and once they're fixed there's a design question about early returns that I think decides whether this approach works at all.
Test suite on the branch as pushed: 539 failed, 109 passed (pytest tests --deselect tests/cli).
What I ran
Patching B1 alone → 6 failures left. Patching B1 + B2 → 648 passed, 3 skipped, matching main. So B1 and B2 account for the entire breakage, and the rest of the refactor doesn't regress anything the suite covers.
Reordering the two displaced # pylint: disable comments (B3) drops pylint from 11 errors to 1. main scores 10.00/10 on visitor.py, so all 11 are new here.
The design question — R1
The decorator wraps the whole function, so it now also swallows early returns that previously bypassed the _check_only gate. Three are reachable, all on openpulse paths. Demonstrated with a pulse program:
m = loads(pulse_program_with_defcal_measure)
m.validate()
len(m._unrolled_ast.statements)
# main: 4 (QubitDeclaration, CalibrationGrammarDeclaration, CalibrationStatement, QuantumMeasurementStatement)
# PR: 3 (QuantumMeasurementStatement dropped)No test covers this, which is why the suite is green once B1/B2 are fixed. It may well be that returning [] there is more correct — but #188 says explicitly that behavioural changes should be proposed separately, so this needs to be either a deliberate, tested decision or excluded from the refactor. Details inline at R1.
Not blocking, worth knowing
- Merge conflict with #346. That PR modifies
_handle_function_init_expression; this one deletes it. Whichever lands second will need a rebase. - No CHANGELOG entry, and you've flagged tests as outstanding yourself — #188 asks for coverage on refactors specifically to catch things like R1.
Labels: B blocking, R semantics, D dead code, N nits.
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Re-checked at 453cee4. All 15 threads are fixed — resolved. This is a good round; the R1 resolution in particular is exactly right.
Verified, not eyeballed:
-
B1–B4 —
visitor.pyis now pylint-clean (was 11 errors). Moving the decorator below the two# pylint: disablecomments restored the block scoping, and dropping it from the nested_evaluate_caseclosure fixed the switch tests. -
R1 — you took the conservative route: decorator off the three methods with reachable early returns, explicit
if self._check_only: return []restored at the tail. I re-ran the differential probe that originally caught this:validate() on a defcal-measure pulse program -> statements in _unrolled_ast main: 4 (…, QuantumMeasurementStatement) this PR: 4 (…, QuantumMeasurementStatement) # was 3Parity with
mainrestored. I also re-swept the 11 still-decorated methods: the only early returns left are the three I'd classified as safe (_visit_phase_operation,_visit_classical_declarationboth delegate to already-gated methods;_visit_classical_assignmentreturns[]regardless). No nested-function applications remain. -
D1 — both
StretchTypebranches reverted. -
N1/N2 — renamed, and
F = TypeVar("F", bound=Callable[..., Any])preserves the wrapped signatures.
Suite: 648 passed, 3 skipped. mypy, black, isort all clean. Net −25 lines.
One new thing, and three carried over
G1 — the docstring fix introduced a line-too-long in validator.py. That's the only thing failing CI now; inline below.
Carried over from my first pass, none of them blocking on their own:
- No tests. You flagged this yourself, and #188 asks for coverage specifically so that refactors like R1 get caught by CI rather than by review. The three
check_only+ openpulse paths are the obvious candidates — there's currently no test that would have failed when R1 was live. - No CHANGELOG entry.
- #346 still conflicts. It modifies
_handle_function_init_expression; this deletes it. GitHub saysMERGEABLEonly because neither has landed yet — whichever goes second needs a rebase.
One for @TheGupta2012 to settle
The decorator is now check_only_return_empty, following my N1. I hadn't seen at the time that you'd already suggested semantic_check_gate in an earlier round — so that rename overrode your call, which wasn't my intent. Happy either way; worth one of you picking so it doesn't churn again.
| rvalue: The initializer or assigned value | ||
| global_scope: Global symbol table. | ||
| statement (Statement): The AST statement node. | ||
| base_type (Any): The declared type, function does nothing if not DurationType or StretchType. |
There was a problem hiding this comment.
G1 — new line-too-long, fails CI.
src/pyqasm/pulse/validator.py:118:0: C0301: Line too long (105/100)
Adding (Any) and the trailing period (N5/N6) pushed it over. Wrapping the clause onto a continuation line matches how the rest of the file handles long Args: entries:
base_type (Any): The declared type; the function does nothing if it is
neither DurationType nor StretchType.This is the only remaining CI failure — visitor.py is clean, and mypy/black/isort all pass.
| F = TypeVar("F", bound=Callable[..., Any]) | ||
|
|
||
|
|
||
| def check_only_return_empty(func: F) -> F: |
There was a problem hiding this comment.
Confirming N1/N2 as fixed. F -> F correctly preserves the wrapped signature (the earlier Callable[..., Any] would have erased it), and # type: ignore on the return is the standard escape for this pattern — mypy is clean.
Worth noting for whoever extends this: the decorator is only safe on methods whose every return should be suppressed under check_only. That's what R1 turned on. If a fourth method gets decorated later, the check is "does it have an early return that isn't itself gated" — a one-line AST sweep catches it.
|
@ryanhill1 Let's get #346 merged first and I can do the rebasing here. |
|
@ryanhill1 Is there a test file that you think would be appropriate for the tests for |
3 participants
Summary of changes
Start on #188
First commit fixes some typos and starts a validation function; long-term goal is to aggregate more of the validation process into helper functions.
Still needs tests!