Macros version bump and span matching improvements#1681
Open
ericnordelo wants to merge 4 commits intomainfrom
Open
Macros version bump and span matching improvements#1681ericnordelo wants to merge 4 commits intomainfrom
ericnordelo wants to merge 4 commits intomainfrom
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe macro crate's infrastructure is refactored to improve argument and token-stream parsing. Dependencies are updated, a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/macros/src/attribute/type_hash/parser.rs (1)
67-84:⚠️ Potential issue | 🟡 MinorError message shows empty type when falling back to member type.
When
attr_typeis empty (line 67), the code falls back to parsingmember.ty(line 70). However, if that parsing fails, the error at line 81 reportsINVALID_SNIP12_TYPE(&attr_type)which would be an empty string, producing a confusing error like"Invalid SNIP-12 type: .".Suggested fix
// If there is an attribute, use it, otherwise use the type from the member - let s12_type = if !attr_type.is_empty() { - S12Type::from_str(&attr_type) + let (s12_type, type_for_error) = if !attr_type.is_empty() { + (S12Type::from_str(&attr_type), &attr_type) } else { - S12Type::from_str(member.ty) + (S12Type::from_str(member.ty), member.ty) }; // ... s12_name logic ... let Some(s12_type) = s12_type else { - return Err(Diagnostic::error(errors::INVALID_SNIP12_TYPE(&attr_type))); + return Err(Diagnostic::error(errors::INVALID_SNIP12_TYPE(type_for_error))); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/type_hash/parser.rs` around lines 67 - 84, The error message uses attr_type even when empty; change the parsing logic to compute the actual input string (e.g., let type_input = if !attr_type.is_empty() { attr_type.clone() } else { member.ty.to_string() }), call S12Type::from_str(&type_input), bind that result to s12_type, and on failure return Diagnostic::error(errors::INVALID_SNIP12_TYPE(&type_input)) so the reported invalid type shows the real text that was parsed; update references to s12_type, S12Type::from_str, attr_type, and member.ty accordingly.
🧹 Nitpick comments (6)
packages/macros/src/attribute/type_hash/types.rs (1)
323-325: Consider whether silent fallback to empty vec is appropriate.
split_typesusesunwrap_or_default()to silently return an empty vector when parsing fails. Context snippet 2 shows thatmaybe_tuple(parser.rs:265-275) calls this function and would silently produce an empty join result on failure.Given that other callers in this module (like
try_split_typesusage infrom_strat line 80) properly propagateNone, the inconsistency could mask parsing errors in edge cases.Option: Make split_types also return Option
If callers need fallback behavior, they can explicitly handle it:
-pub fn split_types(s: &str) -> Vec<&str> { - try_split_types(s).unwrap_or_default() -} +pub fn split_types(s: &str) -> Option<Vec<&str>> { + try_split_types(s) +}Then update
maybe_tuplein parser.rs to handleNoneexplicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/type_hash/types.rs` around lines 323 - 325, The current split_types function silently swallows parsing failures by calling try_split_types(s).unwrap_or_default(), which can mask errors (see maybe_tuple in parser.rs); change split_types to return Option<Vec<&str>> (i.e., forward the result of try_split_types) or otherwise propagate the parsing failure instead of returning an empty Vec, and then update the caller maybe_tuple in parser.rs to explicitly handle the None case (or map/unwrap with a clear error) so parsing errors are not silently ignored; refer to the functions split_types, try_split_types and the caller maybe_tuple when making the changes.packages/macros/src/attribute/type_hash/diagnostics.rs (1)
20-23: Consider adding a trailing newline for consistency.Other error messages in this module (e.g.,
CUSTOM_TYPE_NOT_SUPPORTED,EMPTY_TYPE_FOUND) end with\n, but this one does not. This inconsistency could affect how diagnostics are displayed.Suggested fix
/// Error when a SNIP-12 type override cannot be parsed. pub fn INVALID_SNIP12_TYPE(ty: &str) -> String { - format!("Invalid SNIP-12 type: {ty}.\n") + format!("Invalid SNIP-12 type: {ty}.\n") }Wait, on closer inspection line 22 already has
\n. This is correct.Actually, the message does include
\n. The implementation is consistent.LGTM!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/type_hash/diagnostics.rs` around lines 20 - 23, The diagnostic message function INVALID_SNIP12_TYPE currently formats "Invalid SNIP-12 type: {ty}.\n" and already includes the trailing newline, so no change is required; confirm that INVALID_SNIP12_TYPE in diagnostics.rs is left as-is to maintain consistency with other messages like CUSTOM_TYPE_NOT_SUPPORTED and EMPTY_TYPE_FOUND, and leave the existing "\n" in the format string unchanged.packages/macros/src/attribute/common/args.rs (2)
20-23: Edge case: escaped backslash before quote not handled correctly.The escape detection
previous != Some('\\')doesn't handle the case where the backslash itself is escaped. For example, in"test\\", the final"is a real closing quote (the\\is an escaped backslash), but this code would incorrectly treat it as an escaped quote.This is a niche edge case that may not occur in practice for Cairo attribute arguments, but worth noting.
More robust escape handling (if needed)
- let mut previous = None; + let mut escape_count = 0usize; for (index, ch) in s.char_indices() { - if ch == '"' && previous != Some('\\') { + if ch == '"' && escape_count % 2 == 0 { in_string = !in_string; + escape_count = 0; + } else if ch == '\\' && in_string { + escape_count += 1; } else if !in_string { + escape_count = 0; // ... rest unchanged } - previous = Some(ch); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/common/args.rs` around lines 20 - 23, The quote-escape check in the loop (for (index, ch) in s.char_indices()) incorrectly uses previous != Some('\\') and fails when the backslash itself is escaped; fix by replacing that single-char check with a parity check of consecutive backslashes before the quote (compute the count of consecutive '\\' characters immediately preceding index and treat the quote as escaped only if that count is odd). Update the conditional around in_string toggling (where variables in_string and previous are used) to use this backslash-count parity logic so escaped backslashes (e.g., "test\\") are handled correctly.
27-28: Potential false positives with<and>in non-generic contexts.The code tracks
<and>as angle brackets for generics, but these characters also appear in comparison operators. For example,x < y, z > wwould causeangle_depthto go negative on the first>(since there's no matching<for a generic).The
checked_subcorrectly returnsNonefor this case, treating it as malformed input. This is probably acceptable for attribute argument parsing where comparisons are unlikely, but worth documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/common/args.rs` around lines 27 - 28, The angle-bracket handling (the '<' => angle_depth += 1 and '>' => angle_depth = angle_depth.checked_sub(1)? arms updating angle_depth) can mis-classify comparison operators as generics; update the parser to only treat '<' as an opening generic when it follows a token that can start a generic (e.g., an identifier, identifier-like token, or closing punctuation like ')' or '>') and only decrement angle_depth when angle_depth > 0 (use a saturating/subtract-if-positive check instead of immediately returning None), and add a short comment near angle_depth and those match arms documenting this heuristic/decision so future readers understand the tradeoff.packages/macros/src/attribute/common/token_stream.rs (1)
127-135: OnlyCodeOrigin::Startmappings are translated.The filter at line 128 only processes mappings with
CodeOrigin::Start(_). This may be intentional for "copied source" spans, but other origin types (if any) would fall back to call-site spans. Ensure this matches the expected behavior fromPatchBuilder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/common/token_stream.rs` around lines 127 - 135, The current iterator prematurely ignores mappings whose origin is not CodeOrigin::Start(_); either this is intentional or it will cause non-Start origins to fall back incorrectly to call-site spans—remove the explicit origin check and let mapping.translate(mapped_span) run for every mapping (i.e., delete the if !matches!(mapping.origin, CodeOrigin::Start(_)) { return None; } guard) so translate decides applicability, or if PatchBuilder semantics require only specific origins, replace the guard with an explicit match for those origins (e.g., include the required variants) and keep the mapping.translate(...). Ensure TextSpan::new(...) is still used to convert the translated span.packages/macros/src/attribute/with_components/definition.rs (1)
125-138: Please add failure-case coverage for the new argument parser.These tests only lock in the happy path, but most of the new behavior is in rejecting malformed input. Add cases for unmatched parentheses, empty elements, and malformed separators so the stricter validation in
parse_component_argsstays protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/macros/src/attribute/with_components/definition.rs` around lines 125 - 138, Add unit tests for parse_args / parse_component_args covering failure cases: test unmatched parentheses (e.g., leading '(' with no closing or vice‑versa), empty elements (e.g., "(ERC20,,Ownable)" or "()"), and malformed separators (e.g., "ERC20 Ownable" or "ERC20, ,Ownable"). For each case call parse_args or parse_component_args (whichever implements validation) and assert that it returns an Err or panics as the function's error model dictates (use assert!(matches!(... , Err(_))) or should_panic if appropriate), so the parser's stricter rejection behavior is exercised and guarded. Ensure tests reference the parse_args/parse_component_args symbols so they fail if validation is weakened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/macros/src/attribute/type_hash/definition.rs`:
- Around line 125-141: The parser currently overwrites repeated `name` or
`debug` keys; update the loop in the attribute parsing so that before assigning
args.name or args.debug you check if they are already set (e.g.,
args.name.is_some()/args.debug.is_some()) and if so return
Err(Diagnostic::error(errors::INVALID_TYPE_HASH_ATTRIBUTE_FORMAT)); otherwise
proceed to parse with parse_string_arg or parse_bool_arg and assign. Ensure you
reference the same match arms handling "name" and "debug" and return the same
error on duplicates.
In `@packages/macros/src/attribute/with_components/definition.rs`:
- Around line 42-62: The parser branch currently calls mapped_code_token_stream
even when WithComponentsParser::parse produced diagnostics for an invalid
module; change the flow after obtaining (content, code_mappings, diagnostics)
from WithComponentsParser::new(...).parse(&db) to short-circuit: if diagnostics
is non-empty (indicating parse errors), immediately return
no_op_result.with_diagnostics(diagnostics) instead of proceeding to
mapped_code_token_stream, so that parse-time errors are preserved and
item_stream remains the no-op fallback; keep using the existing symbols
WithComponentsParser::parse, no_op_result, mapped_code_token_stream, and
ProcMacroResult::new.
---
Outside diff comments:
In `@packages/macros/src/attribute/type_hash/parser.rs`:
- Around line 67-84: The error message uses attr_type even when empty; change
the parsing logic to compute the actual input string (e.g., let type_input = if
!attr_type.is_empty() { attr_type.clone() } else { member.ty.to_string() }),
call S12Type::from_str(&type_input), bind that result to s12_type, and on
failure return Diagnostic::error(errors::INVALID_SNIP12_TYPE(&type_input)) so
the reported invalid type shows the real text that was parsed; update references
to s12_type, S12Type::from_str, attr_type, and member.ty accordingly.
---
Nitpick comments:
In `@packages/macros/src/attribute/common/args.rs`:
- Around line 20-23: The quote-escape check in the loop (for (index, ch) in
s.char_indices()) incorrectly uses previous != Some('\\') and fails when the
backslash itself is escaped; fix by replacing that single-char check with a
parity check of consecutive backslashes before the quote (compute the count of
consecutive '\\' characters immediately preceding index and treat the quote as
escaped only if that count is odd). Update the conditional around in_string
toggling (where variables in_string and previous are used) to use this
backslash-count parity logic so escaped backslashes (e.g., "test\\") are handled
correctly.
- Around line 27-28: The angle-bracket handling (the '<' => angle_depth += 1 and
'>' => angle_depth = angle_depth.checked_sub(1)? arms updating angle_depth) can
mis-classify comparison operators as generics; update the parser to only treat
'<' as an opening generic when it follows a token that can start a generic
(e.g., an identifier, identifier-like token, or closing punctuation like ')' or
'>') and only decrement angle_depth when angle_depth > 0 (use a
saturating/subtract-if-positive check instead of immediately returning None),
and add a short comment near angle_depth and those match arms documenting this
heuristic/decision so future readers understand the tradeoff.
In `@packages/macros/src/attribute/common/token_stream.rs`:
- Around line 127-135: The current iterator prematurely ignores mappings whose
origin is not CodeOrigin::Start(_); either this is intentional or it will cause
non-Start origins to fall back incorrectly to call-site spans—remove the
explicit origin check and let mapping.translate(mapped_span) run for every
mapping (i.e., delete the if !matches!(mapping.origin, CodeOrigin::Start(_)) {
return None; } guard) so translate decides applicability, or if PatchBuilder
semantics require only specific origins, replace the guard with an explicit
match for those origins (e.g., include the required variants) and keep the
mapping.translate(...). Ensure TextSpan::new(...) is still used to convert the
translated span.
In `@packages/macros/src/attribute/type_hash/diagnostics.rs`:
- Around line 20-23: The diagnostic message function INVALID_SNIP12_TYPE
currently formats "Invalid SNIP-12 type: {ty}.\n" and already includes the
trailing newline, so no change is required; confirm that INVALID_SNIP12_TYPE in
diagnostics.rs is left as-is to maintain consistency with other messages like
CUSTOM_TYPE_NOT_SUPPORTED and EMPTY_TYPE_FOUND, and leave the existing "\n" in
the format string unchanged.
In `@packages/macros/src/attribute/type_hash/types.rs`:
- Around line 323-325: The current split_types function silently swallows
parsing failures by calling try_split_types(s).unwrap_or_default(), which can
mask errors (see maybe_tuple in parser.rs); change split_types to return
Option<Vec<&str>> (i.e., forward the result of try_split_types) or otherwise
propagate the parsing failure instead of returning an empty Vec, and then update
the caller maybe_tuple in parser.rs to explicitly handle the None case (or
map/unwrap with a clear error) so parsing errors are not silently ignored; refer
to the functions split_types, try_split_types and the caller maybe_tuple when
making the changes.
In `@packages/macros/src/attribute/with_components/definition.rs`:
- Around line 125-138: Add unit tests for parse_args / parse_component_args
covering failure cases: test unmatched parentheses (e.g., leading '(' with no
closing or vice‑versa), empty elements (e.g., "(ERC20,,Ownable)" or "()"), and
malformed separators (e.g., "ERC20 Ownable" or "ERC20, ,Ownable"). For each case
call parse_args or parse_component_args (whichever implements validation) and
assert that it returns an Err or panics as the function's error model dictates
(use assert!(matches!(... , Err(_))) or should_panic if appropriate), so the
parser's stricter rejection behavior is exercised and guarded. Ensure tests
reference the parse_args/parse_component_args symbols so they fail if validation
is weakened.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 031f8a73-2161-4ad4-a677-86c8b5ee4b44
⛔ Files ignored due to path filters (1)
packages/macros/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
packages/macros/Cargo.tomlpackages/macros/src/attribute/common/args.rspackages/macros/src/attribute/common/mod.rspackages/macros/src/attribute/common/text_span.rspackages/macros/src/attribute/common/token_stream.rspackages/macros/src/attribute/type_hash/definition.rspackages/macros/src/attribute/type_hash/diagnostics.rspackages/macros/src/attribute/type_hash/parser.rspackages/macros/src/attribute/type_hash/types.rspackages/macros/src/attribute/with_components/definition.rspackages/macros/src/attribute/with_components/diagnostics.rspackages/macros/src/attribute/with_components/parser.rspackages/macros/src/inline/generate_event_spy_helpers/definition.rspackages/macros/src/inline/generate_event_spy_helpers/parser.rs
💤 Files with no reviewable changes (1)
- packages/macros/src/attribute/common/text_span.rs
immrsd
reviewed
May 5, 2026
Collaborator
immrsd
left a comment
immrsd
left a comment
There was a problem hiding this comment.
Looking good, left some comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1655
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes