-
Notifications
You must be signed in to change notification settings - Fork 30
Handle general expression statements #425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e3138d0
8019589
fcd1eec
85d58f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,7 +196,7 @@ def _construct_visit_map(self): | |
| qasm3_ast.SwitchStatement: self._visit_switch_statement, | ||
| qasm3_ast.SubroutineDefinition: self._visit_subroutine_definition, | ||
| qasm3_ast.ExternDeclaration: self._visit_subroutine_definition, | ||
| qasm3_ast.ExpressionStatement: lambda x: self._visit_function_call(x.expression), | ||
| qasm3_ast.ExpressionStatement: self._visit_expression_statement, | ||
| qasm3_ast.IODeclaration: lambda x: [], | ||
| qasm3_ast.BreakStatement: self._visit_break, | ||
| qasm3_ast.ContinueStatement: self._visit_continue, | ||
|
|
@@ -1638,6 +1638,15 @@ def _visit_generic_gate_operation( # pylint: disable=too-many-branches, too-man | |
| ) | ||
| return stmts # type: ignore | ||
|
|
||
| if ( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain a little why this change is needed here? Just want to understand this through an example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. With |
||
| isinstance(operation, qasm3_ast.QuantumGate) | ||
| and operation.name.name not in self._custom_gates | ||
| and not self._is_black_box_gate(operation.name.name) | ||
| ): | ||
| # Resolve the operation before its operands so an unknown gate is | ||
| # reported even when one of its qubits is also undeclared. | ||
| map_qasm_op_to_callable(operation) | ||
|
|
||
| self._in_generic_gate_op_scope += 1 | ||
|
|
||
| # only needs to be done once for a gate operation | ||
|
|
@@ -3522,6 +3531,24 @@ def _visit_include(self, include: qasm3_ast.Include) -> list[qasm3_ast.Statement | |
|
|
||
| return [include] | ||
|
|
||
| @staticmethod | ||
| def _visit_expression_statement( | ||
| statement: qasm3_ast.ExpressionStatement, | ||
| ) -> list[qasm3_ast.Statement]: | ||
| """Evaluate an expression statement and discard its value. | ||
|
|
||
| Statements produced while evaluating the expression are retained so | ||
| that calls to user-defined and external functions keep their effects. | ||
|
|
||
| Args: | ||
| statement (ExpressionStatement): The expression statement to visit. | ||
|
|
||
| Returns: | ||
| list[Statement]: Statements produced while evaluating the expression. | ||
| """ | ||
| _, statements = Qasm3ExprEvaluator.evaluate_expression(statement.expression) | ||
| return statements | ||
|
|
||
| def _visit_end_statement( | ||
| self, statement: qasm3_ast.EndStatement | ||
| ) -> list[qasm3_ast.EndStatement]: | ||
|
|
@@ -3560,12 +3587,7 @@ def visit_statement( | |
|
|
||
| visitor_function = self._visit_map.get(type(statement)) | ||
| if visitor_function: | ||
| if isinstance(statement, qasm3_ast.ExpressionStatement): | ||
| # these return a tuple of return value and list of statements | ||
| _, ret_stmts = visitor_function(statement) # type: ignore[operator] | ||
| result.extend(ret_stmts) | ||
| else: | ||
| result.extend(visitor_function(statement)) # type: ignore[operator] | ||
| result.extend(visitor_function(statement)) # type: ignore[operator] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. N2 — #388 also asked for a defensive I think you're right to have left it out: a blanket catch at the dispatch would convert genuine internal bugs into validation errors and bury them, which is a bad trade for a net that my fuzzing suggests is already empty. No change requested. The only issue is that the PR says |
||
| else: | ||
| raise_qasm3_error( | ||
| f"Unsupported statement of type {type(statement)}", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N3 — this list now has to stay in sync with
_visit_function_call.That method special-cases eight names; four of them (
get_phase,get_frequency,newframe,play) aren't in any of the three maps, so they're spelled out again here. I checked the current set is complete —set_phase,set_frequency,shift_phaseandshift_frequencyall come in viaOPENPULSE_FRAME_FUNCTION_MAP— so there's no bug today.But a ninth added to
_visit_function_calland not here would silently reroute to the generic evaluator. Existing tests would catch it loudly, so this is maintainability only. Could you pull the four literals into a module-level constant that both sites use, or failing that leave a comment here pointing at_visit_function_call?