Improve ConversionError messages with token details #41

Copilot · 2025-12-25T17:54:06Z

Original prompt

The error message currently includes which AST (struct|enum) is currently being parsed, but does not say which Pest token we are currently at.

"Extraneous tokens" should tell me which tokens are bonus

"No Match" should tell me what token is present.

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

coderabbitai · 2025-12-25T17:54:12Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

- Add NoMatchWithInfo variant with current_node, expected, and actual fields - Update Extraneous variant to include extraneous tokens description - Add is_no_match() helper method - Update Display implementations with detailed error messages - Update derive macro to pass token information to errors - Add tests for improved error messages Co-authored-by: tomtau <[email protected]>

Address code review feedback to use the is_no_match() helper method instead of pattern matching both NoMatch variants explicitly. Co-authored-by: tomtau <[email protected]>

tomtau · 2025-12-26T01:24:31Z

@copilot Can you fix cargo clippy errors?

Add explicit lifetime parameter `<'_>` to Span type to fix mismatched_lifetime_syntaxes clippy warnings. Co-authored-by: tomtau <[email protected]>

Copilot · 2025-12-26T01:28:28Z

@copilot Can you fix cargo clippy errors?

Fixed in 0cb8ecd. Added explicit lifetime parameter <'_> to Span type in examples/csv.rs (which also fixes tests/csv.rs since it's a symlink) to resolve the mismatched_lifetime_syntaxes clippy warning.

Copilot

Pull request overview

This PR enhances error messages in the pest AST conversion framework by providing more detailed information about parsing failures. Instead of generic "no match" and "extraneous tokens" errors, the new error variants include specific details about which tokens were expected versus found, making debugging significantly easier.

Key Changes:

Added NoMatchWithInfo variant to ConversionError that reports the current AST node, expected rule, and actual rule found
Enhanced Extraneous variant to include details about which specific tokens are extraneous
Added is_no_match() helper method to check for both NoMatch and NoMatchWithInfo variants

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`src/lib.rs`	Added `NoMatchWithInfo` and enhanced `Extraneous` variants; removed `Copy` trait; added `is_no_match()` helper; updated Display implementation; updated `Option<T>` and `Vec<T>` implementations to use the helper
`derive/src/from_pest/mod.rs`	Updated struct and enum derivation to generate `NoMatchWithInfo` and enhanced `Extraneous` errors with token details
`derive/src/from_pest/field.rs`	Updated default field strategy to use `is_no_match()` helper for consistency
`tests/error_messages.rs`	Added comprehensive tests for new error message formats and `is_no_match()` helper
`examples/csv.rs`	Fixed clippy warning by adding explicit lifetime annotation to `span_into_str` function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-26T01:34:55Z

tests/error_messages.rs

+#[test]
+fn test_extraneous_error_message() {
+    use from_pest::ConversionError;
+    use from_pest::Void;
+
+    let error: ConversionError<Void> = ConversionError::Extraneous {
+        current_node: "TestNode",
+        extraneous: "[extra_rule1, extra_rule2]".to_string(),
+    };
+
+    let message = error.to_string();
+    assert!(message.contains("TestNode"));
+    assert!(message.contains("extra_rule1"));
+    assert!(message.contains("extra_rule2"));
+    assert!(message.contains("extraneous tokens"));
+}


The test creates error instances manually but doesn't verify that the derive macro actually generates these errors in real usage. Consider adding an integration test that uses the derive macro to generate code that produces an Extraneous error with the new extraneous field populated, to ensure the format string output is as expected (e.g., that it's human-readable and not just raw Debug output like [Rule1, Rule2]).

Copilot · 2025-12-26T01:34:55Z

derive/src/from_pest/mod.rs

-            Err(::from_pest::ConversionError::NoMatch)
+            Err(::from_pest::ConversionError::NoMatchWithInfo {
+                current_node: stringify!(#name),
+                expected: stringify!(#rule_variant),


The actual field uses format!("{:?}", pair.as_rule()) which will produce output like "pair" (including the Debug formatting). Consider whether this produces the desired user-facing error message format. It might be clearer to use pair.as_rule() directly if the Rule type implements Display, or to document that this intentionally uses Debug formatting.

Suggested change

expected: stringify!(#rule_variant),

expected: stringify!(#rule_variant),

// Intentionally use `Debug` formatting here: pest rule types are only

// required to implement `Debug`, and this representation matches other

// diagnostics in the library.

Copilot · 2025-12-26T01:34:56Z

derive/src/from_pest/mod.rs

+            Err(::from_pest::ConversionError::NoMatchWithInfo {
+                current_node: stringify!(#name),
+                expected: stringify!(#rule_variant),
+                actual: format!("{:?}", pair.as_rule()),


Same issue as in the struct case: format!("{:?}", pair.as_rule()) uses Debug formatting which may not produce the most user-friendly output. Consider whether this is the intended format or if a Display implementation would be more appropriate.

Suggested change

actual: format!("{:?}", pair.as_rule()),

actual: pair.as_str().to_string(),

Copilot · 2025-12-26T01:34:56Z

src/lib.rs

+            } => write!(
+                f,
+                "when converting {current_node}, expected {expected}, but found {actual}"
+            ),


The Display implementation for NoMatchWithInfo uses string interpolation correctly, but the actual string will contain Debug-formatted output like "pair" (with quotes potentially). Consider if this produces the desired user experience. The error message might read: "when converting Value, expected value, but found pair" which is clear, but if Debug adds quotes it might be: "when converting Value, expected value, but found "pair"" which could be confusing.

Suggested change

} => write!(

f,

"when converting {current_node}, expected {expected}, but found {actual}"

),

} => {

let actual_display = if actual.len() >= 2

&& actual.starts_with('\"')

&& actual.ends_with('\"')

{

&actual[1..actual.len() - 1]

} else {

actual.as_str()

};

write!(

f,

"when converting {current_node}, expected {expected}, but found {actual_display}"

)

}

Copilot · 2025-12-26T01:34:56Z

src/lib.rs

    std::marker::PhantomData,
 };

 /// An error that occurs during conversion.


The ConversionError enum no longer derives Copy. This is a breaking change that could affect existing code that relies on copying these error values. Since NoMatchWithInfo and Extraneous now contain String fields which are not Copy, this change is necessary but should be documented as a breaking change. Consider if this impacts the library's semver version.

Suggested change

/// An error that occurs during conversion.

/// An error that occurs during conversion.

///

/// This type does **not** implement [`Copy`]. Variants such as

/// [`ConversionError::NoMatchWithInfo`] and [`ConversionError::Extraneous`]

/// contain owned [`String`] values, which cannot be `Copy`.

///

/// If a previous version of this crate exposed `ConversionError` as `Copy`,

/// migrating code may need to be updated to either:

/// - borrow these values instead of copying them, or

/// - clone them explicitly via [`Clone`].

Copilot · 2025-12-26T01:34:56Z

derive/src/from_pest/mod.rs

                #extraneous
                Err(::from_pest::ConversionError::Extraneous {
                    current_node: stringify!(#name),
+                    extraneous: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()),


The format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()) expression can be expensive when there are many extraneous tokens, as it clones the iterator and collects into a Vec. Consider using a more efficient approach, such as collecting the rules first and then formatting, or limiting the number of tokens displayed if there could be many extraneous tokens.

Copilot · 2025-12-26T01:34:57Z

derive/src/from_pest/mod.rs

                    #extraneous
                    Err(::from_pest::ConversionError::Extraneous {
                        current_node: stringify!(#variant_name),
+                        extraneous: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()),


The same performance concern as in the struct case: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()) could be expensive when there are many extraneous tokens. Consider using a more efficient approach or limiting the number of displayed tokens.

Suggested change

extraneous: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()),

extraneous: {

let mut s = ::std::string::String::new();

use ::std::fmt::Write as _;

s.push('[');

for (i, rule) in inner.clone().map(|p| p.as_rule()).enumerate() {

if i > 0 {

s.push_str(", ");

}

if i >= 16 {

s.push_str("...");

break;

}

let _ = write!(&mut s, "{:?}", rule);

}

s.push(']');

s

},

Initial plan

139ee9e

Copilot AI assigned Copilot and tomtau Dec 25, 2025

Copilot started work on behalf of tomtau December 25, 2025 17:54 View session

Copilot AI and others added 2 commits December 25, 2025 18:06

Use is_no_match() helper for better maintainability

e2b717b

Address code review feedback to use the is_no_match() helper method instead of pattern matching both NoMatch variants explicitly. Co-authored-by: tomtau <[email protected]>

Copilot AI changed the title ~~[WIP] Update error messages to include current Pest token~~ Improve ConversionError messages with token details Dec 25, 2025

Copilot AI requested a review from tomtau December 25, 2025 18:10

Copilot finished work on behalf of tomtau December 25, 2025 18:10

Copilot started work on behalf of tomtau December 26, 2025 01:25 View session

Fix clippy warnings for lifetime elision in csv examples

0cb8ecd

Add explicit lifetime parameter `<'_>` to Span type to fix mismatched_lifetime_syntaxes clippy warnings. Co-authored-by: tomtau <[email protected]>

Copilot finished work on behalf of tomtau December 26, 2025 01:29

tomtau requested a review from Copilot December 26, 2025 01:31

Copilot started reviewing on behalf of tomtau December 26, 2025 01:31 View session

Copilot AI reviewed Dec 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve ConversionError messages with token details #41

Improve ConversionError messages with token details #41

Uh oh!

Copilot AI commented Dec 25, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 25, 2025

Review skipped

Uh oh!

tomtau commented Dec 26, 2025

Uh oh!

Copilot AI commented Dec 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Copilot AI Dec 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-                expected: stringify!(#rule_variant),
+                expected: stringify!(#rule_variant),
+                // Intentionally use `Debug` formatting here: pest rule types are only
+                // required to implement `Debug`, and this representation matches other
+                // diagnostics in the library.

	actual: format!("{:?}", pair.as_rule()),
	actual: pair.as_str().to_string(),

-            } => write!(
-                f,
-                "when converting {current_node}, expected {expected}, but found {actual}"
-            ),
+            } => {
+                let actual_display = if actual.len() >= 2
+                    && actual.starts_with('\"')
+                    && actual.ends_with('\"')
+                {
+                    &actual[1..actual.len() - 1]
+                } else {
+                    actual.as_str()
+                };
+                write!(
+                    f,
+                    "when converting {current_node}, expected {expected}, but found {actual_display}"
+                )
+            }

-/// An error that occurs during conversion.
+/// An error that occurs during conversion.
+///
+/// This type does **not** implement [`Copy`]. Variants such as
+/// [`ConversionError::NoMatchWithInfo`] and [`ConversionError::Extraneous`]
+/// contain owned [`String`] values, which cannot be `Copy`.
+///
+/// If a previous version of this crate exposed `ConversionError` as `Copy`,
+/// migrating code may need to be updated to either:
+/// - borrow these values instead of copying them, or
+/// - clone them explicitly via [`Clone`].

-                        extraneous: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()),
+                        extraneous: {
+                            let mut s = ::std::string::String::new();
+                            use ::std::fmt::Write as _;
+                            s.push('[');
+                            for (i, rule) in inner.clone().map(|p| p.as_rule()).enumerate() {
+                                if i > 0 {
+                                    s.push_str(", ");
+                                }
+                                if i >= 16 {
+                                    s.push_str("...");
+                                    break;
+                                }
+                                let _ = write!(&mut s, "{:?}", rule);
+                            }
+                            s.push(']');
+                            s
+                        },

Improve ConversionError messages with token details #41

Are you sure you want to change the base?

Improve ConversionError messages with token details #41

Uh oh!

Conversation

Copilot AI commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 25, 2025

Review skipped

Uh oh!

tomtau commented Dec 26, 2025

Uh oh!

Copilot AI commented Dec 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Dec 25, 2025 •

edited

Loading