Skip to content
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

fix!: tokenize keywords and identifiers correctly #428

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

antalsz
Copy link
Contributor

@antalsz antalsz commented Jan 9, 2025

Fix the tokenization of identifiers and keywords so that keywords are not parsed within word boundaries. Previously, BITS would be parsed as the data type BIT followed by the identifier S; now, it is correctly parsed as the single identifier BITS.

This also fixes the tokenization of NaN, Inf, and Infinity; previously, these were case-insensitively parsed as floats (with the same incorrect prefix behavior, so nana would parse as the float NaN followed by the identifier a), but now they are correctly parsed as normal identifiers.

Closes #427

This prevents `BITS` from being treated as the keyword `BIT` followed by the
identifier `S`.
We could have resolved this the other way, by marking these keywords as
reserved, but the Quil spec doesn't allow for `NaN` or `Infinity` and I think
this is more useful for our purposes.
@antalsz antalsz added the bug Something isn't working label Jan 9, 2025
@antalsz antalsz requested review from Shadow53 and kalzoo January 9, 2025 06:23
@antalsz antalsz self-assigned this Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

⚠️ pyQuil Compatibility Checks Failed:

Tool Status
mypy ❌ Failed
ruff ❌ Failed
pytest ❌ Failed

Note: These failures don't necessarily block the PR but both authors and reviewers should check the results for unintentional breaking changes.

Copy link
Contributor

@jselig-rigetti jselig-rigetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really clean changes! LGTM pending CI errors

quil-rs/src/parser/lexer/mod.rs Show resolved Hide resolved
quil-rs/src/parser/token.rs Outdated Show resolved Hide resolved
quil-rs/src/parser/token.rs Outdated Show resolved Hide resolved
quil-rs/src/reserved.rs Show resolved Hide resolved
@antalsz antalsz force-pushed the 427-keyword-tokenization branch from 7117504 to 3b9cbd0 Compare January 9, 2025 17:34
Copy link

github-actions bot commented Jan 9, 2025

PR Preview Action v1.5.0
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-428/
on branch quil-py-docs at 2025-01-09 17:35 UTC

Copy link

github-actions bot commented Jan 9, 2025

⚠️ pyQuil Compatibility Checks Failed:

Tool Status
mypy ❌ Failed
ruff ❌ Failed
pytest ❌ Failed

Note: These failures don't necessarily block the PR but both authors and reviewers should check the results for unintentional breaking changes.

1 similar comment
Copy link

github-actions bot commented Jan 9, 2025

⚠️ pyQuil Compatibility Checks Failed:

Tool Status
mypy ❌ Failed
ruff ❌ Failed
pytest ❌ Failed

Note: These failures don't necessarily block the PR but both authors and reviewers should check the results for unintentional breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Identifiers cannot begin with a keyword
2 participants