Update thiserror to version 2
#1055
Conversation
MarkusPettersson98
requested review from
NoahTheDuke
and removed request for
a team
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
debugger/Cargo.toml (2)
30-30: Consider updating CHANGELOG and documentationSince this is a major version upgrade of a core error handling dependency, please ensure that:
- The CHANGELOG is updated to reflect this breaking change
- Any error-related documentation is reviewed and updated if necessary
30-30: Consider adding miette integrationSince the main
pestcrate addedmiettealongside this upgrade, consider ifpest_debuggerwould benefit frommietteintegration for consistent error reporting across the workspace.pest/Cargo.toml (1)
31-33: Consider documenting the error handling strategyWith the introduction of miette alongside thiserror v2, it would be valuable to:
- Document the intended error handling strategy
- Provide migration guidelines for downstream crates
- Add examples showing the preferred error handling patterns
Would you like me to help draft documentation for the new error handling approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
debugger/Cargo.toml(1 hunks)pest/Cargo.toml(1 hunks)
🔇 Additional comments (4)
debugger/Cargo.toml (1)
30-30: Verify workspace-wide consistency of thiserror v2 upgrade
The upgrade to thiserror = "2" is a major version bump that could introduce breaking changes.
Let's verify the consistency across the workspace and check for potential breaking changes:
✅ Verification successful
Upgrade to thiserror v2 is consistent across workspace and error types look compatible
The verification shows:
- All instances of thiserror in the workspace are consistently using version 2
- Error types in both pest and debugger crates are using standard thiserror derive macros that are compatible with v2
- The minimum Rust version (1.61) is maintained consistently across the workspace and meets thiserror v2 requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify thiserror version consistency across workspace and check for potential breaking changes
# Check thiserror versions across all Cargo.toml files
echo "Checking thiserror versions across workspace:"
fd Cargo.toml --type f --exec rg -A 1 'thiserror.*='
# Look for potential error type definitions that might need updates
echo -e "\nChecking for error type definitions that might need updates:"
rg -t rust '#\[derive\(.*Error.*\)' -A 5
# Check minimum rust version requirements
echo -e "\nChecking rust-version fields:"
fd Cargo.toml --type f --exec rg 'rust-version.*='
Length of output: 1527
pest/Cargo.toml (3)
33-33: Verify miette configuration and necessity
The miette dependency is added twice with identical configurations:
- As an optional regular dependency
- As a dev dependency
Consider:
- Whether both entries are necessary
- If the fancy feature is required in both contexts
Also applies to: 34-34
✅ Verification successful
Miette configuration is appropriate but can be simplified
The dual miette entries serve different purposes:
- The optional dependency is correctly used with the
miette-errorfeature for production code - The dev dependency is needed for testing the miette integration (verified in
src/error.rstests)
However, since both entries use identical configuration, they can be consolidated by moving the dev dependency under the optional one with dev = true:
miette = { version = "7.2.0", optional = true, features = ["fancy"], dev = true }The "fancy" feature is appropriately used as it's needed for the error formatting demonstrated in:
examples/parens.rsfor pretty error outputsrc/error.rstest assertions that verify the formatted error output
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify miette usage and latest version
# Check latest version
cargo search miette --limit 1
# Look for miette usage in tests vs main code
rg "use.*miette" --type rust
# Check if fancy feature is used
rg "miette.*fancy|fancy.*miette" --type rust
Length of output: 278
Script:
#!/bin/bash
# Let's verify the miette usage more thoroughly
# Check where miette is used in the codebase
rg "miette" --type rust -A 5
# Look for test files using miette
fd "test.*\.rs$" --exec rg "miette" {}
# Check Cargo.toml for miette entries
rg "miette" pest/Cargo.toml
Length of output: 3248
Line range hint 15-24: Review feature flag configuration
The feature flags look well-structured, but let's verify:
- The necessity of bundling std, pretty-print, and thiserror in miette-error feature
- Whether these feature combinations are tested
✅ Verification successful
Feature flag dependencies are correctly configured
The feature flag configuration is well-structured and the dependencies make sense:
-
The
miette-errorfeature correctly depends on:stdsince miette requires standard library featurespretty-printfor error formatting capabilitiesthiserrorwhich is consistent with error handling needsmietteas the core dependency
-
The feature is properly used in the codebase:
- Properly gated in
pest/src/error.rswith#[cfg(feature = "miette-error")] - Has example usage in
pest/examples/parens.rs - Imports and uses miette's core functionality (
Diagnostic,LabeledSpan,SourceCode)
- Properly gated in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify feature flag usage and testing
# Look for feature flag testing
rg --type rust "cfg\(feature.*miette"
# Check if all feature combinations are tested
fd Cargo.toml | xargs rg "features.*test"
Length of output: 378
Script:
#!/bin/bash
# Let's verify the feature dependencies and their usage
# Check the miette-error feature definition in Cargo.toml
rg "miette-error.*=" -A 5 pest/Cargo.toml
# Check where miette is used in the codebase
rg "use.*miette" --type rust
# Look for feature-gated miette imports
rg "#\[cfg\(feature.*miette.*\)\].*use" --type rust -A 1
# Check test files for feature combinations
fd "test.*\.rs$" | xargs rg "cfg\(feature"
Length of output: 4928
31-31: Verify ecosystem compatibility with thiserror v2
The major version upgrade of thiserror could have breaking changes. Let's verify the ecosystem readiness and potential impacts.
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response:
thiserror v2 upgrade is safe to proceed
The upgrade from thiserror v1 to v2 is safe because:
- The crate is used correctly with the
#[cfg_attr(feature = "std", derive(thiserror::Error))]pattern - Error implementations are straightforward and follow thiserror's standard patterns
- The dependency is optional and only used when the "std" feature is enabled
- Both pest and pest_debugger crates are already using thiserror v2 consistently
- The error types are well-encapsulated and not exposed in public APIs that could affect downstream users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check thiserror v2 adoption and compatibility
# Check latest version and release info
cargo search thiserror --limit 1
# Check reverse dependencies to assess ecosystem impact
cargo search --limit 50 "^thiserror" | grep "thiserror.*v2"
# Look for any potential upgrade issues in the codebase
rg -l 'Error|error' | xargs rg 'derive\(Error\)|impl Error'
Length of output: 350
Script:
#!/bin/bash
# Let's try a different approach to analyze error implementations and thiserror usage
# Find files that use thiserror
rg -l "thiserror"
# Look for Error trait implementations and derives
rg -l "derive\(Error\)"
# Check Cargo.toml files for thiserror dependencies
fd "Cargo.toml" --exec grep -l "thiserror"
# Look at actual error implementations
rg "#\[derive\(Error\)\]" -A 3 -B 1
Length of output: 236
Script:
#!/bin/bash
# Let's examine the actual error implementations and thiserror usage
# Check error.rs content to understand error types
cat pest/src/error.rs
# Look at debugger lib.rs for error usage
cat debugger/src/lib.rs
# Check both Cargo.toml files to understand dependency relationships
cat pest/Cargo.toml
cat debugger/Cargo.toml
Length of output: 55264
There was a problem hiding this comment.
it seems this very slightly changed the error formatting in the miette output: https://github.com/pest-parser/pest/actions/runs/12090563797/job/33720850291?pr=1055#step:6:286
could you adjust the expected output?
|
Looking into it! |
Head branch was pushed to by a user without write access
MarkusPettersson98
force-pushed
the
bump-thiserror
branch
from
8844bf4 to
d8d5da7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pest/src/error.rs (3)
Line range hint
281-285: Consider changingwith_pathto take&mut selfinstead of consumingself.The
with_pathmethod currently takes ownership ofself(mut self), which consumes theErrorinstance. This may not be necessary and could prevent callers from reusing theErrorinstance after setting the path. Consider changing the method signature to take a mutable reference&mut selfto modify the instance in place without consuming it.Apply this diff to adjust the method:
-pub fn with_path(mut self, path: &str) -> Error<R> { +pub fn with_path(&mut self, path: &str) { self.path = Some(path.to_owned()); - self }This change allows the method to modify the existing
Errorinstance without consuming it, enhancing usability and aligning with Rust conventions for builder patterns when ownership transfer is not needed.
Line range hint
382-399: Move documentation comments outside the method body inErrorVariant::message.The documentation comments for the
messagemethod are placed inside the method body. In Rust, doc comments (///) should be placed directly above the method signature to ensure proper association with the method and correct generation of external documentation.Apply this diff to correct the documentation placement:
+ /// Returns the error message for [`ErrorVariant`]. + /// + /// If [`ErrorVariant`] is [`CustomError`], it returns a + /// [`Cow::Borrowed`] reference to the `message`. If [`ErrorVariant`] is [`ParsingError`], a + /// [`Cow::Owned`] message is constructed indicating expected and unexpected tokens. + /// + /// [`ErrorVariant`]: enum.ErrorVariant.html + /// [`CustomError`]: enum.ErrorVariant.html#variant.CustomError + /// [`ParsingError`]: enum.ErrorVariant.html#variant.ParsingError + /// [`Cow::Owned`]: https://doc.rust-lang.org/std/borrow/enum.Cow.html#variant.Owned + /// [`Cow::Borrowed`]: https://doc.rust-lang.org/std/borrow/enum.Cow.html#variant.Borrowed + /// pub fn message(&self) -> Cow<'_, str> { - /// - /// Returns the error message for [`ErrorVariant`] - /// - /// If [`ErrorVariant`] is [`CustomError`], it returns a - /// [`Cow::Borrowed`] reference to [`message`]. If [`ErrorVariant`] is [`ParsingError`], a - /// [`Cow::Owned`] containing "expected [ErrorVariant::ParsingError::positives] [ErrorVariant::ParsingError::negatives]" is returned. - /// - /// [`ErrorVariant`]: enum.ErrorVariant.html - /// [`CustomError`]: enum.ErrorVariant.html#variant.CustomError - /// [`ParsingError`]: enum.ErrorVariant.html#variant.ParsingError - /// [`Cow::Owned`]: https://doc.rust-lang.org/std/borrow/enum.Cow.html#variant.Owned - /// [`Cow::Borrowed`]: https://doc.rust-lang.org/std/borrow/enum.Cow.html#variant.Borrowed - /// [`message`]: enum.ErrorVariant.html#variant.CustomError.field.message - /// # Examples - /// - /// ``` - /// # use pest::error::ErrorVariant; - /// let variant = ErrorVariant::<()>::CustomError { - /// message: String::from("unexpected error") - /// }; - /// - /// println!("{}", variant.message()); + // Method implementation... match self {This adjustment ensures that doc comments are correctly associated with the
messagemethod and will be included in generated documentation.
Line range hint
1173-1185: Fix the syntax error in themiette_errortest function.There's a syntax error in the
assert_eq!macro within themiette_errortest function. The format string and the variables are improperly specified, leading to a compilation error.Apply this diff to correct the syntax:
format!("{:?}", miette_error), - [ - "", - " \u{1b}[31m×\u{1b}[0m Failure to parse at {:?}", self.0.line_col), - " ╭────", - " \u{1b}[2m1\u{1b}[0m │ def", - " · \u{1b}[35;1m┬\u{1b}[0m", - " · \u{1b}[35;1m╰── \u{1b}[35;1munexpected 4, 5, or 6; expected 1, 2, or 3\u{1b}[0m\u{1b}[0m", - " ╰────", - "\u{1b}[36m help: \u{1b}[0munexpected 4, 5, or 6; expected 1, 2, or 3\n" - ] - .join("\n") + format!( + "{}\n \u{1b}[31m×\u{1b}[0m Failure to parse at {:?}\n ╭────\n \u{1b}[2m1\u{1b}[0m │ def\n · \u{1b}[35;1m┬\u{1b}[0m\n · \u{1b}[35;1m╰── \u{1b}[35;1munexpected 4, 5, or 6; expected 1, 2, or 3\u{1b}[0m\u{1b}[0m\n ╰────\n\u{1b}[36m help: \u{1b}[0munexpected 4, 5, or 6; expected 1, 2, or 3\n", + "", + error.line_col() + ),This modification ensures that the format string is correctly constructed and the variable
error.line_col()is properly interpolated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
debugger/Cargo.toml(1 hunks)pest/Cargo.toml(1 hunks)pest/src/error.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- debugger/Cargo.toml
- pest/Cargo.toml
This PR simply upgrades
thiserrorto the latest major release. My motivation for doing this is to help the ecosystem move towards this new major version since it is a very nice release and a lot of crates depend on it 😊Summary by CodeRabbit
New Features
miette, enhancing error reporting capabilities.Errorstruct for setting a file path, improving error context.ErrorVariantstruct with a method to generate detailed error messages.Bug Fixes
thiserrordependency to a newer version, potentially improving error handling and stability.