Add opaque keyword to Laurel grammar#969
Conversation
- Add OpaqueClause category and optional opaque parameter to procedure/function grammar ops, placed between invokeOn and ensures clauses - Update ConcreteToAbstractTreeTranslator to parse the new opaque argument and use it to determine Body.Opaque vs Body.Transparent - Update LaurelFormat to output 'opaque' for Opaque bodies - Update all Laurel test files: procedures with ensures clauses now use explicit opaque keyword; procedures that only had 'ensures true' now use just 'opaque' - Remove comments about considering more explicit opaque syntax - Update PythonRuntimeLaurelPart Laurel source strings with opaque keyword Closes #9
|
@keyboardDrummer-bot please resolve the conflicts |
Resolve conflicts: - LaurelGrammar.lean: keep opaque keyword comment - LaurelFormat.lean: accept upstream deletion (replaced by AbstractToConcreteTreeTranslator) - AbstractToConcreteTreeTranslator.lean: add opaque argument to procedureToOp - LaurelGrammar.st: fix opaqueClause format string to use newline+indent prefix - LiftHolesTest.lean: add opaque to hole function expected outputs - ConstrainedTypeElimTest.lean: add opaque to test procedure expected output - AbstractToConcreteTreeTranslatorTest.lean: add opaque to roundtrip test inputs/outputs - T8d_HeapMutatingValueReturn.lean: add opaque to procedures with ensures clauses
|
I think #668 supersedes this. Both add
|
Could you explain why that is stronger?
Does it matter whether Core procedures are modular or not? This is about Laurel procedures.
It's disallowed by design to reduce VC size. Why would you allow this?
Are you sure? Should be a syntax error in this PR |
Aesthetic preference —
Even when the body is visible, the postcondition axiom can help the solver. If the body is complex (recursive, many branches), deriving the postcondition from the body alone may be hard. The axiom acts as a proof hint — logically redundant but practically useful.
I tested on a clean build of this branch. |
Procedures will support both being opaque or transparent. In the upcoming #927 we will support them being transparent if the body is an expression. I'll also add an error for statement bodies marked as transparent.
I thought of having it there first, but it relates to ensures clauses and the body, so seems better to locate it there. It doesn't seem important enough to be the first thing that is mentioned when reading the procedure definition. Do you have a strong opinion about this?
That's right. I think that separate lemmas are better for this use-case since then you don't always get the postcondition. We can offer ways to automatically invoke lemmas, like Verus' broadcast mechanism, so you don't have the boilerplate of having to manually call them. We can also still decide to allow postconditions and a transparent body, but it's much easier to start than to stop allowing it, so I'd rather stick with not allowing it unless we're absolutely sure we do want to allow it.
You're right, updated the PR, thanks. |
|
@keyboardDrummer-bot can you resolve the build issues? |
…opaqueSpec - Changed procedureToOp to produce opaqueSpec op with ensures and modifies as nested args (matching the grammar), instead of separate top-level args - Added missing opaque keyword in MapStmtExprTest and T2_ModifiesClauses tests
I wouldn't call it taking something away because it's never been there.
What part of the explanation I gave does not make sense to you? Here it is again: I think the purpose of a postcondition is to enable encapsulating the body, so it doesn't make sense to have a postcondition and a transparent body. If you want to use the postcondition not for encapsulation but to prove additional properties, then I think it's better to use a different construct, like a lemma.
If you would use opaque as a prefix, it would also not take any arguments. What's the difference? What do you think of the arguments I gave in favor of putting it where this PR puts it?
Why would someone think that it needs to be repeated? |
|
FWIW, here are my opinions on this, in the hopes of tie-breaking:
If this is about the awkward readability when you put multiple clauses on one line, again I think it's less of a concern since the vast majority of the time we're only going to be printing out source code from translated ASTs, so we'll always have the newlines. |
Okay, well (2) is not in scope of this PR. Can you provide your preference for the opaque keyword location? My suggestion would be that if it is not tied to ensures clauses, put it after the ensures clauses, right before the body, and otherwise put it right before the ensures clauses. |
|
I don't have a strong preference for the ordering really. Can we support parsing it in either location? In Dafny I always liked ordering things as: Mainly I liked thinking of With that in mind, I suppose I weakly prefer having it before the |
As a prefix it's a modifier like
I understand them. I think the arguments against outweigh them.
By a few lines of parser code. On ordering: ideally the clause order wouldn't be enforced. If it is, I'd put I still prefer prefix, but this seems like the best postfix option to me. |
I see. I think you're saying that in Java (and other languages?) the modifier keywords are all at the front. That's fair. It makes sense to base syntax on existing languages. In Laurel that precedent had already been violated though,
What do you mean with applying to a whole declaration? I wouldn't say it does that since the signature is unaffected. If we put it before ensures/modifies/body, then it's right before the things that it affects.
If it wouldn't affect the ensures and modifies clause, then I agree, but right now it does affect those. A transparent procedure can not have ensures or modifies clause (unrelated to this PR). @robin-aws @fabiomadge I'm not feeling any strong objections to this PR from either of you. Can you approve it? Future looking discussion on disconnected opaque and ensures+modifiesA question I have for both @robin-aws and @fabiomadge is whether you think a modifies clause should be possible on a transparent procedure. For a transparent procedure you will never have to add it, since the heap is not havoc'ed when calling such a procedure: the state of the heap can be inferred from the body. Adding a modifies clause just provides a separate way of learning something about the heap, as a sort of heuristic. |
Yes. It serves as checked documentation of heap access and catches accidental writes to things the procedure shouldn't touch. |
Okay, I'm open to discussing the opaque vs ensures+modifies. |
c5ca718
Summary
Adds an explicit
opaquekeyword to the Laurel grammar. Previously, a procedure was implicitly opaque when it hadensuresclauses, which sometimes meant one had to addensures truejust to make something opaque. Now theopaquekeyword must be explicitly specified.Syntax
The
opaquekeyword is placed betweenrequires/invokeOnandensuresclauses:Changes
Grammar (
LaurelGrammar.st)OpaqueClausecategory withopaqueClauseopopaqueparameter to bothprocedureandfunctionops, positioned betweeninvokeOnandensuresParser (
ConcreteToAbstractTreeTranslator.lean)parseProcedureto accept 10 arguments (was 9) including the newopaqueArgopaqueflag instead of inferring from postconditionsFormatter (
LaurelFormat.lean)formatBodynow outputsopaqueforBody.Opaquevariantsensuresbeforemodifies(matching grammar order)Test updates
ensuresclauses now includeopaqueensures truenow use justopaque(theensures trueis removed)PythonRuntimeLaurelPart.leanLaurel source stringsLiftHolesTest,ConstrainedTypeElimTest,PreludeVerifyTest, andT8c_BodilessInliningTesting
All tests pass except the pre-existing
StrataTest.DDM.Integration.Java.TestGenfailure (missing jar file, unrelated to this change).