Skip to content

test(position): add should_panic tests for documented panic conditions #1168

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.

Sign up for GitHub

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

Open
MasterOfFeelingFish wants to merge 1 commit into
base: master
Choose a base branch
from
+27 −0
Open

test(position): add should_panic tests for documented panic conditions #1168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions pest/src/position.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,4 +659,31 @@ mod tests {

positions.insert(start);
}

#[test]
#[should_panic(expected = "span created from positions from different inputs")]
fn span_panic_different_inputs() {
let input1 = "a";
let input2 = "b";
let pos1 = Position::from_start(input1);
let pos2 = Position::from_start(input2);

pos1.span(&pos2);
}

#[test]
#[should_panic(expected = "position out of bounds")]
fn line_col_panic_out_of_bounds() {
let input = "ab";
let pos = Position::new_internal(input, 5);
pos.line_col();
}

#[test]
#[should_panic(expected = "position out of bounds")]
fn line_of_panic_out_of_bounds() {
let input = "ab";
let pos = Position::new_internal(input, 5);
pos.line_of();
}
Comment on lines +674 to +688

@coderabbitai coderabbitai Bot Apr 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Confirm the invalid-position tests call new_internal despite its debug_assert invariant check.
rg -n -C2 'fn new_internal|debug_assert!\(input\.get\(pos\.\.\)\.is_some\(\)\)|Position::new_internal\(input, 5\)' pest/src/position.rs

Repository: pest-parser/pest

Length of output: 700

🏁 Script executed:

rg -n "fn line_col|fn line_of" pest/src/position.rs | head -20

Repository: pest-parser/pest

Length of output: 388

🏁 Script executed:

rg -n -A 20 "fn line_col\(" pest/src/position.rs | grep -E "panic|unwrap"

Repository: pest-parser/pest

Length of output: 957

🏁 Script executed:

rg -n -A 20 "fn line_of\(" pest/src/position.rs | grep -E "panic|unwrap"

Repository: pest-parser/pest

Length of output: 947

🏁 Script executed:

rg -n -B 2 "struct Position" pest/src/position.rs | head -20

Repository: pest-parser/pest

Length of output: 212

🏁 Script executed:

rg -n -A 5 "pub struct Position" pest/src/position.rs

Repository: pest-parser/pest

Length of output: 166

🏁 Script executed:

rg -n "mod tests|#\[cfg\(test\)\]" pest/src/position.rs

Repository: pest-parser/pest

Length of output: 92

🏁 Script executed:

rg -n -A 10 "pub struct Position" pest/src/position.rs | head -15

Repository: pest-parser/pest

Length of output: 440

Bypass new_internal to reach the intended panic message in these tests.

The tests on lines 678 and 686 call Position::new_internal(input, 5) for input "ab", but that constructor has a debug_assert!(input.get(pos..).is_some()) that will panic before line_col() or line_of() execute. The debug assertion will panic with an assertion failure message, not the expected "position out of bounds", causing the #[should_panic] check to fail.

Fix 
     fn line_col_panic_out_of_bounds() {
         let input = "ab";
-        let pos = Position::new_internal(input, 5);
+        let pos = Position { input, pos: 5 };
         pos.line_col();
     }
@@
     fn line_of_panic_out_of_bounds() {
         let input = "ab";
-        let pos = Position::new_internal(input, 5);
+        let pos = Position { input, pos: 5 };
         pos.line_of();
     }
📝 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.

Suggested change
#[test]
#[should_panic(expected = "position out of bounds")]
fn line_col_panic_out_of_bounds() {
let input = "ab";
let pos = Position::new_internal(input, 5);
pos.line_col();
}
#[test]
#[should_panic(expected = "position out of bounds")]
fn line_of_panic_out_of_bounds() {
let input = "ab";
let pos = Position::new_internal(input, 5);
pos.line_of();
}
#[test]
#[should_panic(expected = "position out of bounds")]
fn line_col_panic_out_of_bounds() {
let input = "ab";
let pos = Position { input, pos: 5 };
pos.line_col();
}
#[test]
#[should_panic(expected = "position out of bounds")]
fn line_of_panic_out_of_bounds() {
let input = "ab";
let pos = Position { input, pos: 5 };
pos.line_of();
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pest/src/position.rs` around lines 674 - 688, The tests currently call
Position::new_internal(input, 5) which triggers its debug_assert before reaching
line_col()/line_of(); instead construct a Position that bypasses new_internal so
the methods themselves perform the bounds check and panic with "position out of
bounds". Replace the calls to Position::new_internal with a direct construction
that sets the internal input and offset (e.g., create the same backing
string/arc/rc used by Position and instantiate a Position with pos/offset = 5),
or use any existing unsafe/unchecked constructor if provided, then call
pos.line_col() and pos.line_of() so the expected panic message is produced
(refer to Position::new_internal, line_col, line_of to locate the code).

Comment on lines +674 to +688

@tomtau tomtau Apr 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two tests don't seem to pass

}
Loading