fix: mangle OpenQASM keywords #28

atharva-satpute · 2024-06-07T18:43:24Z

Issue #12

Description of changes:
OpenQASM reserved keywords names will be mangled to <keyword>_

Testing done: Yes

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rmshaffer · 2024-06-07T19:40:47Z

@atharva-satpute The unit test failures you're seeing (failures here) are actually a great thing - these were tests that were using variable names like input and qubit which are technically reserved words in OpenQASM. Now that you're (correctly) appending _ to these, can you update those tests so that their expected OpenQASM strings match (i.e., you can change input -> input_ and qubit -> qubit_, etc., until the tests are passing).

src/autoqasm/api.py

1. Fixed few existing test cases that got affected due to this change 2. Added reserved keyword list

codecov · 2024-06-10T16:04:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (360cab1) to head (cd088a6).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #28   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        47    +1     
  Lines         2043      2057   +14     
  Branches       342       343    +1     
=========================================
+ Hits          2043      2057   +14

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

yitchen-tim · 2024-06-10T17:17:18Z

src/autoqasm/api.py

@@ -35,6 +35,63 @@
 from autoqasm.program.gate_calibrations import GateCalibration
 from autoqasm.types import QubitIdentifierType as Qubit

+reserved_keywords = [


Do you have a reference/link to the source of this list?

https://openqasm.com/grammar/index.html

Could you please include a permalink reference as a comment in the code here? Perhaps you can use this URL: https://github.com/openqasm/openqasm/blob/909379e450f5e1b6cdd67b8a96683caca7b13e4d/source/grammar/qasm3Lexer.g4

Also, it would be ideal to move this list (and the function is_reserved_keyword) to a separate file (e.g. reserved_keywords.py) and then just import the function is_reserved_keyword here.

Thanks, I think in long term, it would be better that this list is not hard-coded. Rather, the list depends on the qasm3lexer from the lexer file. But I think it does not necessary need to be implement in this PR. Could you add the reference link as an inline comment?

Thanks, I think in long term, it would be better that this list is not hard-coded. Rather, the list depends on the qasm3lexer from the lexer file. But I think it does not necessary need to be implement in this PR. Could you add the reference link as an inline comment?

Yes, initially, I had done it that way.

rmshaffer · 2024-06-10T17:56:15Z

src/autoqasm/api.py

@@ -35,6 +35,63 @@
 from autoqasm.program.gate_calibrations import GateCalibration
 from autoqasm.types import QubitIdentifierType as Qubit

+reserved_keywords = [


Could you please include a permalink reference as a comment in the code here? Perhaps you can use this URL: https://github.com/openqasm/openqasm/blob/909379e450f5e1b6cdd67b8a96683caca7b13e4d/source/grammar/qasm3Lexer.g4

Also, it would be ideal to move this list (and the function is_reserved_keyword) to a separate file (e.g. reserved_keywords.py) and then just import the function is_reserved_keyword here.

src/autoqasm/api.py

test/unit_tests/autoqasm/test_types.py

1. Created a new file for the reserved keywords

rmshaffer

A few very minor suggestions, but otherwise this looks good to me! Thank you for sticking with this!

test/unit_tests/autoqasm/test_types.py

src/autoqasm/api.py

src/autoqasm/reserved_keywords.py

src/autoqasm/api.py

src/autoqasm/reserved_keywords.py

rmshaffer

🥳

jcjaskula-aws

LGTM. Just wondering if we can improve one error message to avoid confusion since we modify variable name in the background.

test/unit_tests/autoqasm/test_api.py

src/autoqasm/reserved_keywords.py

jcjaskula-aws

LGTM, thanks a lot for your contribution.

src/autoqasm/reserved_keywords.py

Co-authored-by: Lauren Capelluto <[email protected]>

atharva-satpute · 2024-06-13T14:26:35Z

LGTM, thanks a lot for your contribution.

Thanks to you guys

rmshaffer · 2024-06-13T19:00:19Z

Thanks to you guys

Thank you for all the work on this! 🥳

rmshaffer reviewed Jun 7, 2024

View reviewed changes

src/autoqasm/api.py Outdated Show resolved Hide resolved

rmshaffer mentioned this pull request Jun 10, 2024

AutoQASM program transpilation should mangle OpenQASM reserved keywords (if they're not Python keywords) #12

Closed

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from 1dd7e86 to fd2b8bd Compare June 10, 2024 15:48

atharva-satpute marked this pull request as ready for review June 10, 2024 15:50

atharva-satpute requested a review from a team as a code owner June 10, 2024 15:50

atharva-satpute commented Jun 10, 2024

View reviewed changes

src/autoqasm/api.py Outdated Show resolved Hide resolved

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from d39dcd3 to 30e1290 Compare June 10, 2024 15:56

Fix: Append '_' to the variable names that are reserved keywords

54ff709

1. Fixed few existing test cases that got affected due to this change 2. Added reserved keyword list

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from 30e1290 to 54ff709 Compare June 10, 2024 16:00

yitchen-tim reviewed Jun 10, 2024

View reviewed changes

rmshaffer requested changes Jun 10, 2024

View reviewed changes

Add test to verify name mangling works for reserved keywords

3d477b0

1. Created a new file for the reserved keywords

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from e9debfc to 3d477b0 Compare June 10, 2024 19:25

Change return type to tuple

19084a9

rmshaffer requested changes Jun 11, 2024

View reviewed changes

Change function signature to return new name

7cdc39d

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from 19c69f8 to 7cdc39d Compare June 11, 2024 17:04

rmshaffer previously approved these changes Jun 11, 2024

View reviewed changes

jcjaskula-aws reviewed Jun 11, 2024

View reviewed changes

test/unit_tests/autoqasm/test_api.py Show resolved Hide resolved

src/autoqasm/reserved_keywords.py Outdated Show resolved Hide resolved

Separate keywords using comments

beb4801

atharva-satpute dismissed rmshaffer’s stale review via beb4801 June 12, 2024 16:52

atharva-satpute force-pushed the mangle-openqasm-reserved-words branch from 269ddc9 to beb4801 Compare June 12, 2024 16:52

jcjaskula-aws previously approved these changes Jun 12, 2024

View reviewed changes

jcjaskula-aws and others added 2 commits June 12, 2024 15:00

Merge branch 'main' into mangle-openqasm-reserved-words

83dfd3c

Merge branch 'main' into mangle-openqasm-reserved-words

58dadab

laurencap reviewed Jun 12, 2024

View reviewed changes

src/autoqasm/reserved_keywords.py Outdated Show resolved Hide resolved

rmshaffer linked an issue Jun 12, 2024 that may be closed by this pull request

AutoQASM program transpilation should mangle OpenQASM reserved keywords (if they're not Python keywords) #12

Closed

rmshaffer previously approved these changes Jun 12, 2024

View reviewed changes

jcjaskula-aws dismissed stale reviews from rmshaffer and themself via cd088a6 June 12, 2024 21:24

turn into single-liner

cd088a6

Co-authored-by: Lauren Capelluto <[email protected]>

jcjaskula-aws approved these changes Jun 12, 2024

View reviewed changes

rmshaffer approved these changes Jun 13, 2024

View reviewed changes

rmshaffer merged commit f83bb15 into amazon-braket:main Jun 13, 2024
15 checks passed

rmshaffer mentioned this pull request Jun 17, 2024

Unit tests produce OpenQASM programs which use reserved keywords as variable names #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: mangle OpenQASM keywords #28

fix: mangle OpenQASM keywords #28

atharva-satpute commented Jun 7, 2024 •

edited

Loading

rmshaffer commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

yitchen-tim Jun 10, 2024

atharva-satpute Jun 10, 2024

rmshaffer Jun 10, 2024

yitchen-tim Jun 10, 2024 •

edited

Loading

atharva-satpute Jun 10, 2024

rmshaffer Jun 10, 2024

rmshaffer left a comment

rmshaffer left a comment

jcjaskula-aws left a comment

jcjaskula-aws left a comment

atharva-satpute commented Jun 13, 2024

rmshaffer commented Jun 13, 2024

fix: mangle OpenQASM keywords #28

fix: mangle OpenQASM keywords #28

Conversation

atharva-satpute commented Jun 7, 2024 • edited Loading

Merge Checklist

General

Tests

rmshaffer commented Jun 7, 2024 • edited Loading

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

yitchen-tim Jun 10, 2024

Choose a reason for hiding this comment

atharva-satpute Jun 10, 2024

Choose a reason for hiding this comment

rmshaffer Jun 10, 2024

Choose a reason for hiding this comment

yitchen-tim Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

atharva-satpute Jun 10, 2024

Choose a reason for hiding this comment

rmshaffer Jun 10, 2024

Choose a reason for hiding this comment

rmshaffer left a comment

Choose a reason for hiding this comment

rmshaffer left a comment

Choose a reason for hiding this comment

jcjaskula-aws left a comment

Choose a reason for hiding this comment

jcjaskula-aws left a comment

Choose a reason for hiding this comment

atharva-satpute commented Jun 13, 2024

rmshaffer commented Jun 13, 2024

atharva-satpute commented Jun 7, 2024 •

edited

Loading

rmshaffer commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

yitchen-tim Jun 10, 2024 •

edited

Loading