Qiskit / openqasm3_parser

Parser and semantic analyzer for the OpenQASM3 language
Apache License 2.0
10 stars 11 forks source link

`unwrap()` called on `None` panic when parsing qasm3 generated by Qiskit #110

Closed mtreinish closed 7 months ago

mtreinish commented 7 months ago

In attempting to load the qasm using the Qiskit importer I received an internal panic in oq3_semantic that it was running unwrap() on a None value:

thread '<unnamed>' panicked at [/root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/oq3_semantics-0.0.7/src/syntax_to_semantics.rs:254:44](http://localhost:8888/root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/oq3_semantics-0.0.7/src/syntax_to_semantics.rs#line=253):
called `Option::unwrap()` on a `None` value

It looks like here: https://github.com/Qiskit/openqasm3_parser/blob/3e9a05b79f19c756a7dec9e20ca2943060d23dd8/crates/oq3_semantics/src/syntax_to_semantics.rs#L253 the lhs of the BinExpr is None. My assumption this is coming from the custom gate definitions in the qasm. If so and this is just a duplicate of #82 we can close this as such, it just wasn't the most obvious error message so I wasn't 100% and opened an issue.

The qasm file

OPENQASM 3.0;
include "stdgates.inc";
gate rccx _gate_q_0, _gate_q_1, _gate_q_2 {
  u2(0, pi) _gate_q_2;
  u1(pi/4) _gate_q_2;
  cx _gate_q_1, _gate_q_2;
  u1(-pi/4) _gate_q_2;
  cx _gate_q_0, _gate_q_2;
  u1(pi/4) _gate_q_2;
  cx _gate_q_1, _gate_q_2;
  u1(-pi/4) _gate_q_2;
  u2(0, pi) _gate_q_2;
}
gate sxdg _gate_q_0 {
  s _gate_q_0;
  h _gate_q_0;
  s _gate_q_0;
}
gate xx_minus_yy_126160435882640(_gate_p_0, _gate_p_1) _gate_q_0, _gate_q_1 {
  rz(-1.6565605764620428) _gate_q_1;
  rz(-pi/2) _gate_q_0;
  sx _gate_q_0;
  rz(pi/2) _gate_q_0;
  s _gate_q_1;
  cx _gate_q_0, _gate_q_1;
  ry(3.02874427413078) _gate_q_0;
  ry(-3.02874427413078) _gate_q_1;
  cx _gate_q_0, _gate_q_1;
  sdg _gate_q_1;
  rz(-pi/2) _gate_q_0;
  sxdg _gate_q_0;
  rz(pi/2) _gate_q_0;
  rz(1.6565605764620428) _gate_q_1;
}
bit[3] c;
qubit[3] q;
rccx q[1], q[2], q[0];
h q[1];
t q[2];
p(0.7419890998827317) q[0];
sxdg q[0];
xx_minus_yy_126160435882640(6.05748854826156, 1.6565605764620428) q[2], q[1];
c[0] = measure q[0];
c[1] = measure q[1];
c[2] = measure q[2];

The qasm was just generated with:

from qiskit.circuit.random import random_circuit
from qiskit.qasm3 import dump

qc = random_circuit(3, 3, measure=True, seed=1234)
with open("qasm3_failure.qasm", "w") as fd:
    dump(qc, fd)
jlapeyre commented 7 months ago

Thanks for the report. The current main branch of openqasm3_parser parses your example above without panic and without reporting syntax or semantic errors.

I'll release a version. I don't think I changed the API, but I have not been testing carefully with the qiskit importer.

jlapeyre commented 7 months ago

If I run qiskit.qasm3.load_experimental(filename) with the current main branch of the parser, importing fails with

QASM3ImporterError: "can't handle non-built-in gate: 'rccx'"

Importing gate definitions has not yet been implemented in the importer.

I also had to add a couple of enum variants to a default match arm, because the importer lists all variants individually. I have been adding variants as I add support for parts of the langauge.

I should probably close #82 as it is rather irrelevant at the moment.

jlapeyre commented 7 months ago

I just made this release https://github.com/Qiskit/openqasm3_parser/releases/tag/0.2.0 This produces the behavior in the previous comment. Namely, there is no longer a panic. But a QASM3ImporterError error is raised because the example code is not supported by the importer.

Here is a branch of qiskit that changes the dependency to openqasm3_parser 0.2.0. The parent commit is the 1.0rc1 commit. https://github.com/jlapeyre/qiskit-terra/tree/update-for-openqasm3

jlapeyre commented 7 months ago

Adding a dbg! shows that the panic is indeed when analyzing u1(-pi/4) _gate_q_2; . And the problem is the unary minus. This is a dup of #81, which was fixed in #92 .