From 1316f271df2b4e8a37e7f33f041344a547c48fd0 Mon Sep 17 00:00:00 2001 From: Niklas Dewally Date: Sat, 18 Nov 2023 17:13:44 +0000 Subject: [PATCH 1/4] minion_rs: strict unwrap lints --- clippy.toml | 4 ++++ solvers/minion/Cargo.toml | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..470d39ae0 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests=true +allow-expect-in-tests=true + + diff --git a/solvers/minion/Cargo.toml b/solvers/minion/Cargo.toml index 40a4b6dca..895eb8492 100644 --- a/solvers/minion/Cargo.toml +++ b/solvers/minion/Cargo.toml @@ -13,7 +13,6 @@ cc = { version = "1.0.84", features = ["parallel"] } bindgen = "0.69.1" glob = "0.3.1" - -[lints] -workspace = true - +[lints.clippy] +unwrap_used = "deny" +expect_used = "deny" From c81078c2e9d170c319ff6072130399fe82a4df6e Mon Sep 17 00:00:00 2001 From: Niklas Dewally Date: Sun, 19 Nov 2023 13:29:13 +0000 Subject: [PATCH 2/4] update gitignore --- .gitignore | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 149 insertions(+), 5 deletions(-) diff --git a/.gitignore b/.gitignore index b15d45ac8..e2769d9d2 100644 --- a/.gitignore +++ b/.gitignore @@ -1,13 +1,157 @@ -# rust/c++ -target +*.log coverage + +## Rust +debug/ +target/ + +## C++ solvers/**/vendor/build +*.o +*.a -# python +## Python .env venv __pycache__ .ruff_cache -# IDE -.idea +## Global tool/OS ignores from : +## https://github.com/github/gitignore/blob/main/Global + +## LINUX +*~ + +# temporary files which can be created if a process still has a handle open of a deleted file +.fuse_hidden* + +# KDE directory preferences +.directory + +# Linux trash folder which might appear on any partition or disk +.Trash-* + +# .nfs files are created when an open file is removed but is still being accessed +.nfs* + +## MACOS +.DS_Store +.AppleDouble +.LSOverride + +# Icon must end with two \r +Icon + +# Thumbnails +._* + +# Files that might appear in the root of a volume +.DocumentRevisions-V100 +.fseventsd +.Spotlight-V100 +.TemporaryItems +.Trashes +.VolumeIcon.icns +.com.apple.timemachine.donotpresent + +# Directories potentially created on remote AFP share +.AppleDB +.AppleDesktop +Network Trash Folder +Temporary Items +.apdisk + +## VSCODE +.vscode/* +!.vscode/settings.json +!.vscode/tasks.json +!.vscode/launch.json +!.vscode/extensions.json +!.vscode/*.code-snippets + +# Local History for Visual Studio Code +.history/ + +# Built Visual Studio Code Extensions +*.vsix + + +## Jetbrains + +# Covers JetBrains IDEs: IntelliJ, RubyMine, PhpStorm, AppCode, PyCharm, CLion, Android Studio, WebStorm and Rider +# Reference: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839 + +# User-specific stuff +.idea/**/workspace.xml +.idea/**/tasks.xml +.idea/**/usage.statistics.xml +.idea/**/dictionaries +.idea/**/shelf + +# AWS User-specific +.idea/**/aws.xml + +# Generated files +.idea/**/contentModel.xml + +# Sensitive or high-churn files +.idea/**/dataSources/ +.idea/**/dataSources.ids +.idea/**/dataSources.local.xml +.idea/**/sqlDataSources.xml +.idea/**/dynamic.xml +.idea/**/uiDesigner.xml +.idea/**/dbnavigator.xml + +# Gradle +.idea/**/gradle.xml +.idea/**/libraries + +# Gradle and Maven with auto-import +# When using Gradle or Maven with auto-import, you should exclude module files, +# since they will be recreated, and may cause churn. Uncomment if using +# auto-import. +# .idea/artifacts +# .idea/compiler.xml +# .idea/jarRepositories.xml +# .idea/modules.xml +# .idea/*.iml +# .idea/modules +# *.iml +# *.ipr + +# CMake +cmake-build-*/ + +# Mongo Explorer plugin +.idea/**/mongoSettings.xml + +# File-based project format +*.iws + +# IntelliJ +out/ + +# mpeltonen/sbt-idea plugin +.idea_modules/ + +# JIRA plugin +atlassian-ide-plugin.xml + +# Cursive Clojure plugin +.idea/replstate.xml + +# SonarLint plugin +.idea/sonarlint/ + +# Crashlytics plugin (for Android Studio and IntelliJ) +com_crashlytics_export_strings.xml +crashlytics.properties +crashlytics-build.properties +fabric.properties + +# Editor-based Rest Client +.idea/httpRequests + +# Android studio 3.1+ serialized cache file +.idea/caches/build_file_checksums.ser From fe0de684ab58a49a028888e92357d6c592262a41 Mon Sep 17 00:00:00 2001 From: Niklas Dewally Date: Sun, 19 Nov 2023 14:04:44 +0000 Subject: [PATCH 3/4] minion_rs: do not lint unwrap or expects in build.rs --- solvers/minion/build.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/solvers/minion/build.rs b/solvers/minion/build.rs index 3479b8bff..2afe7ec40 100755 --- a/solvers/minion/build.rs +++ b/solvers/minion/build.rs @@ -2,7 +2,8 @@ // - https://github.com/gokberkkocak/rust_glucose/blob/master/build.rs // - https://rust-lang.github.io/rust-bindgen/non-system-libraries.html // - https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed -// +#![allow(clippy::unwrap_used)] +#![allow(clippy::expect_used)] use std::env; use std::path::PathBuf; use std::process::Command; From fb0a4185057aeb9b56ca7702ef9b959da4148cfa Mon Sep 17 00:00:00 2001 From: Niklas Dewally Date: Sun, 19 Nov 2023 14:54:02 +0000 Subject: [PATCH 4/4] minion_rs: better error handling, and clippy fixes --- Cargo.lock | 7 ++ Cargo.toml | 1 + solvers/minion/Cargo.toml | 4 + solvers/minion/build.rs | 4 +- solvers/minion/src/ast.rs | 17 ++-- solvers/minion/src/error.rs | 6 +- solvers/minion/src/run.rs | 146 +++++++++++++++++++------------ solvers/minion/src/scoped_ptr.rs | 6 +- 8 files changed, 124 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5b4c30b54..8ea4f70d3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,6 +11,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "anyhow" +version = "1.0.75" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a4668cab20f66d8d020e1fbc0ebe47217433c1b6c8f2040faf858554e394ace6" + [[package]] name = "bindgen" version = "0.64.0" @@ -249,6 +255,7 @@ checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" name = "minion_rs" version = "0.0.1" dependencies = [ + "anyhow", "bindgen 0.69.1", "cc", "glob", diff --git a/Cargo.toml b/Cargo.toml index 06366bacd..9e51faff1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,4 +1,5 @@ [workspace] +resolver = "2" members = [ "conjure_oxide", "solvers/kissat", diff --git a/solvers/minion/Cargo.toml b/solvers/minion/Cargo.toml index 895eb8492..d1600f5c6 100644 --- a/solvers/minion/Cargo.toml +++ b/solvers/minion/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] +anyhow = "1.0.75" thiserror = "1.0.50" [build-dependencies] @@ -16,3 +17,6 @@ glob = "0.3.1" [lints.clippy] unwrap_used = "deny" expect_used = "deny" +panic_in_result_fn = "warn" +panic = "warn" +todo = "warn" diff --git a/solvers/minion/build.rs b/solvers/minion/build.rs index 2afe7ec40..b43de719d 100755 --- a/solvers/minion/build.rs +++ b/solvers/minion/build.rs @@ -4,6 +4,8 @@ // - https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed #![allow(clippy::unwrap_used)] #![allow(clippy::expect_used)] +#![allow(clippy::panic)] + use std::env; use std::path::PathBuf; use std::process::Command; @@ -61,7 +63,7 @@ fn bind() { .header("vendor/minion/libwrapper.h") // Tell cargo to invalidate the built crate whenever any of the // included header files changed. - .parse_callbacks(Box::new(bindgen::CargoCallbacks)) + .parse_callbacks(Box::new(bindgen::CargoCallbacks::new())) // Make all templates opaque as reccomended by bindgen .opaque_type("std::.*") // Manually allow C++ functions to stop bindgen getting confused. diff --git a/solvers/minion/src/ast.rs b/solvers/minion/src/ast.rs index d89cc0f69..f4ff942d5 100644 --- a/solvers/minion/src/ast.rs +++ b/solvers/minion/src/ast.rs @@ -19,6 +19,13 @@ impl Model { } } +impl Default for Model { + fn default() -> Self { + Self::new() + } +} + +#[derive(Debug)] pub enum Constraint { SumLeq(Vec, Var), SumGeq(Vec, Var), @@ -30,6 +37,7 @@ pub enum Constraint { /// The latter is not stored in the symbol table, or counted in Minions internal list of all /// variables, but is used to allow the use of a constant in the place of a variable in a /// constraint. +#[derive(Debug)] pub enum Var { NameRef(VarName), ConstantAsVar(i32), @@ -41,7 +49,7 @@ pub enum Constant { Integer(i32), } -#[derive(Copy, Clone)] +#[derive(Debug, Copy, Clone)] pub enum VarDomain { Bound(i32, i32), Discrete(i32, i32), @@ -74,14 +82,11 @@ impl SymbolTable { self.table.insert(name.clone(), vartype); self.var_order.push(name); - return Some(()); + Some(()) } pub fn get_vartype(&self, name: VarName) -> Option { - match self.table.get(&name) { - Some(m) => Some(*m), - None => None, - } + self.table.get(&name).cloned() } pub fn get_variable_order(&self) -> Vec { diff --git a/solvers/minion/src/error.rs b/solvers/minion/src/error.rs index 86807ac3d..d183da6c5 100644 --- a/solvers/minion/src/error.rs +++ b/solvers/minion/src/error.rs @@ -8,9 +8,13 @@ use thiserror::Error; /// `Error` allows functions involving Minion to return a single error type. All error types in /// `minion_rs` are able to be converted into this type using into / from. #[derive(Debug, Error)] -pub enum Error { +pub enum MinionError { #[error("runtime error: `{0}.to_string()`")] RuntimeError(#[from] RuntimeError), + #[error("not implemented: {0}")] + NotImplemented(String), + #[error(transparent)] + Other(#[from] anyhow::Error), // source and Display delegate to anyhow::Error } /// RuntimeErrors are thrown by Minion during execution. diff --git a/solvers/minion/src/run.rs b/solvers/minion/src/run.rs index f817aa4ef..12da64e50 100644 --- a/solvers/minion/src/run.rs +++ b/solvers/minion/src/run.rs @@ -5,6 +5,7 @@ use std::{ }; use crate::{ast::*, error::*, raw_bindings::*, scoped_ptr::Scoped}; +use anyhow::anyhow; // TODO: allow passing of options. @@ -31,14 +32,13 @@ use crate::{ast::*, error::*, raw_bindings::*, scoped_ptr::Scoped}; /// fn callback(solutions: HashMap) -> bool { /// let mut guard = ALL_SOLUTIONS.lock().unwrap(); /// guard.push(solutions); -/// return true; +/// true /// } /// /// // Build and run the model. /// let mut model = Model::new(); /// /// // ... omitted for brevity ... -/// # /// # model /// # .named_variables /// # .add_var("x".to_owned(), VarDomain::Bound(1, 3)); @@ -82,8 +82,7 @@ use crate::{ast::*, error::*, raw_bindings::*, scoped_ptr::Scoped}; /// /// // Get solutions /// let guard = ALL_SOLUTIONS.lock().unwrap(); -/// -/// let solution_set_1 = &(*guard.get(0).unwrap()); +/// let solution_set_1 = &(guard.get(0).unwrap()); /// /// let x1 = solution_set_1.get("x").unwrap(); /// let y1 = solution_set_1.get("y").unwrap(); @@ -111,6 +110,8 @@ unsafe extern "C" fn run_callback() -> bool { // get printvars from static PRINT_VARS if they exist. // if not, return true and continue search. + // Mutex poisoning is probably panic worthy. + #[allow(clippy::unwrap_used)] let mut guard: MutexGuard<'_, Option>> = PRINT_VARS.lock().unwrap(); if guard.is_none() { @@ -122,7 +123,7 @@ unsafe extern "C" fn run_callback() -> bool { None => unreachable!(), }; - if print_vars.len() == 0 { + if print_vars.is_empty() { return true; } @@ -130,11 +131,12 @@ unsafe extern "C" fn run_callback() -> bool { let mut solutions: HashMap = HashMap::new(); for (i, var) in print_vars.iter().enumerate() { - let solution_int: i32 = printMatrix_getValue(i as _).try_into().unwrap(); + let solution_int: i32 = printMatrix_getValue(i as _); let solution: Constant = Constant::Integer(solution_int); solutions.insert(var.to_string(), solution); } + #[allow(clippy::unwrap_used)] match *CALLBACK.lock().unwrap() { None => true, Some(func) => func(solutions), @@ -144,27 +146,32 @@ unsafe extern "C" fn run_callback() -> bool { /// Run Minion on the given [Model]. /// /// The given [callback](Callback) is ran whenever a new solution set is found. -pub fn run_minion(model: Model, callback: Callback) -> Result<(), RuntimeError> { + +// Turn it into a warning for this function, cant unwarn it directly above callback wierdness +#[allow(clippy::unwrap_used)] +pub fn run_minion(model: Model, callback: Callback) -> Result<(), MinionError> { + // Mutex poisoning is probably panic worthy. *CALLBACK.lock().unwrap() = Some(callback); unsafe { let options = Scoped::new(newSearchOptions(), |x| searchOptions_free(x as _)); let args = Scoped::new(newSearchMethod(), |x| searchMethod_free(x as _)); - let instance = Scoped::new(convert_model_to_raw(&model), |x| instance_free(x as _)); - let res = runMinion(options.ptr, args.ptr, instance.ptr, Some(run_callback)); + let instance = Scoped::new(newInstance(), |x| instance_free(x as _)); + convert_model_to_raw(&instance, &model)?; + + let res = runMinion(options.ptr, args.ptr, instance.ptr, Some(run_callback)); match res { 0 => Ok(()), - x => Err(RuntimeError::from(x)), + x => Err(MinionError::from(RuntimeError::from(x))), } } } -/// Callee owns the returned instance -unsafe fn convert_model_to_raw(model: &Model) -> *mut ProbSpec_CSPInstance { - // This is managed in scope by the callee - let instance = newInstance(); - +unsafe fn convert_model_to_raw( + instance: &Scoped, + model: &Model, +) -> Result<(), MinionError> { /*******************************/ /* Add variables */ /*******************************/ @@ -181,37 +188,44 @@ unsafe fn convert_model_to_raw(model: &Model) -> *mut ProbSpec_CSPInstance { let search_vars = Scoped::new(vec_var_new(), |x| vec_var_free(x as _)); // store variables and the order they will be returned inside rust for later use. + #[allow(clippy::unwrap_used)] let mut print_vars_guard = PRINT_VARS.lock().unwrap(); *print_vars_guard = Some(vec![]); for var_name in model.named_variables.get_variable_order() { - //TODO: make this return Result - let c_str = CString::new(var_name.clone()).expect(""); + let c_str = CString::new(var_name.clone()).map_err(|_| { + anyhow!( + "Variable name {:?} contains a null character.", + var_name.clone() + ) + })?; let vartype = model .named_variables .get_vartype(var_name.clone()) - .expect(""); + .ok_or(anyhow!("Could not get var type for {:?}", var_name.clone()))?; let (vartype_raw, domain_low, domain_high) = match vartype { - VarDomain::Bound(a, b) => (VariableType_VAR_BOUND, a, b), - _ => panic!("NOT IMPLEMENTED"), - }; + VarDomain::Bound(a, b) => Ok((VariableType_VAR_BOUND, a, b)), + x => Err(MinionError::NotImplemented(format!("{:?}", x))), + }?; newVar_ffi( - instance, + instance.ptr, c_str.as_ptr() as _, vartype_raw, domain_low, domain_high, ); - let var = getVarByName(instance, c_str.as_ptr() as _); + let var = getVarByName(instance.ptr, c_str.as_ptr() as _); - printMatrix_addVar(instance, var); + printMatrix_addVar(instance.ptr, var); // add to the print vars stored in rust so to remember // the order for callback function. + + #[allow(clippy::unwrap_used)] (*print_vars_guard).as_mut().unwrap().push(var_name.clone()); vec_var_push_back(search_vars.ptr, var); @@ -222,9 +236,7 @@ unsafe fn convert_model_to_raw(model: &Model) -> *mut ProbSpec_CSPInstance { |x| searchOrder_free(x as _), ); - // this and other instance_ functions does not move so my use of ptrs are ok - // TODO (nd60): document this - instance_addSearchOrder(instance, search_order.ptr); + instance_addSearchOrder(instance.ptr, search_order.ptr); /*********************************/ /* Add constraints */ @@ -235,25 +247,28 @@ unsafe fn convert_model_to_raw(model: &Model) -> *mut ProbSpec_CSPInstance { // 2. run through arguments and add them to the constraint // 3. add constraint to instance - let constraint_type = get_constraint_type(constraint); + let constraint_type = get_constraint_type(constraint)?; let raw_constraint = Scoped::new(newConstraintBlob(constraint_type), |x| { constraint_free(x as _) }); - constraint_add_args(instance, raw_constraint.ptr, constraint); - instance_addConstraint(instance, raw_constraint.ptr); + constraint_add_args(instance.ptr, raw_constraint.ptr, constraint)?; + instance_addConstraint(instance.ptr, raw_constraint.ptr); } - return instance; + Ok(()) } -unsafe fn get_constraint_type(constraint: &Constraint) -> u32 { +unsafe fn get_constraint_type(constraint: &Constraint) -> Result { match constraint { - Constraint::SumGeq(_, _) => ConstraintType_CT_GEQSUM, - Constraint::SumLeq(_, _) => ConstraintType_CT_LEQSUM, - Constraint::Ineq(_, _, _) => ConstraintType_CT_INEQ, + Constraint::SumGeq(_, _) => Ok(ConstraintType_CT_GEQSUM), + Constraint::SumLeq(_, _) => Ok(ConstraintType_CT_LEQSUM), + Constraint::Ineq(_, _, _) => Ok(ConstraintType_CT_INEQ), #[allow(unreachable_patterns)] - _ => panic!("NOT IMPLEMENTED"), + x => Err(MinionError::NotImplemented(format!( + "Constraint not implemented {:?}", + x, + ))), } } @@ -261,24 +276,27 @@ unsafe fn constraint_add_args( i: *mut ProbSpec_CSPInstance, r_constr: *mut ProbSpec_ConstraintBlob, constr: &Constraint, -) { +) -> Result<(), MinionError> { match constr { Constraint::SumGeq(lhs_vars, rhs_var) => { - read_vars(i, r_constr, &lhs_vars); - read_var(i, r_constr, rhs_var) + read_vars(i, r_constr, lhs_vars)?; + read_var(i, r_constr, rhs_var)?; + Ok(()) } Constraint::SumLeq(lhs_vars, rhs_var) => { - read_vars(i, r_constr, &lhs_vars); - read_var(i, r_constr, rhs_var) + read_vars(i, r_constr, lhs_vars)?; + read_var(i, r_constr, rhs_var)?; + Ok(()) } Constraint::Ineq(var1, var2, c) => { - read_var(i, r_constr, &var1); - read_var(i, r_constr, &var2); - read_const(r_constr, c) + read_var(i, r_constr, var1)?; + read_var(i, r_constr, var2)?; + read_const(r_constr, c)?; + Ok(()) } #[allow(unreachable_patterns)] - _ => panic!("NOT IMPLEMENTED"), - }; + x => Err(MinionError::NotImplemented(format!("{:?}", x))), + } } // DO NOT call manually - this assumes that all needed vars are already in the symbol table. @@ -287,13 +305,17 @@ unsafe fn read_vars( instance: *mut ProbSpec_CSPInstance, raw_constraint: *mut ProbSpec_ConstraintBlob, vars: &Vec, -) { +) -> Result<(), MinionError> { let raw_vars = Scoped::new(vec_var_new(), |x| vec_var_free(x as _)); for var in vars { - // TODO: could easily break and segfault and die and so on let raw_var = match var { Var::NameRef(name) => { - let c_str = CString::new(name.clone()).expect(""); + let c_str = CString::new(name.clone()).map_err(|_| { + anyhow!( + "Variable name {:?} contains a null character.", + name.clone() + ) + })?; getVarByName(instance, c_str.as_ptr() as _) } Var::ConstantAsVar(n) => constantAsVar(*n), @@ -303,17 +325,24 @@ unsafe fn read_vars( } constraint_addVarList(raw_constraint, raw_vars.ptr); + + Ok(()) } unsafe fn read_var( instance: *mut ProbSpec_CSPInstance, raw_constraint: *mut ProbSpec_ConstraintBlob, var: &Var, -) { +) -> Result<(), MinionError> { let raw_vars = Scoped::new(vec_var_new(), |x| vec_var_free(x as _)); let raw_var = match var { Var::NameRef(name) => { - let c_str = CString::new(name.clone()).expect(""); + let c_str = CString::new(name.clone()).map_err(|_| { + anyhow!( + "Variable name {:?} contains a null character.", + name.clone() + ) + })?; getVarByName(instance, c_str.as_ptr() as _) } Var::ConstantAsVar(n) => constantAsVar(*n), @@ -321,16 +350,23 @@ unsafe fn read_var( vec_var_push_back(raw_vars.ptr, raw_var); constraint_addVarList(raw_constraint, raw_vars.ptr); + + Ok(()) } -unsafe fn read_const(raw_constraint: *mut ProbSpec_ConstraintBlob, constant: &Constant) { +unsafe fn read_const( + raw_constraint: *mut ProbSpec_ConstraintBlob, + constant: &Constant, +) -> Result<(), MinionError> { let raw_consts = Scoped::new(vec_int_new(), |x| vec_var_free(x as _)); let val = match constant { - Constant::Integer(n) => n, - _ => panic!("NOT IMPLEMENTED"), - }; + Constant::Integer(n) => Ok(n), + x => Err(MinionError::NotImplemented(format!("{:?}", x))), + }?; vec_int_push_back(raw_consts.ptr, *val); constraint_addConstantList(raw_constraint, raw_consts.ptr); + + Ok(()) } diff --git a/solvers/minion/src/scoped_ptr.rs b/solvers/minion/src/scoped_ptr.rs index a7b328894..756750ef4 100644 --- a/solvers/minion/src/scoped_ptr.rs +++ b/solvers/minion/src/scoped_ptr.rs @@ -14,15 +14,13 @@ pub struct Scoped { // instead impl Scoped { pub unsafe fn new(ptr: *mut T, destructor: fn(*mut T)) -> Scoped { - return Scoped { ptr, destructor }; + Scoped { ptr, destructor } } } // https://doc.rust-lang.org/nomicon/destructors.html impl Drop for Scoped { fn drop(&mut self) { - unsafe { - (self.destructor)(self.ptr); - } + (self.destructor)(self.ptr); } }