Fix several bugs in the build-time AST rewrite by splch · Pull Request #57 · ionq/blqs

splch · 2026-05-01T19:36:21Z

Summary

Cluster of fixes to the @blqs.build AST rewrite. Most impactful:

del of native values didn't unbind them. The rewrite emitted del standard_targets (a helper tuple) instead of the user's variables, so del a for a plain int left a bound. New regression test confirms it now raises NameError after del.
blqs_cirq.build mutated the caller's BuildConfig. Each call prepended two more decorator specs to the user's config; now uses dataclasses.replace.
AST rewrite ran on every call. Now cached on first call; eliminates per-call tempfile / sys.modules leak and per-call rewrite cost.
while/for rewrites double-evaluated the test/iter expressions. Side-effecting conditions and one-shot iterables now match native Python.
_target_names crashed on obj.x = ... / del obj.x. Now returns None and the rewriter falls back to native semantics.
SupportsIsWritable._is_writeable_ spelled with extra 'e' — matched neither is_writable() nor Register._is_writable_.
Op.__eq__ had reversed isinstance arguments; switched to strict type check.
While.__str__ newline placement aligned with For/If.

Test plan

pytest blqs blqs_cirq — 279 passed (added one regression test for del actually unbinding)
black --check --line-length=100 blqs blqs_cirq
mypy --config-file=ci/mypy.ini blqs/blqs blqs_cirq/blqs_cirq
pylint --rcfile=ci/.pylintrc blqs_cirq/blqs_cirq/ blqs/blqs

cla-bot · 2026-05-01T19:36:25Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T19:42:23Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T19:44:39Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T19:49:34Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T19:57:07Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T20:00:00Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

cla-bot · 2026-05-01T20:40:49Z

We require contributors to sign our Contributor License Agreement, and we don't have one on file for your GitHub username. In order for us to review and merge your code, please contact @mjk or opensource@ionq.com to sign the CLA.

- `del` of non-blqs values now actually unbinds them (the rewrite was emitting `del standard_targets`, which deleted the helper tuple instead of the user's variables). - `_target_names` now returns None for attribute/subscript targets instead of raising "this should not happen"; callers fall back to native Python for those statements. - `while`-loop test expression is no longer re-evaluated after the loop. Tracking exit-reason with a flag avoids doubling side effects for non-trivial conditions. - `for`-loop iter expression is bound to a local once instead of being evaluated up to four times. One-shot iterables (file handles, generators) now work correctly. - AST rewrite, tempfile write, and module import are done once on the first call and cached, instead of per call. Eliminates the per-call tempfile / sys.modules leak and the per-call rewrite cost. - `blqs_cirq.build` no longer mutates the caller's `BuildConfig`. Each call previously prepended two more decorator specs to `blqs_build_config.additional_decorator_specs`. - `SupportsIsWritable` protocol method renamed from `_is_writeable_` to `_is_writable_` to match the `is_writable()` consumer (and the `Register` implementation). - `Op.__eq__` uses strict `type(self) is type(other)` rather than the reversed-isinstance pattern, which leaked subclass equality when the subclass didn't override `__eq__`. - `While.__str__` newline placement now matches `For.__str__` and `If.__str__`. Adds a regression test that confirms native bindings are unbound after `del` inside `@blqs.build`.

CI installs unpinned black, which has tightened rules since these files were last formatted (trailing-comma in long signatures, no blank line between an import block and the immediately-following block, spacing around `# type: ignore`).

antalszava

Looks good overall 🎉 Left a few minor comments (without thorough background knowledge), mostly nitpicking.

Most of the fixed cases don't seem to have newly added test cases - would we like such to be added?

antalszava · 2026-06-03T11:48:31Z

+            if "line_map" not in cache:
+                cache["line_map"] = _ast.construct_line_map(
+                    cache["transformed_gast"], cache["transformed_source_code"]
+                )


we're caching the line map too - could it happen that we have two distinct exceptions for the same inputs (and so using the same line map through caching outputs invalid line nums for the 2nd exception)?

no the line map only maps generated line numbers to original ones, so it's the same for every exception. i added a comment noting that :)

antalszava · 2026-06-03T11:50:56Z


    def __eq__(self, other):
-        if not isinstance(self, type(other)):
+        if type(self) is not type(other):


(Minor as unsure how much testing we'd like here): is there a test case for this?

good idea! i added a subclass test that isn't equal to a base Op of the same name

antalszava · 2026-06-03T11:55:43Z

+        # Bind the iterable to a local once. The original re-evaluated `iter` in each
+        # slot (is_iterable / For() / loop_vars() / the loop), double-running side
+        # effects on generators and the like.


if I understand correctly, we're binding here to copy the object, perhaps good to mention that. That'd clarify a bit further the mention of double-running side effects.

agreed i reworded to "Evaluate the iterable once."

antalszava · 2026-06-03T11:57:21Z

+        # loop. The original `if not test or is_readable` re-ran `test`, doubling side
+        # effects for non-trivial conditions.


The original if not test or is_readable re-ran test, doubling side # effects for non-trivial conditions.

This part of the comment won't make sense to readers of the proposed code because if not test or is_readable is not longer an existing condition. Best to rephrase.

i rephrased it so it no longer quotes the removed condition, and added a test that the while test isn't re-evaluated after the loop :)

antalszava · 2026-06-03T12:02:18Z


+        target_names = self._target_names(node.targets)
+        if target_names is None:
+            # Non-Name targets (e.g. `obj.x = ...`); leave alone for native semantics.


(Minor:) What does leave alone for native semantics refer to? maybe good to clarify

i clarified it to "skip the rewrite, run natively" (same for del) and added attribute assign + del tests

antalszava · 2026-06-03T12:10:01Z

+        )
+
+        # Natively `del` each non-deletable value. The original emitted
+        # `del standard_targets`, deleting the helper tuple rather than the user's


Suggested change

# `del standard_targets`, deleting the helper tuple rather than the user's

# `del standard_targets` is deleting the helper tuple rather than the user's

reworded along those lines, kept it past tense since it's describing the old behavior :)

antalszava · 2026-06-03T12:13:50Z

+    # Build the inner blqs config once. `dataclasses.replace` returns a fresh object
+    # rather than mutating the user's BuildConfig, and running `build_with_config`
+    # here (at decoration time) keeps the AST rewrite off the per-call path.


(minor:) 3 parts to this comment, perhaps good to split it and place each part right before each line of code it belongs to (e.g., build_with_config related on line 112). This should help future maintainability.

definitely i split it into 2, 1 per line! thank you :)

- Add tests: Op.__eq__ strict-type, for/while single-evaluation, obj.x = / del obj.x native fallback, blqs_cirq config not mutated - Clarify the For/While/Assign/Delete comments and drop the stale reference to the removed while condition - Note that the cached line map is shared across exceptions by design - Split the blqs_cirq config comment onto the lines it describes

splch · 2026-06-04T23:30:48Z

Thanks for the review @antalszava ! i added regression tests for each fix and tightened the comments per the inline notes

splch force-pushed the fix-build-rewrite-bugs branch from c32c9aa to 41eb67d Compare May 1, 2026 20:40

splch added 3 commits May 1, 2026 19:56

Restore section comments in _build

d61a9ef

Reformat to satisfy current black (26.3.1)

7094325

CI installs unpinned black, which has tightened rules since these files were last formatted (trailing-comma in long signatures, no blank line between an import block and the immediately-following block, spacing around `# type: ignore`).

mjk force-pushed the fix-build-rewrite-bugs branch from 41eb67d to 7094325 Compare May 2, 2026 00:56

cla-bot Bot added the cla-signed label May 2, 2026

splch added 2 commits May 1, 2026 18:06

Merge branch 'main' into fix-build-rewrite-bugs

755e683

Tighten comments and docstrings in the build rewrite

d0bba0b

antalszava approved these changes Jun 3, 2026

View reviewed changes

splch added 2 commits June 3, 2026 12:13

Shorten the build rewrite comments

0b0f891

Update CLA contributors list

fa123e4

mjk approved these changes Jun 5, 2026

View reviewed changes

splch merged commit 2b29a1c into main Jun 5, 2026
10 checks passed

splch deleted the fix-build-rewrite-bugs branch June 5, 2026 18:07

		# loop. The original `if not test or is_readable` re-ran `test`, doubling side
		# effects for non-trivial conditions.

	# `del standard_targets`, deleting the helper tuple rather than the user's
	# `del standard_targets` is deleting the helper tuple rather than the user's

Conversation

splch commented May 1, 2026

Summary

Test plan

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

cla-bot Bot commented May 1, 2026

Uh oh!

antalszava left a comment

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!

splch Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

splch Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

splch commented Jun 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

splch Jun 4, 2026 •

edited

Loading

splch Jun 4, 2026 •

edited

Loading