Fix rotate operator serialization (#655)

[filipeom]

This issue was probably introduced by #629. Should have done a better job testing our parser at that point.

Closes #655

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e13f03c4-2414-429b-8b55-8e423058b4f6

📥 Commits

Reviewing files that changed from the base of the PR and between ea26463 and 8bce020.

📒 Files selected for processing (6)

• src/smtml/lexer.mll
• src/smtml/parser.mly
• src/smtml/ty.ml
• test/regression/dune
• test/regression/test_issue_655.ml
• test/regression/test_regression.ml

🚧 Files skipped from review as they are similar to previous changes (6)

• src/smtml/ty.ml
• test/regression/test_regression.ml
• test/regression/dune
• src/smtml/parser.mly
• test/regression/test_issue_655.ml
• src/smtml/lexer.mll

---

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Extended SMTML bitvector operator support to recognize additional popcnt, rotl/rotr, and ext_rotl/ext_rotr for both 32-bit (i32.*) and 64-bit (i64.*) expressions.

• Bug Fixes

  • Improved SMTML pretty-printing for rotl/rotr to ensure the generated syntax round-trips correctly.

• Tests

  • Added and wired in a regression test for serialize/parse round-tripping of a rotate + popcnt expression.

Walkthrough

The lexer recognizes additional 32-bit and 64-bit bitvector operators. The parser handles fake-binary rotation forms, the printer adjusts rotation formatting, and a regression test verifies print-and-reparse behavior.

Changes

Bitvector rotation parsing

Layer / File(s)	Summary
Operator tokenization and fake-binary parsing src/smtml/lexer.mll, src/smtml/parser.mly	Lexer keywords for popcnt, rotr, rotl, and extension operators are added for 32-bit and 64-bit bitvectors. Parser declares a FAKE_BINARY token and adds a paren_op production that consumes the token payload, rotation amount, and expression, then rewrites zero-rotation forms into unary operators.
Rotation expression printing src/smtml/ty.ml	Unop.pp formatting for Rotl and Rotr is updated to omit surrounding parentheses.
Round-trip regression coverage test/regression/dune, test/regression/test_issue_655.ml, test/regression/test_regression.ml	Dune test stanza is configured to run the new regression test module. A test case builds a 32-bit popcount and rotation expression, prints it with Expr.Printer.pp_expr, reparses it with Parse.Smtml.Expr.from_string, and asserts successful parsing. A test runner wires the test case into the Alcotest suite.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: rotate operator serialization for issue #655.
Description check	✅ Passed	The description is on-topic and matches the parser/serialization bug fix.
Linked Issues check	✅ Passed	The changes address #655 by fixing rotate serialization/parsing and adding a regression test.
Out of Scope Changes check	✅ Passed	The added lexer, parser, pretty-printing, and regression test changes all support the reported bug fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

test/regression/test_issue_655.ml (1)

3-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend regression coverage beyond one successful i32 rotation.

The test only exercises i32.rotr with a successful parse. Add cases for the new i64/rotl forms and at least one malformed or non-literal rotation operand to verify the parser returns an error rather than raising.

As per path instructions, tests must cover edge cases and error paths, not just happy paths.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/regression/test_issue_655.ml` around lines 3 - 22, Extend the regression
test around expr, parse, and reparse to cover i64 rotate-left and rotate-right
expressions in addition to the existing i32 case. Add at least one rotation
using a non-literal or malformed operand, and assert that parsing returns
Result.Error without raising; retain successful Result.Ok assertions for valid
cases.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/smtml/parser.mly`:
- Around line 93-105: Update the FAKE_BINARY rotation branch in the parser to
consume the numeric NUM operand directly rather than parsing an arbitrary sexpr
and inspecting Expr.view. Construct Rotr or Rotl from that token value, and
avoid Fmt.failwith for malformed non-integer input so Expr.from_string retains
its result-based parser error path; use assertions only for impossible token
payloads.

In `@test/regression/test_issue_655.ml`:
- Line 1: Add the mandated three-line SPDX and copyright header at the top of
the new OCaml file, before the existing comment containing the i32 expression,
following the repository’s standard header format.

---

Nitpick comments:
In `@test/regression/test_issue_655.ml`:
- Around line 3-22: Extend the regression test around expr, parse, and reparse
to cover i64 rotate-left and rotate-right expressions in addition to the
existing i32 case. Add at least one rotation using a non-literal or malformed
operand, and assert that parsing returns Result.Error without raising; retain
successful Result.Ok assertions for valid cases.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f0dadf0b-44de-4e06-ab50-1f074fac34c0

📥 Commits

Reviewing files that changed from the base of the PR and between a4f0a0b and 4ac7d54.

📒 Files selected for processing (5)

• src/smtml/lexer.mll
• src/smtml/parser.mly
• src/smtml/ty.ml
• test/regression/dune
• test/regression/test_issue_655.ml

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/regression/test_regression.ml`:
- Around line 1-5: Add a top-level OCaml module documentation comment using the
required `(** ... *)` syntax immediately after the license header and before the
`let ()` implementation in the module.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c1995a8-a7f2-4389-9a3a-e14bb0edd924

📥 Commits

Reviewing files that changed from the base of the PR and between 1511d61 and ea26463.

📒 Files selected for processing (6)

• src/smtml/lexer.mll
• src/smtml/parser.mly
• src/smtml/ty.ml
• test/regression/dune
• test/regression/test_issue_655.ml
• test/regression/test_regression.ml

🚧 Files skipped from review as they are similar to previous changes (4)

• src/smtml/ty.ml
• test/regression/test_issue_655.ml
• src/smtml/lexer.mll
• test/regression/dune