Skip to content

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 28, 2025

Description:

This PR fixes a regression of quote! macro caused by #11144

@kdy1 kdy1 added this to the Planned milestone Oct 28, 2025
@kdy1 kdy1 self-assigned this Oct 28, 2025
@kdy1 kdy1 requested review from a team as code owners October 28, 2025 05:13
@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2025

🦋 Changeset detected

Latest commit: 26688c0

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link
Contributor

claude bot commented Oct 28, 2025

Claude finished @kdy1's task —— View job


Pull Request Review

Reviewing the quote macro fix changes...

Todo List:

  • Analyze changed files and understand the fix
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Analysis Summary

This PR fixes a broken quote macro by adding a missing from_bytes_unchecked method to Wtf8Atom and simplifying the quote macro code generation.

Code Quality & Best Practices ✅

Positive aspects:

  • Good API design: The new from_bytes_unchecked method follows Rust safety conventions with proper unsafe marking and comprehensive documentation
  • Consistent patterns: The implementation mirrors existing patterns in the codebase (lines 91-95 in wtf8_atom.rs)
  • Proper documentation: The safety requirements are clearly documented with references to underlying implementation
  • Code simplification: The quote macro change reduces complexity from a nested from call to a direct method call

Follows repository guidelines:

Potential Issues & Security 🔍

Safety considerations:

  • The from_bytes_unchecked method is correctly marked unsafe with clear documentation of safety requirements
  • The safety contract is properly delegated to the underlying hstr::wtf8::Wtf8::from_bytes_unchecked
  • Good: The quote macro usage at crates/swc_ecma_quote_macros/src/ast/lit.rs:44 uses self.as_bytes() which ensures valid WTF-8 bytes are passed

No security concerns identified - this is a refactoring that improves the API surface.

Performance Considerations ⚡

Performance improvements:

  • The simplified quote macro generation eliminates one intermediate allocation/conversion step
  • Direct method call is more efficient than the previous nested approach
  • No performance regressions expected

Bugs & Edge Cases 🐛

No bugs identified. The changes are:

  1. API addition: New method with safe delegation to existing implementation
  2. Refactoring: Simplified quote generation using the new method

Edge case handling:

  • Both the old and new approaches handle the same edge cases since they use the same underlying WTF-8 implementation
  • The safety requirements remain identical

Test Coverage 📋

Current test coverage:

  • Wtf8Atom has existing tests for UTF-8 conversion scenarios (lines 225-251 in wtf8_atom.rs)
  • Tests cover both valid UTF-8 and invalid UTF-8 cases with to_atom_lossy

Missing test coverage:

  • ⚠️ No direct tests for from_bytes_unchecked - Consider adding tests to verify:
    • Valid WTF-8 byte sequences work correctly
    • The method integrates properly with existing functionality
  • ⚠️ No integration tests for the quote macro fix - Consider adding tests to verify the quote macro generates correct code for Wtf8Atom instances

Recommendations

  1. Add unit tests for the new from_bytes_unchecked method
  2. Add integration tests for the quote macro functionality with Wtf8Atom
  3. Consider running cargo fmt --all as specified in the repository guidelines

Overall Assessment ✅

This is a well-executed fix that:

  • Properly addresses the broken quote macro issue
  • Maintains safety invariants
  • Improves code clarity and performance
  • Follows repository conventions

The changes are minimal, focused, and safe. The main area for improvement is test coverage for the new functionality.

@kdy1 kdy1 enabled auto-merge (squash) October 28, 2025 05:15
@kdy1 kdy1 disabled auto-merge October 28, 2025 05:15
@kdy1 kdy1 changed the title chore(atoms): Fix broken quote macro fi(atoms): Fix broken quote macro Oct 28, 2025
@kdy1 kdy1 changed the title fi(atoms): Fix broken quote macro fix(atoms): Fix broken quote macro Oct 28, 2025
@kdy1 kdy1 enabled auto-merge (squash) October 28, 2025 05:15
@kdy1 kdy1 disabled auto-merge October 28, 2025 05:21
@kdy1 kdy1 merged commit 3485179 into main Oct 28, 2025
168 of 171 checks passed
@kdy1 kdy1 deleted the kdy1/quote branch October 28, 2025 05:21
@github-actions
Copy link
Contributor

Binary Sizes

File Size
swc.linux-x64-gnu.node 31M (32045024 bytes)

Commit: e9d5de8

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 28, 2025

CodSpeed Performance Report

Merging #11195 will not alter performance

Comparing kdy1/quote (26688c0) with main (85d855f)1

Summary

✅ 138 untouched
⏩ 13 skipped2

Footnotes

  1. No successful run was found on main (4d9633d) during the generation of this report, so 85d855f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 13 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@kdy1 kdy1 modified the milestones: Planned, v1.14.0 Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants