Skip to content

Commit

Permalink
fix: empty or([]) being parsed as nothing
Browse files Browse the repository at this point in the history 
Constant matrices inside operations were not recognised by our parser,
so were ignored. This includes expressions like or([]) and
min([1,2,3]).

- Add parsing support for constant matrices inside operations.

- Add support for top level "false" and "true" with Minion. Ideally,
  these models would never get passed to the solver; however, this would
  require solution evaluation and a refactor of our tester, so I have
  left this for future work.

Fixes #366.
  • Loading branch information
@niklasdewally
niklasdewally committed Oct 14, 2024
1 parent 989876b commit c959e13
Show file tree
Hide file tree
Showing 10 changed files with 332 additions and 31 deletions.
6 changes: 3 additions & 3 deletions conjure_oxide/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ pub fn main() -> AnyhowResult<()> {
.append(true)
.open("conjure_oxide.log")?;

//Builder::with_level("Trace")
Builder::new()
Builder::with_level("TRACE")
//Builder::new()
.with_target_writer("info", new_writer(stdout()))
.with_target_writer("file", new_writer(log_file))
.with_target_writer("file,jsonparser", new_writer(log_file))
.init();

if target_family != SolverFamily::Minion {
Expand Down
7 changes: 7 additions & 0 deletions conjure_oxide/tests/integration/bugs/issue-336-empty-or-should-be-false/input.essence
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

$ "By mathematical tradition (and Essence semantics), an 'empty or' is false, however, this model has 5 answers (one for each value of a)"
$ -- https://github.com/conjure-cp/conjure-oxide/issues/366

find a : int(1..5)
such that
or([])
1 change: 1 addition & 0 deletions .../integration/bugs/issue-336-empty-or-should-be-false/input.expected-minion.solutions.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[]
31 changes: 31 additions & 0 deletions .../integration/bugs/issue-336-empty-or-should-be-false/input.expected-parse.serialised.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"constraints": {
"Or": [
{
"clean": false,
"etype": null
},
[]
]
},
"next_var": 0,
"variables": [
[
{
"UserName": "a"
},
{
"domain": {
"IntDomain": [
{
"Bounded": [
1,
5
]
}
]
}
}
]
]
}
33 changes: 33 additions & 0 deletions ...ntegration/bugs/issue-336-empty-or-should-be-false/input.expected-rewrite.serialised.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"constraints": {
"Constant": [
{
"clean": false,
"etype": null
},
{
"Bool": false
}
]
},
"next_var": 0,
"variables": [
[
{
"UserName": "a"
},
{
"domain": {
"IntDomain": [
{
"Bounded": [
1,
5
]
}
]
}
}
]
]
}
64 changes: 64 additions & 0 deletions ...sts/integration/eprime-minion/partial-eval-02-or-and/input.expected-parse.serialised.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,70 @@
]
]
},
{
"Or": [
{
"clean": false,
"etype": null
},
[
{
"AllDiff": [
{
"clean": false,
"etype": null
},
[
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 1
}
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 2
}
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 3
}
]
}
]
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Bool": true
}
]
}
]
]
},
{
"And": [
{
Expand Down
64 changes: 64 additions & 0 deletions .../tests/integration/eprime-minion/partial-eval-or-and/input.expected-parse.serialised.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,70 @@
]
]
},
{
"Or": [
{
"clean": false,
"etype": null
},
[
{
"AllDiff": [
{
"clean": false,
"etype": null
},
[
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 1
}
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 2
}
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Int": 3
}
]
}
]
]
},
{
"Constant": [
{
"clean": false,
"etype": null
},
{
"Bool": true
}
]
}
]
]
},
{
"And": [
{
Expand Down
78 changes: 70 additions & 8 deletions crates/conjure_core/src/parse/parse_model.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ use crate::error::{Error, Result};
use crate::metadata::Metadata;
use crate::Model;

macro_rules! parser_trace {
($($arg:tt)+) => {
log::trace!(target:"jsonparser",$($arg)+)
};
}

macro_rules! parser_debug {
($($arg:tt)+) => {
log::debug!(target:"jsonparser",$($arg)+)
};
}

pub fn model_from_json(str: &str, context: Arc<RwLock<Context<'static>>>) -> Result<Model> {
let mut m = Model::new_empty(context);
let v: JsonValue = serde_json::from_str(str)?;
Expand Down Expand Up @@ -240,6 +252,10 @@ fn parse_expression(obj: &JsonValue) -> Option<Expression> {
))
}
Value::Object(constant) if constant.contains_key("Constant") => parse_constant(constant),
Value::Object(constant) if constant.contains_key("ConstantInt") => parse_constant(constant),
Value::Object(constant) if constant.contains_key("ConstantBool") => {
parse_constant(constant)
}
otherwise => panic!("Unhandled Expression {:#?}", otherwise),
}
}
Expand Down Expand Up @@ -282,24 +298,46 @@ fn parse_vec_op(
let (key, value) = vec_op.into_iter().next()?;
let constructor = vec_operators.get(key.as_str())?;

let args_parsed: Vec<Option<Expression>> = value["AbstractLiteral"]["AbsLitMatrix"][1]
.as_array()?
.iter()
.map(parse_expression)
.collect();
parser_debug!("Trying to parse vec_op: {key} ...");

let mut args_parsed: Option<Vec<Option<Expression>>> = None;
if let Some(abs_lit_matrix) = value.pointer("/AbstractLiteral/AbsLitMatrix/1") {
parser_trace!("... containing a matrix of literals");
args_parsed = abs_lit_matrix.as_array().map(|x| {
x.iter()
.map(parse_expression)
.collect::<Vec<Option<Expression>>>()
});
}
// the input of this expression is constant - e.g. or([]), or([false]), min([2]), etc.
else if let Some(const_abs_lit_matrix) =
value.pointer("/Constant/ConstantAbstract/AbsLitMatrix/1")
{
parser_trace!("... containing a matrix of constants");
args_parsed = const_abs_lit_matrix.as_array().map(|x| {
x.iter()
.map(parse_expression)
.collect::<Vec<Option<Expression>>>()
});
}

let args_parsed = args_parsed?;

let number_of_args = args_parsed.len();
parser_debug!("... with {number_of_args} args {args_parsed:#?}");

let valid_args: Vec<Expression> = args_parsed.into_iter().flatten().collect();
if number_of_args != valid_args.len() {
None
} else {
parser_debug!("... success!");
Some(constructor(Metadata::new(), valid_args))
}
}

fn parse_constant(constant: &serde_json::Map<String, Value>) -> Option<Expression> {
match &constant["Constant"] {
Value::Object(int) if int.contains_key("ConstantInt") => {
match &constant.get("Constant") {
Some(Value::Object(int)) if int.contains_key("ConstantInt") => {
let int_32: i32 = match int["ConstantInt"].as_array()?[1].as_i64()?.try_into() {
Ok(x) => x,
Err(_) => {
Expand All @@ -314,10 +352,34 @@ fn parse_constant(constant: &serde_json::Map<String, Value>) -> Option<Expressio
Some(Expression::Constant(Metadata::new(), Constant::Int(int_32)))
}

Value::Object(b) if b.contains_key("ConstantBool") => {
Some(Value::Object(b)) if b.contains_key("ConstantBool") => {
let b: bool = b["ConstantBool"].as_bool().unwrap();
Some(Expression::Constant(Metadata::new(), Constant::Bool(b)))
}

// sometimes (e.g. constant matrices) we can have a ConstantInt / Constant bool that is
// not wrapped in Constant
None => {
let int_expr = constant["ConstantInt"]
.as_array()
.and_then(|x| x[1].as_i64())
.and_then(|x| x.try_into().ok())
.map(|x| Expression::Constant(Metadata::new(), Constant::Int(x)));

if let e @ Some(_) = int_expr {
return e;
}

let bool_expr = constant["ConstantBool"]
.as_bool()
.map(|x| Expression::Constant(Metadata::new(), Constant::Bool(x)));

if let e @ Some(_) = bool_expr {
return e;
}

panic!("Unhandled parse_constant {:#?}", constant);
}
otherwise => panic!("Unhandled parse_constant {:#?}", otherwise),
}
}
Loading

0 comments on commit c959e13

Please sign in to comment.