-
Notifications
You must be signed in to change notification settings - Fork 279
feat: Compute triggers for such-that operations #6023
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
feat: Compute triggers for such-that operations #6023
Conversation
# Conflicts: # Source/DafnyCore/Triggers/QuantifiersCollector.cs
# Conflicts: # Source/DafnyCore/AST/Expressions/Comprehensions/ExistsExpr.cs # Source/DafnyCore/AST/Substituter.cs # Source/DafnyCore/Dafny.atg # Source/DafnyCore/Triggers/ComprehensionTriggerGenerator.cs # Source/DafnyCore/Verifier/BoogieGenerator.ExpressionWellformed.cs # Source/DafnyCore/Verifier/BoogieGenerator.cs # Source/DafnyCore/Verifier/Statements/BoogieGenerator.TrStatement.cs # Source/IntegrationTests/TestFiles/LitTests/LitTest/auditor/TestAuditor.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/CoinductiveProofs.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/Comprehensions.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/ComprehensionsNewSyntax.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/DirtyLoops.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/LetExpr.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/snapshots/Snapshots5.run.legacy.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/git-issues/git-issue-1207.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/git-issues/git-issue-1545.dfy.refresh.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/git-issues/git-issue-897.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/triggers/emptyTrigger.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/triggers/loop-detection-messages--unit-tests.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/triggers/some-terms-do-not-look-like-the-triggers-they-match.dfy.expect # Source/IntegrationTests/TestFiles/LitTests/LitTest/wishlist/assign-such-that-antecedent.dfy.expect
# Conflicts: # Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/CoinductiveProofs.dfy.expect
# Conflicts: # Source/DafnyCore/Verifier/BoogieGenerator.Types.cs
# Conflicts: # Source/DafnyCore/Verifier/Statements/BoogieGenerator.TrStatement.cs
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 currently reading through the changes. The core changes look good to me; I've left some suggestions but nothing blocking. The rest I'm currently reading through.
Source/IntegrationTests/TestFiles/LitTests/LitTest/git-issues/git-issue-3855.dfy
Outdated
Show resolved
Hide resolved
Source/IntegrationTests/TestFiles/LitTests/LitTest/dafny0/DiscoverBounds.dfy
Show resolved
Hide resolved
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.
Looks great to me; I've left comments but I don't think any of them require significant refactorings.
} else if (LiteralExpr.IsFalse(a) || LiteralExpr.IsTrue(b)) { | ||
return a; | ||
} | ||
// Note, to respect evaluation order, we don't simplify "X && false" to "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.
Is it really a matter of evaluation order? What actually goes wrong with the relaxed criterion from before?
It could be side effets, but what side effects does an expression have?
Also, isn't that what the allowSimplification flag is for?
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.
Good call-out. I thought some more and agree that it's okay that, when requested by allowSimplification
, include that simplication after all. The test suite seems to confirm this. (I applied the change to &&
, ==>
, and ||
.)
@@ -76,7 +76,9 @@ method Correct2<W>(s: seq<W>) returns (ghost r: Option<W>) | |||
} | |||
var b := exists w: W :: P(w); | |||
ghost var nonempty: W := s[0]; // this leads the verifier to see that W must be nonempty after all | |||
ghost var w: W :| b ==> P(w); | |||
ghost var w: W :| b ==> P(w) by { | |||
var pw := P(nonempty); // introduce something to trigger quantification with |
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 wonder if we actually left :|
alone (without triggers) on purpose back when we added them elsewhere, because it's not completely unreasonable to just try ambient values when we're trying to prove that a :|
clause is inhabited (and indeed we do try to enumerate a few reasonable values by hand). That said I think that more stability is a reasonable price to pay here.
Source/IntegrationTests/TestFiles/LitTests/LitTest/git-issues/git-issue-1163.dfy
Outdated
Show resolved
Hide resolved
Thanks for your detailed review, @cpitclaudel! I've made changes accordingly, so the PR is ready for your reapproval (if you like my changes). |
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.
Perfect, looks great now!
I agree that we can put the refactoring where the existential is explicit and :| is separate from := to some hypothetical future.
This PR computes triggers for
:|
operations ("assign such that", "let such that", bindingif
statements, and bindingif case
statements). It also improves the logic used to guess witnesses.Main new features
:|
constructsif
andif-case
statements with:|
, output warning if there is no trigger:|
constructs, support the same trigger attributes as for quantifiers, namely:{:trigger <exprs>}
to manually specify a trigger{:trigger}
to suppress any warning{:nowarn}
to change the warning into an information messageRelated new features
A proof obligation associated with assign-such-that and let-such-that constructs
x :| P(x)
is to show that such anx
exists. The general form of this is the proof obligationexists x :: P(x)
. However, Dafny also allows the proof obligation to be established from candidate witnesses that the verifier guesses. Thus, the proof obligation may have several disjuncts, for example:This PR improves this machinery further; specifically:
x
is a mapP(x)
has the formx in map[...]
for some map displaymap[...]
, use the given keys as guesses (just like is done for other collection displays)P(x)
that don't mentionx
to outside the existential, so that instead ofexists x :: R || P(x)
, the proof obligation isR || exists x :: P(x)
(for soundness, this is done only if the type ofx
is nonempty)Notes for reviewers
The PR also contains smaller changes, namely:
BoundVar
creation to indicate ghost statusif
statements to print the original:|
syntax in the guard, rather than its desugaringLiteralExpr.IsFalse
methodBlockByProofStmt
statementsZero
, return an empty mapGuessWitnesses
make more use of theZero
methodAnd regarding tests:
triggers/TriggersForSuchThat.dfy
(also relevant:wishlist/assign-such-that-antecedent.dfy
):|
constructs have specific triggersBy submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.