Skip to content

fix: check for out-of-field sign chars #19

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
david-r-cox wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: check for out-of-field sign chars #19

Show file tree
Hide file tree
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
82 changes: 64 additions & 18 deletions src/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,23 +509,44 @@ impl<'a, T: FastFloat> Parser<'a, T> {

// Try strict field positioning first (no comment stripping for strict parsing)
let strict_result = (|| -> Result<WideLine<T>> {
// Extract value string and check if we might be missing a negative sign
let val_str = line_str.get(L4..R4).ok_or_eyre("")?.trim();

// Check character just before L4 to ensure we're not missing a sign
// If L4 > 0 and the char at L4-1 is '-', we're probably misaligned
if L4 > 0 && line_str.len() > L4 {
if let Some(prev_char) = line_str.chars().nth(L4 - 1) {
if prev_char == '-' || prev_char == '+' {
// Sign character right before our field - reject strict parsing
return Err(eyre!(
"potential sign character excluded from value field"
));
}
}
}

let first_pair = RowValuePair {
row_name: line_str.get(L3..R3).ok_or_eyre("")?.trim(),
value: fast_float2::parse(
line_str.get(L4..R4).ok_or_eyre("")?.trim(),
)?,
value: fast_float2::parse(val_str)?,
};
let second_pair = match line_str.get(L5..R5) {
Some(row_name) => {
let row_name = row_name.trim();
if row_name.is_empty() {
None
} else {
// Check for sign before second value too
let val2_str = line_str.get(L6..R6).ok_or_eyre("")?.trim();
if L6 > 0 && line_str.len() > L6 {
if let Some(prev_char) = line_str.chars().nth(L6 - 1) {
if prev_char == '-' || prev_char == '+' {
return Err(eyre!("potential sign character excluded from second value field"));
}
}
}
Some(RowValuePair {
row_name,
value: fast_float2::parse(
line_str.get(L6..R6).ok_or_eyre("")?.trim(),
)?,
value: fast_float2::parse(val2_str)?,
})
}
}
Expand Down Expand Up @@ -890,23 +911,48 @@ impl<'a, T: FastFloat> Parser<'a, T> {

/// Parse bounds line using strict field positioning
fn parse_bounds_strict(line: Span) -> Result<BoundsLine<T>> {
let length = line.len();
let bound_type = BoundType::try_from(line.get(L1..R1).ok_or_eyre("")?)?;
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let line_str = *line.fragment();
} else {
let line_str = line;
}
}
let length = line_str.len();
let bound_type = BoundType::try_from(line_str.get(L1..R1).ok_or_eyre("")?)?;
Ok(match bound_type {
BoundType::Fr | BoundType::Pl => BoundsLine::<T> {
bound_type,
bound_name: line.get(L2..R2).ok_or_eyre("")?.trim(),
column_name: line.get(L3..cmp::min(length, R3)).ok_or_eyre("")?.trim(),
bound_name: line_str.get(L2..R2).ok_or_eyre("")?.trim(),
column_name: line_str
.get(L3..cmp::min(length, R3))
.ok_or_eyre("")?
.trim(),
value: None,
},
_ => BoundsLine::<T> {
bound_type,
bound_name: line.get(L2..R2).ok_or_eyre("")?.trim(),
column_name: line.get(L3..R3).ok_or_eyre("")?.trim(),
value: Some(fast_float2::parse(
line.get(L4..cmp::min(length, R4)).ok_or_eyre("")?.trim(),
)?),
},
_ => {
// Check for sign character just before the value field
if L4 > 0 && line_str.len() > L4 {
if let Some(prev_char) = line_str.chars().nth(L4 - 1) {
if prev_char == '-' || prev_char == '+' {
return Err(eyre!(
"potential sign character excluded from value field"
));
}
}
}
BoundsLine::<T> {
bound_type,
bound_name: line_str.get(L2..R2).ok_or_eyre("")?.trim(),
column_name: line_str.get(L3..R3).ok_or_eyre("")?.trim(),
value: Some(fast_float2::parse(
line_str
.get(L4..cmp::min(length, R4))
.ok_or_eyre("")?
.trim(),
)?),
}
}
})
}

Expand Down
209 changes: 209 additions & 0 deletions tests/unit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,55 @@ mod tests {
}),
),
},
// Regression test: line without leading spaces and large negative value
TestData {
input: " C0000823 OBJECTRW -401552000.00\n",
expected: (
"",
Some(WideLine {
name: "C0000823",
first_pair: RowValuePair {
row_name: "OBJECTRW",
value: -401552000.0,
},
second_pair: None,
}),
),
},
// Test case with properly formatted strict field positioning
TestData {
input: " X99 ROW1 -123.0\n",
expected: (
"",
Some(WideLine {
name: "X99",
first_pair: RowValuePair {
row_name: "ROW1",
value: -123.0,
},
second_pair: None,
}),
),
},
// Test case with two values where sign character falls just before second value field
// This should trigger the sign validation check for second value (line 538-546 in parse.rs)
TestData {
input: " X98 ROW1 100.0 ROW2 -20\n",
expected: (
"",
Some(WideLine {
name: "X98",
first_pair: RowValuePair {
row_name: "ROW1",
value: 100.0,
},
second_pair: Some(RowValuePair {
row_name: "ROW2",
value: -20.0,
}),
}),
),
},
];
for case in test_cases {
cfg_if::cfg_if! {
Expand Down Expand Up @@ -578,6 +627,58 @@ mod tests {
}),
),
},
// Test FR (Free) bound type - no value field (line 924-932 in parse.rs)
TestData {
input: " FR BND1 XFREE\n",
expected: (
"",
Some(BoundsLine {
bound_type: BoundType::Fr,
bound_name: "BND1",
column_name: "XFREE",
value: None,
}),
),
},
// Test PL (Plus infinity) bound type - no value field (line 924-932 in parse.rs)
TestData {
input: " PL BND1 XPLUS\n",
expected: (
"",
Some(BoundsLine {
bound_type: BoundType::Pl,
bound_name: "BND1",
column_name: "XPLUS",
value: None,
}),
),
},
// Test FX (Fixed) bound type with value - should trigger sign check (line 934-956 in parse.rs)
TestData {
input: " FX BND1 XFIXED -5\n",
expected: (
"",
Some(BoundsLine {
bound_type: BoundType::Fx,
bound_name: "BND1",
column_name: "XFIXED",
value: Some(-5.0),
}),
),
},
// Test MI (Minus infinity) bound type with value - should trigger sign check (line 934-956 in parse.rs)
TestData {
input: " MI BND1 XMINUS\n",
expected: (
"",
Some(BoundsLine {
bound_type: BoundType::Mi,
bound_name: "BND1",
column_name: "XMINUS",
value: None,
}),
),
},
];
for case in test_cases {
cfg_if::cfg_if! {
Expand Down Expand Up @@ -1832,6 +1933,114 @@ ENDATA
Ok(())
}

/// Test sign character validation for columns lines
/// When a sign character appears just before the expected field position,
/// the strict parser should detect it and fall back to flexible parsing
#[test]
fn test_columns_sign_validation() -> Result<()> {
// Test case: sign character at position L4-1 (position 22, just before value field at 23)
// L4 = 23, so position 22 should have the sign character
// Format: " COL1 ROW1 -123\n"
// Positions: 0-3 (spaces), 4-11 (COL1), 12-13 (spaces), 14-21 (ROW1), 22 (-), 23-35 (value field)
let input = " COL1 ROW1 -123\n";
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let info = TracableInfo::new().forward(false).backward(false);
let result = Parser::<f32>::columns_line(LocatedSpan::new_extra(input, info));
} else {
let result = Parser::<f32>::columns_line(input);
}
}
// This should succeed with flexible parsing fallback
assert!(result.is_ok());

// Also test the second value field sign validation
// L6 = 48, so position 47 should have the sign character for second value
let input2 = " COL2 ROW1 100 ROW2 -200\n";
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let info = TracableInfo::new().forward(false).backward(false);
let result2 = Parser::<f32>::columns_line(LocatedSpan::new_extra(input2, info));
} else {
let result2 = Parser::<f32>::columns_line(input2);
}
}
assert!(result2.is_ok());

Ok(())
}

/// Test sign character validation for bounds lines
/// When a sign character appears just before the value field position,
/// the strict parser should detect it and fall back to flexible parsing
#[test]
fn test_bounds_sign_validation() -> Result<()> {
// Test case: sign character at position L4-1 (just before value field)
// For bound types that require a value (not FR or PL)
let input = " UP BND1 VAR1-999.99\n";
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let info = TracableInfo::new().forward(false).backward(false);
let result = Parser::<f32>::bounds_line(LocatedSpan::new_extra(input, info));
} else {
let result = Parser::<f32>::bounds_line(input);
}
}
// This should succeed with flexible parsing fallback
assert!(result.is_ok());

// Additional test: ensure FR and PL bounds (no value) work correctly
let fr_input = " FR BND1 VAR2\n";
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let info = TracableInfo::new().forward(false).backward(false);
let fr_result = Parser::<f32>::bounds_line(LocatedSpan::new_extra(fr_input, info));
} else {
let fr_result = Parser::<f32>::bounds_line(fr_input);
}
}
assert!(fr_result.is_ok());
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
if let Ok((_, Some(bound))) = fr_result {
assert_eq!(bound.bound_type, BoundType::Fr);
assert_eq!(bound.value, None);
}
} else {
if let Ok((_, Some(bound))) = fr_result {
assert_eq!(bound.bound_type, BoundType::Fr);
assert_eq!(bound.value, None);
}
}
}

let pl_input = " PL BND1 VAR3\n";
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
let info = TracableInfo::new().forward(false).backward(false);
let pl_result = Parser::<f32>::bounds_line(LocatedSpan::new_extra(pl_input, info));
} else {
let pl_result = Parser::<f32>::bounds_line(pl_input);
}
}
assert!(pl_result.is_ok());
cfg_if::cfg_if! {
if #[cfg(feature = "trace")] {
if let Ok((_, Some(bound))) = pl_result {
assert_eq!(bound.bound_type, BoundType::Pl);
assert_eq!(bound.value, None);
}
} else {
if let Ok((_, Some(bound))) = pl_result {
assert_eq!(bound.bound_type, BoundType::Pl);
assert_eq!(bound.value, None);
}
}
}

Ok(())
}

/// Test indicator constraints with correct format
#[test]
fn test_indicators_format() -> Result<()> {
Expand Down
Loading