From 6304e1128a93d2d254c10fd634cfc3fedccc2a19 Mon Sep 17 00:00:00 2001 From: RunasSudo Date: Fri, 29 Oct 2021 23:07:38 +1100 Subject: [PATCH] Validate well-formedness of constraints, better constraint errors --- src/cli/stv.rs | 30 +++++-- src/constraints.rs | 148 ++++++++++++++++++++++++++++---- src/election.rs | 1 + src/stv/wasm.rs | 10 ++- tests/tests_impl/constraints.rs | 8 +- 5 files changed, 168 insertions(+), 29 deletions(-) diff --git a/src/cli/stv.rs b/src/cli/stv.rs index 27614c3..09b30e7 100644 --- a/src/cli/stv.rs +++ b/src/cli/stv.rs @@ -203,25 +203,25 @@ pub fn main(cmd_opts: SubcmdOptions) -> Result<(), i32> { // Read and count election according to --numbers if cmd_opts.numbers == "rational" { let mut election = election_from_file(&cmd_opts.filename, cmd_opts.bin)?; - maybe_load_constraints(&mut election, &cmd_opts.constraints); + maybe_load_constraints(&mut election, &cmd_opts.constraints)?; // Must specify :: here and in a few other places because ndarray causes E0275 otherwise count_election::(election, cmd_opts)?; } else if cmd_opts.numbers == "float64" { let mut election = election_from_file(&cmd_opts.filename, cmd_opts.bin)?; - maybe_load_constraints(&mut election, &cmd_opts.constraints); + maybe_load_constraints(&mut election, &cmd_opts.constraints)?; count_election::(election, cmd_opts)?; } else if cmd_opts.numbers == "fixed" { Fixed::set_dps(cmd_opts.decimals); let mut election = election_from_file(&cmd_opts.filename, cmd_opts.bin)?; - maybe_load_constraints(&mut election, &cmd_opts.constraints); + maybe_load_constraints(&mut election, &cmd_opts.constraints)?; count_election::(election, cmd_opts)?; } else if cmd_opts.numbers == "gfixed" { GuardedFixed::set_dps(cmd_opts.decimals); let mut election = election_from_file(&cmd_opts.filename, cmd_opts.bin)?; - maybe_load_constraints(&mut election, &cmd_opts.constraints); + maybe_load_constraints(&mut election, &cmd_opts.constraints)?; count_election::(election, cmd_opts)?; } @@ -244,12 +244,30 @@ fn election_from_file(path: &str, bin: bool) -> Result, i } } -fn maybe_load_constraints(election: &mut Election, constraints: &Option) { +fn maybe_load_constraints(election: &mut Election, constraints: &Option) -> Result<(), i32> { if let Some(c) = constraints { let file = File::open(c).expect("IO Error"); let lines = io::BufReader::new(file).lines(); - election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error")))); + let lines: Vec<_> = lines.map(|r| r.expect("IO Error")).collect(); + + match Constraints::from_con(lines.into_iter()) { + Ok(c) => { + election.constraints = Some(c); + } + Err(err) => { + println!("Constraint Syntax Error: {}", err); + return Err(1); + } + } + + // Validate constraints + if let Err(err) = election.constraints.as_ref().unwrap().validate_constraints(election.candidates.len()) { + println!("Constraint Validation Error: {}", err); + return Err(1); + } } + + Ok(()) } fn count_election(election: Election, cmd_opts: SubcmdOptions) -> Result<(), i32> diff --git a/src/constraints.rs b/src/constraints.rs index e1219e8..41dacba 100644 --- a/src/constraints.rs +++ b/src/constraints.rs @@ -27,6 +27,7 @@ use rkyv::{Archive, Deserialize, Serialize}; use std::collections::HashMap; use std::fmt; +use std::num::ParseIntError; use std::ops; /// Constraints for an [crate::election::Election] @@ -36,24 +37,28 @@ pub struct Constraints(pub Vec); impl Constraints { /// Parse the given CON file and return a [Constraints] - pub fn from_con>(lines: I) -> Self { + pub fn from_con, I: Iterator>(lines: I) -> Result { let mut constraints = Constraints(Vec::new()); - for line in lines { - let mut bits = line.split(' ').peekable(); + for (line_no, line) in lines.enumerate() { + let mut bits = line.as_ref().split(' ').peekable(); // Read constraint category and group - let constraint_name = read_quoted_string(&mut bits); - let group_name = read_quoted_string(&mut bits); + let constraint_name = read_quoted_string(line_no, &mut bits)?; + let group_name = read_quoted_string(line_no, &mut bits)?; // Read min, max - let min: usize = bits.next().expect("Syntax Error").parse().expect("Syntax Error"); - let max: usize = bits.next().expect("Syntax Error").parse().expect("Syntax Error"); + let min: usize = bits + .next().ok_or(ParseError::UnexpectedEOL(line_no, "minimum number"))? + .parse().map_err(|e| ParseError::InvalidNumber(line_no, e))?; + let max: usize = bits + .next().ok_or(ParseError::UnexpectedEOL(line_no, "maximum number"))? + .parse().map_err(|e| ParseError::InvalidNumber(line_no, e))?; // Read candidates let mut candidates: Vec = Vec::new(); for x in bits { - candidates.push(x.parse::().expect("Syntax Error") - 1); + candidates.push(x.parse::().map_err(|e| ParseError::InvalidNumber(line_no, e))? - 1); } // Insert constraint/group @@ -70,7 +75,7 @@ impl Constraints { }; if constraint.groups.iter().any(|g| g.name == group_name) { - panic!("Duplicate group \"{}\" in constraint \"{}\"", group_name, constraint.name); + return Err(ParseError::DuplicateGroup(line_no, group_name, constraint.name.clone())); } constraint.groups.push(ConstrainedGroup { @@ -81,26 +86,132 @@ impl Constraints { }); } - // TODO: Validate constraints + return Ok(constraints); + } + + /// Validate that each candidate is specified exactly once in each constraint + pub fn validate_constraints(&self, num_candidates: usize) -> Result<(), ValidationError> { + for constraint in &self.0 { + let mut remaining_candidates: Vec = (0..num_candidates).collect(); + + for group in &constraint.groups { + for candidate in &group.candidates { + match remaining_candidates.iter().position(|c| c == candidate) { + Some(idx) => { + remaining_candidates.remove(idx); + } + None => { + return Err(ValidationError::DuplicateCandidate(*candidate, constraint.name.clone())); + } + } + } + } + + if !remaining_candidates.is_empty() { + return Err(ValidationError::UnassignedCandidate(*remaining_candidates.first().unwrap(), constraint.name.clone())); + } + } - return constraints; + Ok(()) } } +/// Error parsing constraints +pub enum ParseError { + /// Duplicate group in a constraint + DuplicateGroup(usize, String, String), + /// Unexpected EOL, expected ... + UnexpectedEOL(usize, &'static str), + /// Invalid number + InvalidNumber(usize, ParseIntError), +} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ParseError::DuplicateGroup(line_no, group_name, constraint_name) => { + f.write_fmt(format_args!(r#"Line {}, duplicate group "{}" in constraint "{}""#, line_no, group_name, constraint_name)) + } + ParseError::UnexpectedEOL(line_no, expected) => { + f.write_fmt(format_args!(r#"Line {}, unexpected end-of-line, expected {}"#, line_no, expected)) + } + ParseError::InvalidNumber(line_no, err) => { + f.write_fmt(format_args!(r#"Line {}, invalid number: {}"#, line_no, err)) + } + } + } +} + +impl fmt::Debug for ParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + return fmt::Display::fmt(self, f); + } +} + +/// Error validating constraints +pub enum ValidationError { + /// Duplicate candidate in a constraint + DuplicateCandidate(usize, String), + /// Unassigned candidate in a constraint + UnassignedCandidate(usize, String), +} + +impl fmt::Display for ValidationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ValidationError::DuplicateCandidate(candidate, constraint_name) => { + f.write_fmt(format_args!(r#"Duplicate candidate {} in constraint "{}""#, candidate + 1, constraint_name)) + } + ValidationError::UnassignedCandidate(candidate, constraint_name) => { + f.write_fmt(format_args!(r#"Unassigned candidate {} in constraint "{}""#, candidate + 1, constraint_name)) + } + } + } +} + +impl fmt::Debug for ValidationError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + return fmt::Display::fmt(self, f); + } +} + +#[test] +fn duplicate_contraint_group() { + let input = r#""Constraint 1" "Group 1" 0 3 1 2 3 +"Constraint 1" "Group 1" 0 3 4 5 6"#; + Constraints::from_con(input.lines()).unwrap_err(); +} + +#[test] +fn duplicate_candidate() { + let input = r#""Constraint 1" "Group 1" 0 3 1 2 3 4 +"Constraint 1" "Group 2" 0 3 4 5 6"#; + let constraints = Constraints::from_con(input.lines()).unwrap(); + constraints.validate_constraints(6).unwrap_err(); +} + +#[test] +fn unassigned_candidate() { + let input = r#""Constraint 1" "Group 1" 0 3 1 2 3 +"Constraint 1" "Group 2" 0 3 4 5 6"#; + let constraints = Constraints::from_con(input.lines()).unwrap(); + constraints.validate_constraints(7).unwrap_err(); +} + /// Read an optionally quoted string, returning the string without quotes -fn read_quoted_string<'a, I: Iterator>(bits: &mut I) -> String { - let x = bits.next().expect("Syntax Error"); +fn read_quoted_string<'a, I: Iterator>(line_no: usize, bits: &mut I) -> Result { + let x = bits.next().ok_or(ParseError::UnexpectedEOL(line_no, "string continuation"))?; if let Some(x1) = x.strip_prefix('"') { if let Some(x2) = x.strip_suffix('"') { // Complete string - return String::from(x2); + return Ok(String::from(x2)); } else { // Incomplete string let mut result = String::from(x1); // Read until matching " loop { - let x = bits.next().expect("Syntax Error"); + let x = bits.next().ok_or(ParseError::UnexpectedEOL(line_no, "string continuation"))?; result.push(' '); if let Some(x1) = x.strip_suffix('"') { // End of string @@ -112,11 +223,11 @@ fn read_quoted_string<'a, I: Iterator>(bits: &mut I) -> String { } } - return result; + return Ok(result); } } else { // Unquoted string - return String::from(x); + return Ok(String::from(x)); } } @@ -227,7 +338,8 @@ impl ConstraintMatrix { return j + 1; } } - panic!("Candidate \"{}\" not represented in constraint \"{}\"", candidate.name, c.name); + // Should be caught by validate_constraints + unreachable!("Candidate \"{}\" not represented in constraint \"{}\"", candidate.name, c.name); }).collect(); let cell = &mut self[&idx[..]]; diff --git a/src/election.rs b/src/election.rs index 73cc170..1260414 100644 --- a/src/election.rs +++ b/src/election.rs @@ -179,6 +179,7 @@ impl<'a, N: Number> CountState<'a, N> { // Init constraints if let Some(constraints) = &election.constraints { + // Init constraint matrix let mut num_groups: Vec = constraints.0.iter().map(|c| c.groups.len()).collect(); let mut cm = ConstraintMatrix::new(&mut num_groups[..]); diff --git a/src/stv/wasm.rs b/src/stv/wasm.rs index 399bb54..9031653 100644 --- a/src/stv/wasm.rs +++ b/src/stv/wasm.rs @@ -92,7 +92,15 @@ macro_rules! impl_type { #[cfg_attr(feature = "wasm", wasm_bindgen)] #[allow(non_snake_case)] pub fn [](election: &mut [], text: String) { - election.0.constraints = Some(Constraints::from_con(text.lines().map(|s| s.to_string()).into_iter())); + election.0.constraints = match Constraints::from_con(text.lines()) { + Ok(c) => Some(c), + Err(err) => wasm_error!("Constraint Syntax Error", err), + }; + + // Validate constraints + if let Err(err) = election.0.constraints.as_ref().unwrap().validate_constraints(election.0.candidates.len()) { + wasm_error!("Constraint Validation Error", err); + } } /// Wrapper for [stv::preprocess_election] diff --git a/tests/tests_impl/constraints.rs b/tests/tests_impl/constraints.rs index 39ea9bc..dc8b6d3 100644 --- a/tests/tests_impl/constraints.rs +++ b/tests/tests_impl/constraints.rs @@ -37,7 +37,7 @@ fn prsa1_constr1_rational() { let file = File::open("tests/data/prsa1_constr1.con").expect("IO Error"); let file_reader = io::BufReader::new(file); let lines = file_reader.lines(); - election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter())); + election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter()).unwrap()); let stv_opts = stv::STVOptionsBuilder::default() .round_surplus_fractions(Some(3)) @@ -86,7 +86,7 @@ fn prsa1_constr2_rational() { let file = File::open("tests/data/prsa1_constr2.con").expect("IO Error"); let file_reader = io::BufReader::new(file); let lines = file_reader.lines(); - election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter())); + election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter()).unwrap()); let stv_opts = stv::STVOptionsBuilder::default() .round_surplus_fractions(Some(3)) @@ -135,7 +135,7 @@ fn prsa1_constr3_rational() { let file = File::open("tests/data/prsa1_constr3.con").expect("IO Error"); let file_reader = io::BufReader::new(file); let lines = file_reader.lines(); - election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter())); + election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter()).unwrap()); let stv_opts = stv::STVOptionsBuilder::default() .round_surplus_fractions(Some(3)) @@ -196,7 +196,7 @@ fn ers97old_cantbulkexclude_rational() { let file = File::open("tests/data/ers97old_cantbulkexclude.con").expect("IO Error"); let file_reader = io::BufReader::new(file); let lines = file_reader.lines(); - election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter())); + election.constraints = Some(Constraints::from_con(lines.map(|r| r.expect("IO Error").to_string()).into_iter()).unwrap()); let stv_opts = stv::STVOptionsBuilder::default() .round_surplus_fractions(Some(2))