Move skip parsing#74
Conversation
…e unneeded fields
Summary by CodeRabbit
WalkthroughBumps crate versions from 0.4.4 to 0.4.5 across workspace crates. Moves field skip filtering from IDL conversion to parsing stage, removing StructField::is_skipped, adding a private helper for #[skip] detection, and updating a test fixture to include skipped and renamed fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UserCode as User code (struct with fields)
participant Macro as shank-macro-impl (parser)
participant IDL as shank-idl (IDL conversion)
rect rgb(238,245,255)
note over Macro: New behavior (0.4.5)
UserCode->>Macro: Parse ItemStruct
Macro->>Macro: Filter fields with #[skip]
Macro->>Macro: Build StructField(s) (no is_skipped)
end
Macro->>IDL: ParsedStruct (only non-skipped fields)
IDL->>IDL: Convert to IdlTypeDefinition (no skip filtering)
IDL-->>UserCode: IDL output
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
danenbm
left a comment
danenbm
left a comment
There was a problem hiding this comment.
Thank you LGTM makes more sense to throw away before parsing since that's a place we'd want to use it for sure!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
shank-cli/Cargo.toml(2 hunks)shank-idl/Cargo.toml(2 hunks)shank-idl/src/idl_type_definition.rs(0 hunks)shank-idl/tests/fixtures/accounts/single_file/field_attributes.rs(1 hunks)shank-macro-impl/Cargo.toml(1 hunks)shank-macro-impl/src/parsed_struct/parsed_struct.rs(2 hunks)shank-macro/Cargo.toml(2 hunks)shank-render/Cargo.toml(2 hunks)shank/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- shank-idl/src/idl_type_definition.rs
🔇 Additional comments (12)
shank-macro-impl/src/parsed_struct/parsed_struct.rs (1)
116-118: Moving skip filtering into parsing is the right call; add a test to cover “un-IDL-able” skipped types.Early filtering prevents
RustTypeconversion from choking on intentionally skipped fields (e.g., trait objects or complex generics). This aligns with the PR goal and reduces false negatives during parsing.Two follow-ups:
- Add/expand tests to assert that fields like
Box<dyn Any + Send + Sync>behind#[skip]do not cause parse errors.- Document that attributes on a skipped field (e.g.,
#[idl_name],#[idl_type]) are ignored, to avoid user confusion.shank-idl/tests/fixtures/accounts/single_file/field_attributes.rs (2)
7-8: IDL rename fixture looks good; ensure the generated IDL key matches “customName”.Great coverage for
#[idl_name]. Ensure the corresponding test asserts that the output IDL containscustomNameand notrenamed_field.
28-28: Wrapper type presence is helpful for#[idl_type]scenarios.Having
SomeWrapper<T>in the fixture ensures we continue to exercise theidl_typeoverride path alongside the new skip behavior.shank-render/Cargo.toml (2)
3-3: Version bump is aligned; adjust if you decide to honor SemVer with a minor bump.If
shank-macro-impl’s public API removal stands, consider bumping all workspace crates to 0.5.0 to reflect the breaking change; otherwise, re-introduce/deprecate the method and keep 0.4.5.
13-13: Dependency alignment onshank_macro_impl = 0.4.5looks consistent.Once the SemVer decision is finalized, ensure this dependency tracks the chosen version (0.4.5 with deprecation or 0.5.0 with a true break).
shank-cli/Cargo.toml (2)
7-7: CLI crate version bump is fine; keep it consistent with workspace SemVer decision.If you move to 0.5.0 for a breaking change, update this crate as well. Otherwise, 0.4.5 is consistent.
20-20: All internal crate versions are aligned to 0.4.5Verified across all Cargo.toml files using the provided ripgrep checks:
- Every
version = "0.4.x"entry is now"0.4.5"- Every
shank_*internal dependency references version"0.4.5"No remaining 0.4.4 references were found.
shank-macro/Cargo.toml (1)
3-3: Version bump to 0.4.5 looks good and aligns with the PR scope.shank-idl/Cargo.toml (2)
3-3: Patch version bump acknowledged.
16-16: Dependency versions are synchronizedAll shank crates consistently use version 0.4.5 and reference each other via
path + version. Ensure you publish in the following order to satisfy these dependencies:
- shank-macro-impl (v0.4.5)
- shank-render (v0.4.5)
- shank-macro (v0.4.5)
- shank-idl (v0.4.5)
- shank-cli (v0.4.5)
If
shank_macro_implintroduced any breaking changes in this release, increment its major version (e.g. to 0.5.0) and update dependent crates accordingly before publishing.Nitpick: consider adopting a workspace-managed versioning workflow or a release tool to centralize and automate these version bumps.
shank/Cargo.toml (2)
7-7: Crate version bump to 0.4.5 is consistent with the workspace.
12-12: All internal shank crates updated to v0.4.5; no stale 0.4.4 references remain
- Confirm publishing order: publish shank_macro 0.4.5 before the main shank crate to prevent transient resolution failures.
- Review any API changes introduced by the new skip-handling logic; if there are breaking changes, consider a major version bump (to 0.5.0) for this crate.
| #[skip] | ||
| pub slice_field: &[u8], | ||
|
|
||
| #[skip] | ||
| pub complex_skipped_field: Box<dyn std::any::Any + Send + Sync>, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Excellent additions to validate early-skip semantics.
Adding &[u8] and a Box<dyn Any + Send + Sync> behind #[skip] is precisely the kind of “un-IDL-able” type that benefits from early filtering.
Consider also adding one negative case (same fields without #[skip]) in a separate fixture to assert that parsing would fail as expected without the skip.
| [package] | ||
| name = "shank_macro_impl" | ||
| version = "0.4.4" | ||
| version = "0.4.5" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SemVer risk: public API removal elsewhere warrants a minor bump (0.5.0) or a deprecation path, not a patch bump.
Per the PR/AI summary, StructField::is_skipped (a public method) was removed in shank-macro-impl. Removing a public item is a breaking change. For 0.y.z crates, Cargo treats y as the “major”; breaking changes should bump 0.4.x → 0.5.0. Either:
- Re-introduce the method and deprecate it (returning a constant), or
- Bump all workspace crates to 0.5.0 and align internal dependency versions.
Run to confirm no remaining references and assess whether a deprecation path is preferable:
🏁 Script executed:
#!/bin/bash
# 1) Check for any usage of the removed method (should be empty)
rg -nP '\.is_skipped\s*\(' --type=rust -C2 || true
# 2) Check whether a `Skip` variant still exists and is now dead code
rg -nP '\benum\s+StructFieldAttr\b' --type=rust -C3 || true
rg -nP '\bStructFieldAttr::Skip\b' --type=rust -C2 || true
# 3) Quick sanity: verify internal crates are aligned on version 0.4.5 (or update all to 0.5.0 if you decide to bump)
fd Cargo.toml | xargs -I{} sh -c 'echo "== {} =="; rg -nP "^(version\s*=\s*|.*\{\s*version\s*=\s*)\"0\.(4|5)\.[0-9]+\"" "{}"'Length of output: 2662
Bump to 0.5.0 or provide a deprecation for is_skipped to uphold SemVer
Verification summary:
- No internal references to the removed
StructField::is_skippedmethod were found, confirming its removal does not break the workspace’s own code. - The
StructFieldAttr::Skipvariant remains in use for attribute parsing. - Every workspace crate is currently at version 0.4.5, and all path-dependencies point to 0.4.5.【run_scripts】
Action required: choose one of the following to address the breaking change
• Option A – Minor-version bump (0.5.0)
- Update the
version = "0.4.5"line in every Cargo.toml under the workspace (e.g. root, shank, shank-cli, shank-idl, shank-macro, shank-macro-impl, shank-render) toversion = "0.5.0". - Update all internal path-dependency version references from
"0.4.5"to"0.5.0".
• Option B – Deprecation path (patch bump to 0.4.6)
- Reintroduce
StructField::is_skippedas a public method, e.g.:impl StructField { #[deprecated(note = "Use `attrs().any(|a| a == StructFieldAttr::Skip)` instead")] pub fn is_skipped(&self) -> bool { self.attrs().contains(&StructFieldAttr::Skip) } }
- Release as version 0.4.6 to maintain backward compatibility while warning downstream users.
Either approach is critical to prevent accidental API breaks for consumers. Please implement your chosen path and synchronize all workspace crate versions accordingly.
🤖 Prompt for AI Agents
In shank-macro-impl/Cargo.toml around line 3, the crate version is still "0.4.5"
but a breaking removal of StructField::is_skipped requires a minor version bump;
update version = "0.4.5" to version = "0.5.0" in every Cargo.toml across the
workspace (root and all crates: shank, shank-cli, shank-idl, shank-macro,
shank-macro-impl, shank-render) and update all internal path-dependency version
references from "0.4.5" to "0.5.0" so workspace versions remain consistent.
| /// Helper function to check if a field has the skip attribute | ||
| fn field_has_skip_attr(field: &Field) -> bool { | ||
| field.attrs.iter().any(|attr| attr.path.is_ident("skip")) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make skip detection resilient and future-proof (namespaced attrs, meta forms).
Current check only matches #[skip] exactly. If we ever pivot to namespaced forms like #[shank(skip)], this will miss it. Optional refinement below preserves current behavior and adds resilience.
Apply within this helper:
-fn field_has_skip_attr(field: &Field) -> bool {
- field.attrs.iter().any(|attr| attr.path.is_ident("skip"))
-}
+fn field_has_skip_attr(field: &Field) -> bool {
+ use syn::{Meta, NestedMeta};
+ field.attrs.iter().any(|attr| {
+ // Fast path: #[skip]
+ if attr.path.is_ident("skip") {
+ return true;
+ }
+ // Optional: handle #[shank(skip)] and other meta forms gracefully
+ attr.parse_meta().ok().map_or(false, |meta| match meta {
+ Meta::Path(path) => path.is_ident("skip"),
+ Meta::List(list) => {
+ // e.g. #[shank(skip)]
+ list.nested.iter().any(|nm| match nm {
+ NestedMeta::Meta(Meta::Path(p)) => p.is_ident("skip"),
+ _ => false,
+ })
+ }
+ Meta::NameValue(nv) => nv.path.is_ident("skip"),
+ })
+ })
+}Note: This keeps compatibility with syn 1.x APIs we’re already using.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Helper function to check if a field has the skip attribute | |
| fn field_has_skip_attr(field: &Field) -> bool { | |
| field.attrs.iter().any(|attr| attr.path.is_ident("skip")) | |
| } | |
| /// Helper function to check if a field has the skip attribute | |
| fn field_has_skip_attr(field: &Field) -> bool { | |
| use syn::{Meta, NestedMeta}; | |
| field.attrs.iter().any(|attr| { | |
| // Fast path: #[skip] | |
| if attr.path.is_ident("skip") { | |
| return true; | |
| } | |
| // Optional: handle #[shank(skip)] and other meta forms gracefully | |
| attr.parse_meta().ok().map_or(false, |meta| match meta { | |
| Meta::Path(path) => path.is_ident("skip"), | |
| Meta::List(list) => { | |
| // e.g. #[shank(skip)] | |
| list.nested.iter().any(|nm| match nm { | |
| NestedMeta::Meta(Meta::Path(p)) => p.is_ident("skip"), | |
| _ => false, | |
| }) | |
| } | |
| Meta::NameValue(nv) => nv.path.is_ident("skip"), | |
| }) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In shank-macro-impl/src/parsed_struct/parsed_struct.rs around lines 103 to 106,
the helper currently only matches bare #[skip] and will miss namespaced or
nested forms (e.g. #[shank(skip)] or other meta shapes); update the function to
iterate attrs and return true if any attr.path.is_ident("skip") OR if parsing
the attr.meta yields a Meta::List whose path is a namespace ident like "shank"
(or any namespace you expect) and whose nested items contain a Meta::Path with
ident "skip" (also consider Meta::Path at top level); implement this using
attr.parse_meta(), match on Meta::Path and Meta::List, and check
nested.Meta::Path idents for "skip" so both #[skip] and #[shank(skip)] (and
similar meta forms) are detected while keeping syn 1.x APIs.
| shank_macro_impl = { version = "0.4.5", path = "../shank-macro-impl" } | ||
| shank_render = { version = "0.4.5", path = "../shank-render" } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm semver: if a public API was removed in shank_macro_impl, a 0.5.0 bump may be required.
PR context mentions removing a public is_skipped API in shank_macro_impl. If that function (or any item) was truly public to downstream crates, a breaking change under 0.y.z requires bumping the y component (i.e., 0.5.0), not just a patch to 0.4.5. Please verify exposure and external usage; if public, consider bumping all affected crates accordingly.
Run the following to sanity-check exposure and references:
If this is indeed a breaking change, apply a coordinated 0.5.0 bump across the workspace.
- version = "0.4.5"
+ version = "0.5.0"
- shank_macro_impl = { version = "0.4.5", path = "../shank-macro-impl" }
- shank_render = { version = "0.4.5", path = "../shank-render" }
+ shank_macro_impl = { version = "0.5.0", path = "../shank-macro-impl" }
+ shank_render = { version = "0.5.0", path = "../shank-render" }🏁 Script executed:
#!/bin/bash
# 1) Was `is_skipped` public and removed?
rg -nP -C2 'pub\s+fn\s+is_skipped\s*\(' shank-macro-impl -g '!**/target/**' || echo "No public is_skipped() found (current tree)."
# 2) Any crates in the repo referencing it?
rg -nP '\bis_skipped\s*\(' -g '!**/target/**'
# 3) Any 0.4.4 leftovers across manifests?
rg -nP 'version\s*=\s*"0\.4\.4"|{[^}]*version\s*=\s*"0\.4\.4"' -g '**/Cargo.toml' -nLength of output: 311
Semver bump required due to removed public is_skipped API
We’ve confirmed that the pub fn is_skipped(...) symbol no longer exists in shank-macro-impl and there are no in-repo references to it, which indicates that this was indeed removed. Removing a previously public API is a breaking change under semver, so you must bump the minor version (0.y.z → 0.5.0) for all affected crates.
Please apply the following coordinated version updates:
- In shank-macro/Cargo.toml (root of the workspace), bump dependencies:
- shank_macro_impl = { version = "0.4.5", path = "../shank-macro-impl" } - shank_render = { version = "0.4.5", path = "../shank-render" } + shank_macro_impl = { version = "0.5.0", path = "../shank-macro-impl" } + shank_render = { version = "0.5.0", path = "../shank-render" }
- In shank-macro-impl/Cargo.toml, update the package version field from
0.4.5to0.5.0. - In shank-render/Cargo.toml, update the package version field from
0.4.5to0.5.0.
This ensures downstream consumers see the correct minor-version bump for the breaking change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shank_macro_impl = { version = "0.4.5", path = "../shank-macro-impl" } | |
| shank_render = { version = "0.4.5", path = "../shank-render" } | |
| shank_macro_impl = { version = "0.5.0", path = "../shank-macro-impl" } | |
| shank_render = { version = "0.5.0", path = "../shank-render" } |
🤖 Prompt for AI Agents
In shank-macro/Cargo.toml around lines 17-18, the dependencies still reference
version 0.4.5 while shank-macro-impl removed a public API and requires a semver
minor bump; update the dependency entries here to use version "0.5.0" and then
also open shank-macro-impl/Cargo.toml and shank-render/Cargo.toml and change
their package version fields from "0.4.5" to "0.5.0" so all workspace crates
reflect the coordinated minor-version bump.
2 participants
Moving skip parsing to output check to parsing check so we don't parse unneeded fields