Reject other QASM versions in QASM 2 parser #7456

3yakuya · 2022-01-01T05:57:02Z

Summary

Fixing QASM-319, qasm parser will fail with appropriate error if invalid OPENQASM version is specified. Reno release note is added.

Details and comments

Small change in qasm lexer to only accept 2.0 as a version, rather than any digit.digit. The latter caused a bug where user could provide any version, like 3.0 and believe it works while in reality their code was still parsed as 2.0.

Should not break any workflows, unless they rely on broken openqasm strings. Added reno release note. Did not see any place in the documentation per se that would require updating.

Other numerical will now result in error from parser.

3yakuya · 2022-01-01T19:53:34Z

Its effectively blocked by #7457

jakelishman

Thanks for looking at this!

If we're making more programmes fail, we should make sure that our error messages are particularly clear - in this case, it'd be good if we can ensure that the error tells us that only parsing of QASM 2 is currently supported, if we encounter a bad OPENQASM statement. At the moment, the error is:

QasmError: 'Unable to match any token rule, got -->O<-- Check your OPENQASM source and any include statements.'

which isn't super helpful.

qiskit/qasm/qasmlexer.py

releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml

3yakuya · 2022-01-04T14:54:07Z

Makes sense, thanks! Will try to look late today or early tomorrow

Lexer accepts any structurally proper version string, parser fails if it is other than 2.0. Added tests and updated docs.

…akuya/fix-openqasm-319

3yakuya · 2022-01-13T00:52:26Z

@jakelishman any chance for some quick eyes here?

jakelishman

Sorry this took so long for me to look at again. The errors look much better now, in most of the situations, I just had one little tweak for a new situation that OpenQASM 3 allows, but OpenQASM 2 doesn't, that'll improve them a little more too.

We're good to go on this after that.

qiskit/qasm/qasmparser.py

releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml

coveralls · 2022-01-13T20:10:40Z

Pull Request Test Coverage Report for Build 1714209317

7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.0003%) to 83.141%

Files with Coverage Reduction	New Missed Lines	%
qiskit/extensions/quantum_initializer/uc.py	2	87.94%

Totals
Change from base Build 1714031495:	0.0003%
Covered Lines:	51983
Relevant Lines:	62524

💛 - Coveralls

Co-authored-by: Jake Lishman <jake@binhbar.com>

Version 2; should succeed, as the lexer will accept single-digit versions. Parser adds default version 0. OPENQASM 2; or OPENQASM 2.0; both succeed, else fail.

qiskit/qasm/node/format.py

test/python/test_qasm_parser.py

jakelishman · 2022-01-17T16:12:34Z

test/python/qasm/example_version_2.qasm

@@ -0,0 +1,11 @@
+OPENQASM 2;


The fact that OPENQASM 2; works now is kind of a side-effect - it'd be best for the good test file to use 2.0.

For this one we have a few tests that run with 2.0 and I wanted to keep at least one that shows that indeed a single-digit version will work

That's a good idea, let's leave it as it is.

jakelishman · 2022-01-17T16:14:15Z

test/python/qasm/example_version_fail.qasm

@@ -0,0 +1,11 @@
+OPENQASM 5.0;


You already test OPENQASM 3; so this isn't testing anything new; a good other test would be that minor versions of 2 are also detected, such as OPENQASM 2.1; - this would have caught the error with has_minor_version above, because the file would have parsed successfully when it should have thrown an error.

Eh you got more work than help with my contribution here, sorry for rookie mistakes! Working on it

Don't worry about it - it's not really true anyway. Before you made the PR, I didn't know how hard or easy it would be to fix. After you'd made it, all I had to do was check that what you'd done was sensible, not actually go through and do it myself, and you did this well.

Finding extra special test cases, catching weirdnesses with re and things like that is just more experience, and it's why we have review anyway.

Well I appreciate the encouragement and would continue to work on this anyways :D

Tests now fail with incorrect minor version. Tests of qasmparser cleaned up a bit.

jakelishman

I pushed up a couple of commits just to simplify the creation of some strings, and fix a typo in the test case (which was probably my fault anyway). This is good to merge now, and it's stable for backport, so we'll put it out in Terra 0.19.2 whenever we get around to releasing that 🎉

* Explicitly specifying QASM version 2.0 in lexer. Other numerical will now result in error from parser. * adding release note. * Moving QASM version check to parser. Lexer accepts any structurally proper version string, parser fails if it is other than 2.0. Added tests and updated docs. * Fixing qasm version checking. * Fixing tab in docs * Update releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml Co-authored-by: Jake Lishman <jake@binhbar.com> * Improved error message for invalid QASM version. * Adding ability to parse OPENQASM single-digit ver Version 2; should succeed, as the lexer will accept single-digit versions. Parser adds default version 0. OPENQASM 2; or OPENQASM 2.0; both succeed, else fail. * Linting and adding docstrings * Fixing QASM minor-version parsing & testing Tests now fail with incorrect minor version. Tests of qasmparser cleaned up a bit. * Convert string expressions to format strings * Remove unused test assertion * Fix typo in test case Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 2d8bb5c)

* Explicitly specifying QASM version 2.0 in lexer. Other numerical will now result in error from parser. * adding release note. * Moving QASM version check to parser. Lexer accepts any structurally proper version string, parser fails if it is other than 2.0. Added tests and updated docs. * Fixing qasm version checking. * Fixing tab in docs * Update releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml Co-authored-by: Jake Lishman <jake@binhbar.com> * Improved error message for invalid QASM version. * Adding ability to parse OPENQASM single-digit ver Version 2; should succeed, as the lexer will accept single-digit versions. Parser adds default version 0. OPENQASM 2; or OPENQASM 2.0; both succeed, else fail. * Linting and adding docstrings * Fixing QASM minor-version parsing & testing Tests now fail with incorrect minor version. Tests of qasmparser cleaned up a bit. * Convert string expressions to format strings * Remove unused test assertion * Fix typo in test case Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 2d8bb5c) Co-authored-by: Kuba Pilch <6464505+3yakuya@users.noreply.github.com>

3yakuya added 2 commits January 1, 2022 00:28

Explicitly specifying QASM version 2.0 in lexer.

8a7c5a1

Other numerical will now result in error from parser.

adding release note.

950c148

3yakuya requested a review from a team as a code owner January 1, 2022 05:57

3yakuya force-pushed the 3yakuya/fix-openqasm-319 branch from c844ac5 to 950c148 Compare January 3, 2022 17:02

jakelishman linked an issue Jan 4, 2022 that may be closed by this pull request

QASM does not error when unrecognized version is provided #7465

Closed

jakelishman reviewed Jan 4, 2022

View reviewed changes

qiskit/qasm/qasmlexer.py Outdated Show resolved Hide resolved

releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml Outdated Show resolved Hide resolved

releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml Outdated Show resolved Hide resolved

jakelishman changed the title ~~3yakuya/fix openqasm 319~~ Reject other QASM versions in QASM 2 parser Jan 4, 2022

3yakuya added 6 commits January 4, 2022 13:59

Moving QASM version check to parser.

9e61df8

Lexer accepts any structurally proper version string, parser fails if it is other than 2.0. Added tests and updated docs.

Merge branch 'main' of https://github.com/Qiskit/qiskit-terra into 3y…

7968431

…akuya/fix-openqasm-319

Fixing qasm version checking.

6472d8a

Merge branch 'main' of https://github.com/Qiskit/qiskit-terra into 3y…

130e3fc

…akuya/fix-openqasm-319

Fixing tab in docs

81e134f

Merge branch 'main' of https://github.com/Qiskit/qiskit-terra into 3y…

39c4aed

…akuya/fix-openqasm-319

3yakuya requested a review from jakelishman January 5, 2022 15:23

jakelishman reviewed Jan 13, 2022

View reviewed changes

qiskit/qasm/qasmparser.py Outdated Show resolved Hide resolved

releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml Outdated Show resolved Hide resolved

3yakuya and others added 4 commits January 16, 2022 13:58

Update releasenotes/notes/qasm-lexer-bugfix-1779525b3738902c.yaml

77c0121

Co-authored-by: Jake Lishman <jake@binhbar.com>

Improved error message for invalid QASM version.

1eacd64

Adding ability to parse OPENQASM single-digit ver

9eea938

Version 2; should succeed, as the lexer will accept single-digit versions. Parser adds default version 0. OPENQASM 2; or OPENQASM 2.0; both succeed, else fail.

Linting and adding docstrings

31af32a

3yakuya requested a review from jakelishman January 17, 2022 14:56

jakelishman reviewed Jan 17, 2022

View reviewed changes

Fixing QASM minor-version parsing & testing

6696124

Tests now fail with incorrect minor version. Tests of qasmparser cleaned up a bit.

3yakuya force-pushed the 3yakuya/fix-openqasm-319 branch from f1da822 to 6696124 Compare January 18, 2022 03:20

jakelishman added 3 commits January 18, 2022 18:12

Convert string expressions to format strings

542bee9

Remove unused test assertion

82dccac

Fix typo in test case

ead201f

Merge remote-tracking branch 'ibm/main' into 3yakuya/fix-openqasm-319

36a6620

jakelishman approved these changes Jan 18, 2022

View reviewed changes

jakelishman added automerge Changelog: Bugfix Include in the "Fixed" section of the changelog stable backport potential The bug might be minimal and/or import enough to be port to stable labels Jan 18, 2022

jakelishman added this to the 0.19.2 milestone Jan 18, 2022

mergify bot merged commit 2d8bb5c into Qiskit:main Jan 18, 2022

mergify bot mentioned this pull request Jan 18, 2022

Reject other QASM versions in QASM 2 parser (backport #7456) #7536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject other QASM versions in QASM 2 parser #7456

Reject other QASM versions in QASM 2 parser #7456

3yakuya commented Jan 1, 2022

3yakuya commented Jan 1, 2022

jakelishman left a comment

3yakuya commented Jan 4, 2022

3yakuya commented Jan 13, 2022

jakelishman left a comment

coveralls commented Jan 13, 2022 •

edited

Loading

jakelishman Jan 17, 2022

3yakuya Jan 17, 2022

jakelishman Jan 18, 2022

jakelishman Jan 17, 2022

3yakuya Jan 17, 2022

jakelishman Jan 18, 2022

3yakuya Jan 18, 2022

jakelishman left a comment

Reject other QASM versions in QASM 2 parser #7456

Reject other QASM versions in QASM 2 parser #7456

Conversation

3yakuya commented Jan 1, 2022

Summary

Details and comments

3yakuya commented Jan 1, 2022

jakelishman left a comment

Choose a reason for hiding this comment

3yakuya commented Jan 4, 2022

3yakuya commented Jan 13, 2022

jakelishman left a comment

Choose a reason for hiding this comment

coveralls commented Jan 13, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1714209317

💛 - Coveralls

jakelishman Jan 17, 2022

Choose a reason for hiding this comment

3yakuya Jan 17, 2022

Choose a reason for hiding this comment

jakelishman Jan 18, 2022

Choose a reason for hiding this comment

jakelishman Jan 17, 2022

Choose a reason for hiding this comment

3yakuya Jan 17, 2022

Choose a reason for hiding this comment

jakelishman Jan 18, 2022

Choose a reason for hiding this comment

3yakuya Jan 18, 2022

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

coveralls commented Jan 13, 2022 •

edited

Loading