-
Notifications
You must be signed in to change notification settings - Fork 60
Variants #1622
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
base: master
Are you sure you want to change the base?
Variants #1622
Conversation
6f86486
to
88cb570
Compare
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.
Great job!
All in all, I think that most of this is OK, but the way variant assertion are inserted need to be improved:
-
For loops, it should really rely on the WTO, and therefore I think that this should really be done by the backend. In particular, I'm not sure that we should store sets of variants to check at "continue" blocks. Instead, when entering a WTO component whose head has a variant, create a new handler for the head of the loop that will check the variant decrease, and make all the jumps from inside the component point to this new handler.
-
For recursive calls in program code, I would really prefer not translating operands to terms. This would be yet another translation from places to XXX, and I'm afraid of subtle semantic differences betwen the versions. Instead, please use a method similar to the one for loop I propose above: create a new sub-handler for recursive calls, and make it check that the variant decreases before performing the actual recursive call.
self >= 0 && self > other | ||
} | ||
|
||
#[trusted] // FIXME: find the right why3 lemma |
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 don't think that this lemma exists. But it's easy by induction on s[0]
, and you have access to that in Why3 IDE.
@@ -91,7 +90,7 @@ impl Condition { | |||
pub struct Contract { | |||
pub requires: Box<[Condition]>, | |||
pub ensures: Box<[Condition]>, | |||
pub variant: Option<Exp>, | |||
pub variant: Option<(Exp, Type)>, |
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.
What's the purpose of this change? It seems like the type is not used anywhere in the why3
crate.
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.
It's used in creusot/src/backend/program.rs
, when defining the variable that holds the variant.
creusot/src/gather_spec_closures.rs
Outdated
let res = !self.before_loop.contains(*p); | ||
if !res && std::mem::replace(&mut has_met_before_loop, true) { | ||
self.ctx.dcx().span_bug(self.body.span, "a basic block is marked as the entry for multiple loops: this should not happen.") | ||
} | ||
res |
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.
That's very fragile. This is essentially assuming that a loop header is always following immediately a block annotated with before_loop
. Thus, this rely on rustc not inserting intermediary no-op blocks, etc...
I don't think this is a good design. predecessors should be classified based on the WTO.
creusot/src/translation/fmir.rs
Outdated
@@ -65,6 +64,47 @@ impl<'tcx> Place<'tcx> { | |||
None | |||
} | |||
} | |||
|
|||
fn into_term(self, tcx: TyCtxt<'tcx>, locals: &LocalDecls<'tcx>, span: Span) -> Term<'tcx> { |
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 don't like this much, both because this means that we are introducing yet another translation for places (with maybe subtle semantic differences), and because the resulting code will have some duplication.
Instead, could we not add to FMir the possibility to annotate calls with a variant to check, before substituting the parameters ? In the backend, it should be easy to check the variant when translating the call.
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.
An alternative is to generate a separate handlers for recursive calls, with one additional precondition: the fact that the variant decreased.
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 did your second solution, which is somewhat similar to loop variants.
}; | ||
} | ||
|
||
// detect mutual recursion | ||
let cycles = tarjan_scc(&call_graph); | ||
for cycle in cycles { | ||
// FIXME: if there is a variant on all the functions in the cycle, then | ||
// - Insert the recursive call in `recursive_calls` |
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.
Somehow we eed to check that variants all have the same type. This is tricky because this involves substitutions.
@@ -450,6 +478,11 @@ impl CallGraph { | |||
if !(is_pearlite(ctx.tcx, def_id) || ctx.sig(def_id).contract.terminates) { | |||
// Only consider functions marked with `terminates`: we already ensured | |||
// that a `terminates` functions only calls other `terminates` functions. | |||
if let Some(variant) = &ctx.sig(def_id).contract.variant { | |||
ctx.dcx().struct_span_warn(variant.span, "variant will be ignored") |
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.
ignored, really ? It seems to me that the variant will be checked, but it is useless to prove the termination of the function.
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.
It is ignored, because we don't even do the analysis in this case. Thus we don't know what recursive calls the function will make, which is needed to know where to insert the variant checks.
ddf49af
to
32bed6d
Compare
32bed6d
to
7f93023
Compare
7f93023
to
3d9bc4e
Compare
This allow variants in various places:
Together with a rework of the
WellFounded
trait, to allow variants on custom types.Fixes #561, #754.