visitor.py LOC cleanup by micpap25 · Pull Request #348 · qBraid/pyqasm

micpap25 · 2026-08-05T04:24:38Z

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!

argus-eye · 2026-08-05T04:24:41Z

Argus review

Auto-review is off for this repo. Tick the box below to run a review on this PR.

Trigger Argus review

Estimated cost

Files changed: 2
Diff lines (±): 47
Historical avg: ~318.9k tokens · ~$1.35 · across last 6 review(s)

Tip: you can also comment @argus-eye review at any time.

coderabbitai · 2026-08-05T04:24:47Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fc9e460-9bff-4b5d-8af3-1950d2a40dec

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-05T04:24:54Z

👋 Hey there! It looks like the changelog might need an update.

Please take a moment to edit the CHANGELOG.md with:

A brief, one-to-two sentence summary of your changes.
A link back to this PR for reference.
(Optional) A small working example if you've added new features.

TheGupta2012

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

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 · 2026-08-06T13:35:24Z

+    def wrapper(self, *args, **kwargs):
+        """Wrapper that intercepts the return value and replaces it with an empty list."""
+        result = func(self, *args, **kwargs)
+        if self.check_only:


B1 — blocking: wrong attribute name, breaks 539 of 648 tests.

The attribute is self._check_only (set at L144); there is no check_only property on QasmVisitor.

src/pyqasm/visitor.py:3260: in visit_statement E AttributeError: 'QasmVisitor' object has no attribute 'check_only' src/pyqasm/visitor.py:95: AttributeError

Every decorated method raises on first call. s/self.check_only/self._check_only/ alone takes the suite from 539 failed to 6 failed.

ryanhill1 · 2026-08-06T13:35:24Z

        #    each element in the list of the values
        #    should be of const int type and no duplicates should be present

+        @semantic_check_gate


B2 — blocking: decorator applied to a nested function, breaks all 6 switch tests.

_evaluate_case(statements) is a closure, not a method. The wrapper's first positional parameter is named self, so it binds to the statements list:

tests/qasm3/test_switch.py::test_switch E AttributeError: 'list' object has no attribute '_check_only' src/pyqasm/visitor.py:95: AttributeError

(That traceback is after B1 is fixed — this is an independent bug.) The closure already reads self._check_only from the enclosing scope, so the decorator buys nothing here. Removing it is the fix; with B1 and B2 both fixed the suite is 648 passed / 3 skipped.

ryanhill1 · 2026-08-06T13:35:24Z

                    qubit_node = self._module._qubit_depths[(qubit_name, qubit_id)]
                    qubit_node.depth = max_involved_depth

+    @semantic_check_gate


B3 — blocking: decorator displaces a block-scoped # pylint: disable, adding 10 pylint errors.

A standalone # pylint: disable= comment is block-scoped. Inserting the decorator above it detaches it from the class body scope, so the suppression stops applying:

visitor.py:1203:4: R0914: Too many local variables (17/15) visitor.py:1437:4: R0914: Too many local variables (20/15) visitor.py:1950:4: R0912: Too many branches (25/12) ... 10 total

main scores 10.00/10 on this file. Moving the decorator below the comment at both sites (here and L1714) restores it — verified, drops pylint to a single remaining error (B4).

ryanhill1 · 2026-08-06T13:35:25Z


        return statements

+    @semantic_check_gate


B3 (cont.) — same displacement here, for # pylint: disable=too-many-branches, too-many-statements, too-many-locals.

ryanhill1 · 2026-08-06T13:35:25Z

+        result = func(self, *args, **kwargs)
+        if self.check_only:
+            return []
+        else:


B4 — blocking: pylint failure.

visitor.py:95:8: R1705: Unnecessary "else" after "return" (no-else-return)

if self._check_only: return [] return result

ryanhill1 · 2026-08-06T13:35:25Z

            qasm3_ast.CalibrationGrammarDeclaration: self._visit_calibration_grammar_declaration,
        }

+    @semantic_check_gate


N1 — semantic_check_gate reads as "quantum gate" in this file. Something like skip_if_check_only or _check_only_returns_empty says what it does without the collision.

N2 — worth type-hinting func: Callable[..., Any] while you're here; #346 is adding hints across these modules and this would land unhinted.

ryanhill1 · 2026-08-06T13:35:25Z

-            )
-        self._handle_extern_function_cleanup(statements, statement)
+            function_name = statement.init_expression.name.name
+            if function_name in FUNCTION_MAP and isinstance(init_value, (float, int)):


N3 — the inlining looks behaviour-preserving. The old helper(...) or statement.init_expression fallback was equivalent because FloatLiteral is a truthy dataclass, so nothing was relying on the or.

Only note: this is now duplicated three times (here, L1943, L2113). If it grows a fourth case, a small _maybe_fold_function_call(expr, value) helper would be worth it — but three identical two-liners is arguably clearer than the indirection you removed, so no objection.

ryanhill1 · 2026-08-06T13:35:25Z


        Returns:
-            None
+            list[qasm3_ast.QuantumReset] - A list of unrolled resets.


N4 — the rest of the file uses Google style with a colon: list[qasm3_ast.QuantumReset]: A list of unrolled resets. This one uses a dash.

ryanhill1 · 2026-08-06T13:35:25Z

-            base_type: The declared type (DurationType or StretchType)
-            rvalue: The initializer or assigned value
+            statement: The AST statement node.
+            statement_type: The expected AST node type.


N5 — pre-existing, not yours, but you're editing this block: statement_type isn't a parameter. It's derived inside the function at L128 (statement_type = type(statement)). Worth deleting the line while you're here.

ryanhill1 · 2026-08-06T13:35:25Z

-            rvalue: The initializer or assigned value
+            statement: The AST statement node.
+            statement_type: The expected AST node type.
+            base_type: The declared type, function does nothing if not DurationType or StretchType


N6 — this is the one line in the hunk that didn't get a trailing period, and the other three did.

Start on visitor cleanup

1a297c5

micpap25 requested a review from TheGupta2012 as a code owner August 5, 2026 04:24

remove a single-use function, simplify check_only

46fd472

TheGupta2012 reviewed Aug 5, 2026

View reviewed changes

Comment thread src/pyqasm/visitor.py Outdated

TheGupta2012 reviewed Aug 5, 2026

View reviewed changes

Comment thread src/pyqasm/visitor.py

TheGupta2012 reviewed Aug 5, 2026

View reviewed changes

Comment thread src/pyqasm/visitor.py Outdated

TheGupta2012 reviewed Aug 5, 2026

View reviewed changes

micpap25 added 2 commits August 5, 2026 09:19

Apply comments and lint file

aeccc52

Refactor _handle_function_init_expression

3638488

Function is simple but has unintuitive logic; the "return None" approach used here should be removed if we decide not to use this refactoring.

micpap25 mentioned this pull request Aug 6, 2026

typing and docstring fixes in modules folder and visitor.py #346

Open

ryanhill1 reviewed Aug 6, 2026

View reviewed changes

more changes requested

453cee4

Uh oh!

Conversation

micpap25 commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Uh oh!

argus-eye Bot commented Aug 5, 2026

Argus review

Uh oh!

coderabbitai Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheGupta2012 left a comment

Choose a reason for hiding this comment

Uh oh!

ryanhill1 left a comment

Choose a reason for hiding this comment

What I ran

The design question — R1

Not blocking, worth knowing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

micpap25 commented Aug 5, 2026 •

edited

Loading

coderabbitai Bot commented Aug 5, 2026 •

edited

Loading