-
Notifications
You must be signed in to change notification settings - Fork 232
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
SLDump: print the current vprop #2642
base: master
Are you sure you want to change the base?
Conversation
On tahina-pro/FStar@4636dda, I even tried simplifying |
In all cases, so far |
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.
Thank you @tahina-pro for this PR. I left a few comments directly about the code.
At a higher level, I still do not understand how guard_vprop
works, nor how it interacts with the core invariants of the Steel frame inference tactic. Some of the core questions:
- The framing tactic used to separate between separation logic goals, and logical goals, and solved the latter once all the former had been solved. How does this work now that you can have arbitrary propositions annotated with
framing_implicit
? - How does the use of
guard_vprop
interact with the unitriangular invariant we formalized in the ICFP 21 paper, which ensured that scheduling would not get stuck? - How do different uses of
override_resolve_implicits
interact with each other?
(text: string) | ||
(sq: squash (sldump_prop text p q)) | ||
(_: unit) | ||
: SteelGhost unit opened p (fun _ -> guard_vprop q) (fun _ -> True) (fun h _ h' -> h' p == h p) // TODO: replace with frame_equalities p h h'? |
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.
Yes, this and the one below needs to be a frame_equalities
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 sorry, I don't know how to do that. I'm not even sure it is possible to make a Steel function typecheck with a frame_equalities
postcondition without F* failing with:
rewrite_with_tactic left open goals
unless that function is written via effect computation reflection, which are the only instances I can see of such functions in the Steel library.
By the way, even in the existing Steel library, change_equal_slprop
does not have a frame_equalities
postcondition, so I think, if this is an issue for sldump
, then this should also be an issue for change_equal_slprop
.
I believe this is orthogonal to making sldump
print the context, but I think if all Steel functions with a frame_equalities
postcondition can only be written using effect computation reflection, then we need to move the definition of sldump'
to Steel.Effect.Atomic
. (And yes, without framing_implicit
s, sldump'
can live there.)
In all cases, thank you in advance for your help there.
| _ -> T.fail "ill-formed squash" | ||
|
||
[@@ resolve_implicits; framing_implicit; plugin; | ||
override_resolve_implicits_handler framing_implicit [`%Steel.Effect.Common.init_resolve_tac]] |
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.
How does override_resolve_implicits_handler
behave when another one is also in scope? (e.g., your gen_elim)
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.
See #2553 (comment) : for a given tag
, basically, there can be at most one ("dominating") definition tagged with the resolve_implicits; tag
attributes and not overridden elsewhere; otherwise, F* will raise an error. More precisely, see 5c61481 : if there are several resolve_implicits
candidates not overridden, then F* will raise warning 348 and leave the implicit unsolved.
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.
Sorry, I meant when you have several definitions with overrride_resolve_implicits_handler
in scope, as for instance would be the case if you open both SLDump and GenElim. Would this lead to an error?
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.
Yes, F* would fail, and you would need to explicitly "close the diamond" yourself by defining a tactic that would override both.
(This is all due to unquote
not supported by native tactics, see my detailed explanation in #2553, section "Generic elimination of exists_ and pure", Step 3)
include Steel.Effect | ||
include Steel.Effect.Atomic | ||
|
||
val sldump' |
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 do you need sldump'
and sldump
?
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 provide a version without any framing_implicit
arguments for those who want to explicitly instantiate implicit arguments. I have some trouble calling functions with framing_implicit
arguments when explicitly instantiating some, but not all such implicits.
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 are the issues that you observe? I'm a bit surprised that there is a difference in behavior between a tagged implicit provided explicitly, and the same argument marked explicit, I can't remember observing this when debugging the Steel tactic.
(#[@@@framing_implicit] p: vprop) | ||
(#[@@@framing_implicit] q: vprop) | ||
(text: string) | ||
(#[@@@framing_implicit] sq: squash (sldump_prop text p q)) |
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 think you could put this squash before the text
argument, and remove the need for the trailing unit.
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 so: I made sldump_prop
depends on text
, so that solve_sldump_prop
(the tactic to solve sldump_prop
) can print that additional text
. If I were to remove that dependency, then I wouldn't know how solve_sldump_prop
would grab text
.
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.
Ah right, sorry, I was still thinking of sldump_prop as being in the requires, and not as an argument
if not (q_is_uvar) | ||
then T.fail "sldump_prop is already solved"; | ||
let p' = T.term_to_string p in | ||
T.print (msg ^ ": " ^ p'); |
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.
Using print_atoms
here would lead to a nicer output.
Regarding the data uvars, I think that's fine, and actually useful this way. This will provide better information about what has been solved in the current state, and which implicits might need to be explicited. Regarding |
Thank you @R1kM for your detailed review. I can answer most of your questions based on #2553:
I am not enabling arbitrary propositions in FStar/ulib/experimental/Steel.Effect.Common.fsti Line 1326 in 4d0b805
I enabled their resolution by adding a case in FStar/ulib/experimental/Steel.Effect.Common.fsti Lines 2746 to 2758 in eddc84f
This code snippet, which I added, is responsible for choosing, from the dictionary provided as argument to (Also note that, in the case where the user tactic fails but the goal has no uvars, it is passed to SMT.) See also my detailed explanation in #2553, section "Generic elimination of exists_ and pure", Step 3. |
The key parts are that:
Now, based on these changes of mine, the unitriangular invariant is preserved if all the following conditions hold:
Which is the case both for |
This PR implements a suggestion by @R1kM : provide a SteelGhost function,
sldump
, to print the current vprop at type-checking time.For instance, the code below (available in this PR at
tests/micro-benchmarks/SteelSLDump.fst
) will behave as follows when type-checking:As Aymeric noted, this PR gathers the whole vprop available at its call site by virtue of being specified in the
SteelGhostF
effect.The critical takeaway of this PR is that it implements
sldump
without any change to the Steel tactic. The secret sauce: follow the same pattern asgen_elim
(#2553).Here is the signature of
sldump
:The post-resource is guarded by
guard_vprop
: if any vprop uvar ?r appears in the same goal asguard_vprop q
(or transitively "depends" on such a vprop uvar), thenq
must be solved before?r
. Thus,sldump
is the only place whereguard_vprop q
can be solved.In practice,
q
is solved by solving the logical goalsldump_prop text p q
introduced when callingsldump
.To solve this logical goal, we register a tactic,
solve_sldump_prop
, separate from the Steel tactic, by including it as an entry into the dictionary passed when instantiating the Steel framing tactic:(This step wouldn't be necessary if
unquote
were supported by native tactics; if so, then an attribute onsolve_sldump_prop
would be enough.)Then, I implemented
solve_sldump_prop
in such a way that it makes progress only oncep
is fully resolved (i.e. has no remaining vprop uvars.)And, most importantly, that tactic is in charge of printing
p
...