Skip to content

Replace panics with proper compile errors in jet_push_macros#55

Merged
tcrypt25519 merged 5 commits into
mainfrom
copilot/create-push-macro-crate
Feb 13, 2026
Merged

Replace panics with proper compile errors in jet_push_macros#55
tcrypt25519 merged 5 commits into
mainfrom
copilot/create-push-macro-crate

Conversation

Copilot AI commented Feb 13, 2026

Copy link
Copy Markdown

Plan: Replace panics with proper compile errors in jet_push_macros

  • Refactor generate_push_macros to use the shim + _inner pattern
  • Replace all panic!() calls with syn::Error::new_spanned()
  • Replace expect() calls with proper error handling using ?
  • Replace unwrap() on parse with proper error handling
  • Ensure all errors point to the specific problematic tokens
  • Test that compile errors work correctly
  • Verify existing tests still pass
  • Fix unused variable from review suggestion
  • Remove unnecessary clones

Changes Made

Fixed issues from code review:

  1. Removed unused start_expr variable that was left after applying the string-based to quote-based macro generation suggestion
  2. Changed *start.clone() and *end.clone() to start.as_ref() and end.as_ref() to avoid unnecessary clones
  3. Reverted to string-based macro generation since quote! doesn't work correctly for generating macro_rules! with dollar signs

The string-based approach is actually the correct one here because we need to generate literal $ tokens in the output macro_rules! definition, which quote! can't do directly.

All tests pass, clippy is clean, no warnings.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@semanticdiff-com

semanticdiff-com Bot commented Feb 13, 2026

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  crates/jet/tests/test_roms.rs  2% smaller
  Cargo.lock Unsupported file format
  Cargo.toml Unsupported file format
  crates/jet/Cargo.toml Unsupported file format
  crates/jet_push_macros/Cargo.toml Unsupported file format
  crates/jet_push_macros/src/lib.rs  0% smaller
  crates/jet_push_macros/tests/compile_fail/invalid_range.rs  0% smaller
  crates/jet_push_macros/tests/compile_fail/not_integer.rs  0% smaller
  crates/jet_push_macros/tests/compile_fail/not_range.rs  0% smaller
  crates/jet_push_macros/tests/compile_fail/open_range.rs  0% smaller
  crates/jet_push_macros/tests/macro_tests.rs  0% smaller

Copilot AI and others added 3 commits February 13, 2026 03:18
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Copilot AI changed the title [WIP] Add proc macro for generating push macros Add proc macro for compile-time generation of PUSH0..PUSH32 macros Feb 13, 2026
Copilot AI requested a review from tcrypt25519 February 13, 2026 03:27
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Copilot AI changed the title Add proc macro for compile-time generation of PUSH0..PUSH32 macros Replace panics with proper compile errors in jet_push_macros Feb 13, 2026
@tcrypt25519
tcrypt25519 marked this pull request as ready for review February 13, 2026 05:00
Copilot AI review requested due to automatic review settings February 13, 2026 05:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors error handling in the jet_push_macros procedural macro from panic-based failures to proper compile-time errors using syn::Error. The change improves developer experience by providing clear, token-specific error messages instead of cryptic internal compiler errors.

Changes:

  • Introduced new jet_push_macros crate with proper proc macro error handling using the shim + _inner pattern
  • Replaced manual PUSH macro definitions in test code with generated versions from the proc macro
  • Added comprehensive test coverage including functional tests and compile_fail examples

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
crates/jet_push_macros/Cargo.toml New proc macro crate configuration with syn, quote, and proc-macro2 dependencies
crates/jet_push_macros/src/lib.rs Core proc macro implementation using syn::Error for all error cases
crates/jet_push_macros/tests/macro_tests.rs Functional tests covering PUSH0-PUSH5 and PUSH32 macro generation
crates/jet_push_macros/tests/compile_fail/*.rs Error case documentation (not_integer, not_range, open_range, invalid_range)
crates/jet/tests/test_roms.rs Integration update replacing manual PUSH macros with generated versions
crates/jet/Cargo.toml Added jet_push_macros as dev dependency
Cargo.toml Added jet_push_macros to workspace members
Cargo.lock Dependency graph updates for new crate

Comment thread crates/jet_push_macros/src/lib.rs
Comment thread crates/jet_push_macros/src/lib.rs
Comment thread crates/jet_push_macros/src/lib.rs
Comment thread crates/jet_push_macros/Cargo.toml
@tcrypt25519

Copy link
Copy Markdown
Owner

@copilot Merge your suggestion, and you broke the build. You have an unused variable. I don't know if it's supposed to be unused and you just failed to remove it, or if you messed up even worse than that and removed some code that you shouldn't have that should be using it. Either way, you need to fix it.

Copilot AI commented Feb 13, 2026

Copy link
Copy Markdown
Author

@copilot Merge your suggestion, and you broke the build. You have an unused variable. I don't know if it's supposed to be unused and you just failed to remove it, or if you messed up even worse than t...

Fixed in commit 1c5e099. The unused start_expr variable has been removed. The quote!-based suggestion didn't work because we need to generate literal $ tokens in the macro_rules! output, which quote! can't do directly. Reverted to the string-based approach which is the correct solution for this use case, while keeping the fix to avoid unnecessary clones.

@tcrypt25519
tcrypt25519 force-pushed the copilot/create-push-macro-crate branch from 1c5e099 to 262e5e4 Compare February 13, 2026 06:43
@tcrypt25519
tcrypt25519 merged commit b83cbba into main Feb 13, 2026
8 of 9 checks passed
tcrypt25519 added a commit that referenced this pull request Feb 16, 2026
* Initial plan

* Add jet_push_macros crate and integrate with test_roms

Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>

* Add allow(unused_macros) and format code

Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>

* Fix Cargo.toml formatting

Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>

* Replace panics with proper compile errors in proc macro

Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
@tcrypt25519
tcrypt25519 deleted the copilot/create-push-macro-crate branch February 16, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants