deps: drop thiserror for a manual impl
#1120
Conversation
CosmicHorrorDev
requested review from
tomtau
and removed request for
a team
WalkthroughThe PR removes the thiserror dependency and replaces derives with explicit core::error::Error / std::error::Error and fmt::Display implementations gated by the Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| #[cfg(feature = "std")] | ||
| impl<R: RuleType> core::error::Error for Error<R> {} |
There was a problem hiding this comment.
oh awkward mix of std and core here, but worth asking. Would you be fine bumping the MSRV up to 1.81.0 for core::error::Error? It's only one version up and I believe debian's latest release is 1.84
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pest/src/error.rs (1)
752-754: Remove leftover thiserror derive from MietteAdapter and fix other thiserror usages.Leftover #[derive(thiserror::Error)] will break the build if the thiserror dependency is removed.
- Apply to pest/src/error.rs (lines ~752-754):
- #[derive(thiserror::Error, Debug)] + #[derive(Debug)] pub(crate) struct MietteAdapter<R: RuleType>(pub(crate) Error<R>);
- Also address other occurrences: debugger/src/lib.rs:82 has #[derive(Debug, thiserror::Error)] — remove the thiserror derive or keep the thiserror dependency (debugger/Cargo.toml:30:thiserror = "2") so the workspace stays buildable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pest/Cargo.toml(1 hunks)pest/src/error.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pest/src/error.rs (2)
pest/src/iterators/pair.rs (1)
line_col(290-293)pest/src/position.rs (2)
line_col(129-170)line_col(503-518)
🪛 GitHub Actions: pest Continuous Integration
pest/src/error.rs
[error] 752-752: cargo check failed: failed to resolve: use of undeclared crate or module thiserror (pest/src/error.rs:752).
[error] 780-780: cargo check failed: failed to resolve: use of undeclared crate or module fmt (pest/src/error.rs:780).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Fuzzing
🔇 Additional comments (2)
pest/Cargo.toml (1)
25-25: miette-error feature: removal of thiserror dep is correct.pest/src/error.rs (1)
65-67: LGTM: ErrorVariant implements std::error::Error under std.
CosmicHorrorDev
force-pushed
the
drop-thiserror
branch
from
161f21e to
34f62c1
Compare
CosmicHorrorDev
force-pushed
the
drop-thiserror
branch
from
34f62c1 to
d0e4230
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pest/src/error.rs (1)
45-46: Usestd::error::Errorinstead ofcore::error::Errorfor compatibility with MSRV.The current implementation uses
core::error::Errorwhich wasn't stabilized until Rust 1.81.0. Based on the existing comment thread, the maintainer hasn't confirmed whether they want to bump the MSRV. Until that decision is made, you should usestd::error::Errorto maintain compatibility with the current MSRV.Apply this diff to maintain MSRV compatibility:
#[cfg(feature = "std")] -impl<R: RuleType> core::error::Error for Error<R> {} +impl<R: RuleType> std::error::Error for Error<R> {}
🧹 Nitpick comments (1)
pest/src/error.rs (1)
743-743: Remove redundantuse core::fmt;import.There's already a
use core::fmt;statement at line 23 in the parent module scope. This import is redundant and can be removed.Apply this diff to remove the redundant import:
mod miette_adapter { - use core::fmt; use alloc::string::ToString;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pest/Cargo.toml(1 hunks)pest/src/error.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pest/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test Coverage
- GitHub Check: check for no_std compatibility
- GitHub Check: Minimal Versions Testing
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: check for semver compatibility
- GitHub Check: Documentation check
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: Fuzzing
🔇 Additional comments (5)
pest/src/error.rs (5)
65-66: LGTM! Correct use ofstd::error::ErrorforErrorVariant.The implementation correctly uses
std::error::Errorand is properly gated with thestdfeature flag.
776-778: LGTM! Correct return type forhelpmethod.The signature correctly returns
Box<dyn fmt::Display + 'a>which aligns with themiette::Diagnostictrait requirements.
781-785: LGTM! Proper manualDisplayimplementation.The manual implementation correctly formats the error message and properly uses the fully qualified
core::fmttypes.
787-789: LGTM! Correct manualErrorimplementation with proper trait bounds.The implementation correctly requires both
DebugandDisplaytraits as bounds, matching whatthiserrorwould have generated. The use ofstd::error::Erroris appropriate since this is within astdfeature-gated module.
753-754: LGTM! Clean removal ofthiserrorderive macros.The struct now only derives
Debug, which is correct. The previousthiserror::Errorderive and its#[error]attribute have been properly replaced with manual implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pest/src/error.rs (2)
744-744: fmt import fix looks good.
45-47: MSRV: implement std::error::Error (not core::error::Error).Rust 1.80 doesn’t have core::error::Error. Keep the existing std feature‑gate but switch to std::error::Error.
-#[cfg(feature = "std")] -impl<R: RuleType> core::error::Error for Error<R> {} +#[cfg(feature = "std")] +impl<R: RuleType> std::error::Error for Error<R> {}
🧹 Nitpick comments (1)
pest/src/error.rs (1)
65-67: Do we really want Error for ErrorVariant?Implementing std::error::Error for the variant enum expands the public trait surface and can surprise downstream users. If not required, consider dropping it to avoid unnecessary API commitments.
-#[cfg(feature = "std")] -impl<R: RuleType> std::error::Error for ErrorVariant<R> {} +// (consider removing this impl unless there is a concrete use case)Would you confirm if any downstream code relies on ErrorVariant: Error? If not, I recommend removing this impl.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pest/Cargo.toml(1 hunks)pest/src/error.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pest/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cargo hack check --feature-powerset
- GitHub Check: Unit, Style, and Lint Testing
- GitHub Check: Fuzzing
🔇 Additional comments (3)
pest/src/error.rs (3)
776-779: help() return type is correct and lifetime-safe.
781-785: Display message for MietteAdapter is clear and matches test expectations.
741-742: Resolved — miette adapter already gated on std via featureVerified pest/Cargo.toml (and vm/Cargo.toml) declare
miette-error = ["std", "pretty-print", "dep:miette"], so the module's use of Box andstd::error::Erroris safe and no cfg change is required.
tomtau
left a comment
tomtau
left a comment
There was a problem hiding this comment.
lgtm; I think it's ok to bump MSRV to 1.81 -- maybe in a separate PR?
|
I can handle that in another PR tomorrow 👍 |
2 participants
thiserrorlargely wasn't getting much use inpest, and it's a rather heavy dependency for an otherwise lightweight crate. The only other crate in the workspace that usesthiserrorisdebuggerwhich doesn't seem worth trying to removeSummary by CodeRabbit
New Features
Refactor
Chores
Performance