-
Notifications
You must be signed in to change notification settings - Fork 17
Improve ConversionError messages with token details #41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -151,12 +151,17 @@ fn derive_for_struct( | |||||||||||||||||||||||||||||||||||||
| #extraneous | ||||||||||||||||||||||||||||||||||||||
| Err(::from_pest::ConversionError::Extraneous { | ||||||||||||||||||||||||||||||||||||||
| current_node: stringify!(#name), | ||||||||||||||||||||||||||||||||||||||
| extraneous: format!("{:?}", inner.clone().map(|p| p.as_rule()).collect::<Vec<_>>()), | ||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| *pest = clone; | ||||||||||||||||||||||||||||||||||||||
| Ok(this) | ||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||
| Err(::from_pest::ConversionError::NoMatch) | ||||||||||||||||||||||||||||||||||||||
| Err(::from_pest::ConversionError::NoMatchWithInfo { | ||||||||||||||||||||||||||||||||||||||
| current_node: stringify!(#name), | ||||||||||||||||||||||||||||||||||||||
| expected: stringify!(#rule_variant), | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| 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
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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 | |
| }, |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| actual: format!("{:?}", pair.as_rule()), | |
| actual: pair.as_str().to_string(), |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,14 +18,38 @@ use { | |||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// An error that occurs during conversion. | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| /// 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
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| } => 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}" | |
| ) | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| //! Test for improved error messages. | ||
|
|
||
| #![allow(dead_code)] | ||
|
|
||
| #[macro_use] | ||
| extern crate pest_derive; | ||
| extern crate from_pest; | ||
| #[macro_use] | ||
| extern crate pest_ast; | ||
| extern crate pest; | ||
|
|
||
| mod grammar { | ||
| #[derive(Parser)] | ||
| #[grammar_inline = r#" | ||
| value = { "number" | "string" } | ||
| pair = { name ~ ":" ~ value } | ||
| name = { ASCII_ALPHA+ } | ||
| WHITESPACE = _{ " " } | ||
| "#] | ||
| pub struct Parser; | ||
| } | ||
|
|
||
| mod ast { | ||
| use super::grammar::Rule; | ||
|
|
||
| #[derive(Debug, FromPest)] | ||
| #[pest_ast(rule(Rule::name))] | ||
| pub struct Name<'pest> { | ||
| #[pest_ast(outer())] | ||
| pub span: pest::Span<'pest>, | ||
| } | ||
|
|
||
| #[derive(Debug, FromPest)] | ||
| #[pest_ast(rule(Rule::value))] | ||
| pub struct Value<'pest> { | ||
| #[pest_ast(outer())] | ||
| pub span: pest::Span<'pest>, | ||
| } | ||
|
|
||
| #[derive(Debug, FromPest)] | ||
| #[pest_ast(rule(Rule::pair))] | ||
| pub struct Pair<'pest> { | ||
| pub name: Name<'pest>, | ||
| pub value: Value<'pest>, | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_no_match_error_message() { | ||
| use from_pest::ConversionError; | ||
| use from_pest::FromPest; | ||
| use pest::Parser; | ||
|
|
||
| // Parse "name: number" as a value rule (wrong rule type) | ||
| let source = "name: number"; | ||
| let mut pairs = grammar::Parser::parse(grammar::Rule::pair, source).expect("parse success"); | ||
|
|
||
| // Try to parse as Value (should fail with informative error) | ||
| let result: Result<ast::Value<'_>, _> = ast::Value::from_pest(&mut pairs); | ||
|
|
||
| assert!(result.is_err()); | ||
| let error = result.unwrap_err(); | ||
|
|
||
| match &error { | ||
| ConversionError::NoMatchWithInfo { | ||
| current_node, | ||
| expected, | ||
| actual, | ||
| } => { | ||
| assert_eq!(*current_node, "Value"); | ||
| assert_eq!(*expected, "value"); | ||
| assert!(actual.contains("pair")); // The actual rule is 'pair' | ||
| } | ||
| _ => panic!("Expected NoMatchWithInfo error, got: {:?}", error), | ||
| } | ||
|
|
||
| // Verify the Display implementation | ||
| let error_message = error.to_string(); | ||
| assert!(error_message.contains("Value")); | ||
| assert!(error_message.contains("value")); | ||
| assert!(error_message.contains("pair")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_no_match_helper() { | ||
| use from_pest::ConversionError; | ||
|
|
||
| let no_match: ConversionError<()> = ConversionError::NoMatch; | ||
| assert!(no_match.is_no_match()); | ||
|
|
||
| let no_match_with_info: ConversionError<()> = ConversionError::NoMatchWithInfo { | ||
| current_node: "Test", | ||
| expected: "expected", | ||
| actual: "actual".to_string(), | ||
| }; | ||
| assert!(no_match_with_info.is_no_match()); | ||
|
|
||
| let extraneous: ConversionError<()> = ConversionError::Extraneous { | ||
| current_node: "Test", | ||
| extraneous: "[extra]".to_string(), | ||
| }; | ||
| assert!(!extraneous.is_no_match()); | ||
| } | ||
|
|
||
| #[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")); | ||
| } | ||
|
Comment on lines
+105
to
+120
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.