-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Limited functions #54
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! I've left mostly documentation- and cosmetic-related comments. We can discuss the theoretical questions in our next meeting.
src/opt/fuzz_test.rs
Outdated
@@ -198,7 +198,7 @@ fn prove_equiv(expr: Expr, optimized: Expr, tcx: &TyCtx) -> TestCaseResult { | |||
optimized.clone(), | |||
); | |||
let ctx = z3::Context::new(&z3::Config::new()); | |||
let smt_ctx = SmtCtx::new(&ctx, tcx); | |||
let smt_ctx = SmtCtx::new(&ctx, tcx, false, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These additional parameters are rather mysterious here. I suggest just adding a new type like SmtCtxOptions
to capture those options.
src/smt/mod.rs
Outdated
.map(|param| ty_to_sort(self, ¶m.ty)) | ||
.collect(); | ||
|
||
let mut domain = if self.is_limited_function_decl(&func) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe capture this functionality in some method somewhere else in a section/module of code specifically for the fuel transformation?
src/smt/mod.rs
Outdated
translate.pop(); | ||
} | ||
|
||
let mut add_defining_axiom = |name: Ident, lhs: &Expr, rhs: &Expr| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, maybe capture this in another method?
src/smt/mod.rs
Outdated
// Defining axiom | ||
add_defining_axiom(func.name, &app, body); // TODO: create a new name for the axiom | ||
|
||
// Computing axiom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, capture in another method. And please add comments showing roughly what the generated axiom(s) look like.
src/smt/mod.rs
Outdated
@@ -204,3 +299,36 @@ fn ty_to_sort<'ctx>(ctx: &SmtCtx<'ctx>, ty: &TyKind) -> Sort<'ctx> { | |||
} | |||
} | |||
} | |||
|
|||
pub enum Lit<'ctx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment how this wraps z3rro's Lit type? And also explain what this is and why it's needed.
src/smt/translate_exprs.rs
Outdated
@@ -175,18 +207,24 @@ impl<'smt, 'ctx> TranslateExprs<'smt, 'ctx> { | |||
panic!("illegal cast to {:?} from {:?}", &expr.ty, &operand.ty) | |||
} | |||
ExprKind::Quant(quant_op, quant_vars, ann, operand) => { | |||
self.push(); // avoid leaking any fresh quant_vars into the outside scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this now needed? Why wasn't this an issue before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I relied heavily on the functionality of the SmtScope
for creating fresh quantified variables (creating the quantified fuel on demand). The quantifier translation does actually not use this feature. Instead, a scope with all quantified variables is manually created. The problem is that mk_scope
calls get_local
which calls init_local
if the variable is new (this is always the case for mk_scope
) and init_local
automatically adds the new variable to the current scope. Therefore, we have the variables twice. Once in the current scope and once in the scope created by mk_scope
.
This is my easy "fix". Maybe this is actually a problem with mk_scope
or with my understanding of the translation code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explanation does make some sense to me. But I just do not understand why this issue wouldn't have shown up before already. Wouldn't any call to mk_scope
always create the same variables on the outer quantifier as well?
Could you maybe create a quick example HeyVL file with (nested?) quantifiers where this problem does/does not show up in the SMT output?
Also if this change is correct, definitely don't forget the case for t_eureal
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested quantifiers in heyvl are not a problem since they are created with mk_scope
and the variables are fixed. The problem only occurs if the capturing of fresh variables and mk_scope
is used together. This was previously not the case as the axioms were first build as heyvl expressions and then translated.
Regarding the fix: I'm not super happy and we might need to talk about it. I think it is not conceptually sound and there are theoretically situations where the fuel translation fails here. For example, if the fuel parameter is first needed inside a heyvl quantifier, the fuel variable is created and added to the current "dummy scope", which is then dropped after the quantifier translation. This can't happen currently since we always translate the function application (lhs) first which needs the fuel variable.
src/smt/translate_exprs.rs
Outdated
}; | ||
self.locals.insert(ident, local); | ||
} | ||
|
||
fn get_maybe_const_local(&mut self, ident: Ident) -> Symbolic<'ctx> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very unsure what this method does or why it's needed?
src/smt/translate_exprs.rs
Outdated
} | ||
} | ||
|
||
#[derive(Default)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment saying that this is a lazily initialized fuel's ScopeSymbolic
. Explain where it is initialized and when not.
z3rro/src/lit.rs
Outdated
func: FuncDef<'ctx>, | ||
} | ||
|
||
/// Identity function that is used to mark constants values. They allow for axioms instantiation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this comment on the struct, not on the impl. Say that it is parameterized over the Z3 Sort of the literal.
z3rro/src/prover.rs
Outdated
pub fn enforce_ematching(&mut self) { | ||
let mut params = Params::new(self.solver.get_context()); | ||
params.set_bool("auto-config", false); | ||
params.set_bool("smt.mbqi", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add this to the Rustdoc documentation of the Fuel
type or somewhere that preventing instantiations by the SMT solver of the fuel type is necessary and that disabling MBQI accomplishes that.
Thanks for the feedback! I think I address most of it. Regarding the |
src/smt/translate_exprs.rs
Outdated
@@ -98,8 +98,13 @@ impl<'smt, 'ctx> TranslateExprs<'smt, 'ctx> { | |||
&mut self.fuel_context | |||
} | |||
|
|||
pub fn add_constant_exprs(&mut self, constant_vars: &[Ident], expr_to_analyse: &mut Expr) { | |||
let mut collector = ConstantExprCollector::new(constant_vars); | |||
pub fn add_constant_exprs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I think the proper phrase would be "literal" instead of constant. And this function needs a comment explaining why it exists.
Previously, the limited functions had no effect, since z3 made up fuel parameters. Forcing z3 to only use ematching (by disabling mbqi) has the desired effect of forcing it to fail fast.
- Added command line options for generating limited-functions and enforcing ematching. - Fix some bugs regarding nested quantifiers
The SMT-lib output (without any extra flags) is now the same as previously (main) except that the definitional axiom has a patter
Reverts back to explicitly enumerate the quantified variables through `mk_scope`. Therefor, the need for the `push()` and `pop()` operations is no longer necessary. Now also correctly encodes calls of limited functions in user defined axioms.
This allows approach allows us to limit user defined with triggers in a sensible way
calls to limited function outside the generated axioms are always translated with a fixed fuel value (i.e. call context). Also remove a non-working examples from the tests.
- the trigger for the computation axiom was wrong - lit wrapping was not always applied properly - define lit functions as uninterpreted functions + axiom to avoid them being removed during preprocessing
Some tests where failing after constructing foralls differently
A function call is now only consider to be constant if - all arguments are constant - if the function has a definition <-- new
The ability to lit wrap is now a trait that a datatype must implement. This enables different specialisations per datatype. The main motivation is to avoid boxing the pair EUReal representation into a single value before passing it into the lit function. The bool and real are now wrapped separately.
This should avoid triggering the z3 incremental solver
This reverts commit 669d70c.
Parameters set with `Solver::set_params` get ignored without a `Solver::push`
In the process the fuel encoding code was completely restructured
This PR implements limited functions from the paper "Computing with an SMT Solver".
If enabled, the transformation is only applied to functions that have a direct definition, i.e. of the form
func func_name(<args>): <returnType> = <def>
.Three new command line options have been added
--limited-functions
implicitly adds afuel
parameter to all applicable functions and restricts the defining axiom accordingly.--force-ematching
forces e-matching by disabling mbqi. Otherwise the different encoding has no effect.--lit-wrap
adds aLit
marker around constant values and a version of the defining axiom for constant values, that does not consume any fuel.Open TODOs and questions
TODO
s / questions in the codeLit
-wrapping is not propagated (Lit(1) + Lit(2) -> Lit(1 + 2)
orFac(Lit(3)) -> Lit(Fac(Lit(3)))
). I don't have a good idea here.SmtVcUnit::simplify
removesLit
wrapper from the verification condition. Hence,--no-simplify
is currently also needed.coupon-collector.heyvl
no longer verifies with everything new disabled -.\tests/coupon-collector.heyvl::inner_loop: Unknown result! (reason: (incomplete (theory array)))
The generated SMT-lib code is now identical (except for patterns, but removing them does also not change the following fact) but z3 does no longer terminate