feat: Implement built-in `state_result` function #905

tatiana-s · 2025-04-01T14:59:11Z

Closes #899

codecov-commenter · 2025-04-01T15:02:06Z

Codecov Report

Attention: Patch coverage is 98.83721% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.16%. Comparing base (51b1b81) to head (bf464c5).

Files with missing lines	Patch %	Lines
guppylang/std/_internal/debug.py	97.72%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #905      +/-   ##
==========================================
+ Coverage   93.12%   93.16%   +0.04%     
==========================================
  Files          84       86       +2     
  Lines        9701     9775      +74     
==========================================
+ Hits         9034     9107      +73     
- Misses        667      668       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tatiana-s · 2025-04-08T14:28:31Z

guppylang/compiler/expr_compiler.py

+        )
+
+        qubits_in = [self.visit(e) for e in node.args[1:]]
+        # TODO: This should be caught during checking.


Importing qubit for the StateResultChecker in checker.py leads to circular import errors - not sure if there is an obvious other place to move it to

The problem is that qubit is a library defined type and not specifically known by the compiler (unlike the types in builtins).

You would need to do something like the following:

class StateResultChecker(CustomCallChecker): """Call checker for the `state_result` function.""" def synthesize(self, args: list[ast.expr]) -> tuple[ast.expr, Type]: ... from guppylang.std.quantum import qubit # Need to make sure that the quantum module is loaded qubit_defn = self.globals[qubit.id] assert isinstance(qubit_defn, TypeDef) qubit_ty = qubit_defn.check_instantiate([], self.globals) # Check against qubit_ty

Also, this only works if the quantum module is loaded. Therefore, it might be a good idea to move state_result into quantum? Or its own debug module that depends on quantum?

tatiana-s · 2025-04-08T14:29:16Z

guppylang/compiler/expr_compiler.py

+        qubits_arr = self.builder.add_op(array_new(ht.Qubit, num_qubits), *qubits_in)
+        qubits_arr = self.builder.add_op(op, qubits_arr)
+        qubits_out = unpack_array(self.builder, qubits_arr)


Not sure if using arrays in the hugr op is worth this overhead or there is a better way of representing the list of qubits in the extension

I think this is fine for now. Would be a good idea to add an overload of result_state that takes an array anyways as a follow-up

tatiana-s · 2025-04-08T14:30:21Z

tests/error/misc_errors/state_result_missing_tag.err

+10 | def main() -> None:
+11 |     q1 = qubit()
+12 |     state_result(q1)
+   |                  ^^ Expected expression of type `str`, got `qubit`


(While it would be nice to be more specific about the missing tag this is the same as result at the moment)

I think this is fine and also consistent with the message you would get when calling regular functions 👍

mark-koch

Looks good! Just a couple of nits and suggestions

mark-koch · 2025-04-08T16:25:39Z

guppylang/std/_internal/checker.py

+            raise GuppyTypeError(ExpectedError(tag, "a string literal"))
+        if len(tag.value.encode("utf-8")) > TAG_MAX_LEN:
+            err: Error = TooLongError(tag)
+            err.add_sub_diagnostic(TooLongError.Hint(None))
+            raise GuppyTypeError(err)


Could you add tests for those two errors?

mark-koch · 2025-04-08T16:26:29Z

tests/error/misc_errors/state_result_missing_tag.err

+10 | def main() -> None:
+11 |     q1 = qubit()
+12 |     state_result(q1)
+   |                  ^^ Expected expression of type `str`, got `qubit`


I think this is fine and also consistent with the message you would get when calling regular functions 👍

mark-koch · 2025-04-08T16:38:11Z

guppylang/compiler/expr_compiler.py

+        )
+
+        qubits_in = [self.visit(e) for e in node.args[1:]]
+        # TODO: This should be caught during checking.


The problem is that qubit is a library defined type and not specifically known by the compiler (unlike the types in builtins).

You would need to do something like the following:

class StateResultChecker(CustomCallChecker): """Call checker for the `state_result` function.""" def synthesize(self, args: list[ast.expr]) -> tuple[ast.expr, Type]: ... from guppylang.std.quantum import qubit # Need to make sure that the quantum module is loaded qubit_defn = self.globals[qubit.id] assert isinstance(qubit_defn, TypeDef) qubit_ty = qubit_defn.check_instantiate([], self.globals) # Check against qubit_ty

Also, this only works if the quantum module is loaded. Therefore, it might be a good idea to move state_result into quantum? Or its own debug module that depends on quantum?

mark-koch · 2025-04-09T08:17:16Z

guppylang/compiler/expr_compiler.py

+        qubits_arr = self.builder.add_op(array_new(ht.Qubit, num_qubits), *qubits_in)
+        qubits_arr = self.builder.add_op(op, qubits_arr)
+        qubits_out = unpack_array(self.builder, qubits_arr)


I think this is fine for now. Would be a good idea to add an overload of result_state that takes an array anyways as a follow-up

mark-koch · 2025-04-09T08:21:39Z

guppylang/std/_internal/checker.py

+            err.add_sub_diagnostic(TooLongError.Hint(None))
+            raise GuppyTypeError(err)
+
+        tys = [ExprSynthesizer(self.ctx).synthesize(val)[1] for val in args]


You shouldn't ignore the expression returned by synthesize. E.g. instead

Suggested change

tys = [ExprSynthesizer(self.ctx).synthesize(val)[1] for val in args]

args, tys = zip(*(ExprSynthesizer(self.ctx).synthesize(val) for val in args), strict=True)

Also no need to resynthesize the type for args[0] since you already checked it above

mark-koch · 2025-04-09T08:25:35Z

guppylang/std/_internal/checker.py

+            [FuncInput(tys[0], InputFlags.NoFlags)]
+            + [FuncInput(t, InputFlags.Inout) for t in tys[1:]],


Then you could just do

Suggested change

[FuncInput(tys[0], InputFlags.NoFlags)]

+ [FuncInput(t, InputFlags.Inout) for t in tys[1:]],

[FuncInput(string_type(), InputFlags.NoFlags)]

+ [FuncInput(t, InputFlags.Inout) for t in tys],

here

mark-koch · 2025-04-09T08:28:33Z

tests/error/misc_errors/state_result_not_qubit.err

+9 |     state_result("tag", x)
+  |     ^^^^^^^^^^^^^^^^^^^^^^ Non-qubit inputs to state_result are not supported


Would be nicer if the span only highlighted x

mark-koch · 2025-04-09T08:38:02Z

tests/integration/test_state_result.py

+def test_empty(validate):
+    @compile_quantum_guppy
+    def main() -> None:
+        state_result("tag")


Not sure if we should allow this. I could see users writing this with the assumption that the whole state will be outputted because they are not aware that you have to pass the qubits.

Since empty state results are useless anyways, maybe it would be more helpful to add a diagnostic along the lines of

2 | state_result("tag") | ^ Qubits whose state should be reported must be passed explicitly

tatiana-s added 2 commits April 1, 2025 15:57

Implement builtin state_result debug function

fa94aae

Merge remote-tracking branch 'origin/main' into ts/state-result

a9dc3a9

tatiana-s added 5 commits April 8, 2025 12:11

Lower to state result

4b6dc3b

Merge remote-tracking branch 'origin/main' into ts/state-result

f3861d4

Mark op as having side effects

930aa5b

Add more tests

86abbb4

Add error tests

4459c05

tatiana-s commented Apr 8, 2025

View reviewed changes

tatiana-s requested a review from mark-koch April 8, 2025 14:31

tatiana-s marked this pull request as ready for review April 8, 2025 14:31

tatiana-s requested a review from a team as a code owner April 8, 2025 14:31

tatiana-s added the wait to merge This PR to be merged after all dependencies are ready label Apr 8, 2025

mark-koch reviewed Apr 9, 2025

View reviewed changes

Address comments

bf464c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement built-in `state_result` function #905

feat: Implement built-in `state_result` function #905

tatiana-s commented Apr 1, 2025 •

edited

Loading

codecov-commenter commented Apr 1, 2025 •

edited

Loading

tatiana-s Apr 8, 2025

mark-koch Apr 8, 2025

tatiana-s Apr 8, 2025

mark-koch Apr 9, 2025

tatiana-s Apr 8, 2025

mark-koch Apr 8, 2025

mark-koch left a comment

mark-koch Apr 8, 2025

mark-koch Apr 8, 2025

mark-koch Apr 8, 2025

mark-koch Apr 9, 2025

mark-koch Apr 9, 2025

mark-koch Apr 9, 2025

mark-koch Apr 9, 2025

mark-koch Apr 9, 2025

mark-koch Apr 9, 2025

	tys = [ExprSynthesizer(self.ctx).synthesize(val)[1] for val in args]
	args, tys = zip(*(ExprSynthesizer(self.ctx).synthesize(val) for val in args), strict=True)

		[FuncInput(tys[0], InputFlags.NoFlags)]
		+ [FuncInput(t, InputFlags.Inout) for t in tys[1:]],

		9 \| state_result("tag", x)
		\| ^^^^^^^^^^^^^^^^^^^^^^ Non-qubit inputs to state_result are not supported

feat: Implement built-in state_result function #905

Are you sure you want to change the base?

feat: Implement built-in state_result function #905

Conversation

tatiana-s commented Apr 1, 2025 • edited Loading

codecov-commenter commented Apr 1, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-koch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Implement built-in `state_result` function #905

feat: Implement built-in `state_result` function #905

tatiana-s commented Apr 1, 2025 •

edited

Loading

codecov-commenter commented Apr 1, 2025 •

edited

Loading