Skip to content
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

fix: Don't generate the "assume T.F(ins) == C.F(ins)" statement in the function override ensures check #2504

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- feat: *Less code navigation when verifying code*: In the IDE, failing postconditions and preconditions error messages now immediately display the sub-conditions that Dafny could not prove. Both on hover and in the Problems window. (https://github.com/dafny-lang/dafny/pull/2434)
- feat: Whitespaces and comments are kept in relevant parts of the AST (https://github.com/dafny-lang/dafny/pull/1801)
- fix: NuGet packages no longer depend on specific patch releases of the .NET frameworks.
- fix: Resolved unsoundness issue related to overriding functions declared in traits with `{:termination false}`. (https://github.com/dafny-lang/dafny/pull/2504)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong section of the release notes.


# 3.7.2

Expand Down
66 changes: 21 additions & 45 deletions Source/Dafny/Verifier/Translator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3475,53 +3475,29 @@ private void AddFunctionOverrideEnsChk(Function f, BoogieStmtListBuilder builder
builder.Add(TrAssumeCmd(f.tok, etran.TrExpr(en.E)));
}

//generating assume J.F(ins) == C.F(ins)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff here is deceptive: removing the builder.Add(TrAssumeCmd(...)) line makes half of the lines here unused, but the other half is still needed if there is a result variable declared. Consider this as a two step edit: first removing those interspersed unneeded lines, and then moving the remainder inside the if (resultVariable != null) block.

Bpl.FunctionCall funcIdC = new Bpl.FunctionCall(new Bpl.IdentifierExpr(f.tok, f.FullSanitizedName, TrType(f.ResultType)));
Bpl.FunctionCall funcIdT = new Bpl.FunctionCall(new Bpl.IdentifierExpr(f.OverriddenFunction.tok, f.OverriddenFunction.FullSanitizedName, TrType(f.OverriddenFunction.ResultType)));
List<Bpl.Expr> argsC = new List<Bpl.Expr>();
List<Bpl.Expr> argsT = new List<Bpl.Expr>();
// add type arguments
argsT.AddRange(GetTypeArguments(f.OverriddenFunction, f).ConvertAll(TypeToTy));
argsC.AddRange(GetTypeArguments(f, null).ConvertAll(TypeToTy));
// add fuel arguments
if (f.IsFuelAware()) {
argsC.Add(etran.layerInterCluster.GetFunctionFuel(f));
}
if (f.OverriddenFunction.IsFuelAware()) {
argsT.Add(etran.layerInterCluster.GetFunctionFuel(f));
}
// add heap arguments
if (f is TwoStateFunction) {
argsC.Add(etran.Old.HeapExpr);
argsT.Add(etran.Old.HeapExpr);
}
if (AlwaysUseHeap || f.ReadsHeap) {
argsC.Add(etran.HeapExpr);
}
if (AlwaysUseHeap || f.OverriddenFunction.ReadsHeap) {
argsT.Add(etran.HeapExpr);
}
// add "ordinary" parameters (including "this", if any)
var prefixCount = implInParams.Count - f.Formals.Count;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to imply that an implementing function can have more parameters than the trait-declared version. Given that this broke zero tests, it either a) doesn't exist as a feature (anymore?), or b) has no testing coverage. My gut tells me this was implemented optimistically but never used. If no one knows the history here I can dig into it.

for (var i = 0; i < implInParams.Count; i++) {
Bpl.Expr cParam = new Bpl.IdentifierExpr(f.tok, implInParams[i]);
Bpl.Expr tParam = new Bpl.IdentifierExpr(f.OverriddenFunction.tok, implInParams[i]);
if (prefixCount <= i && ModeledAsBoxType(f.OverriddenFunction.Formals[i - prefixCount].Type)) {
tParam = BoxIfNecessary(f.tok, tParam, f.Formals[i - prefixCount].Type);
}
argsC.Add(cParam);
argsT.Add(tParam);
}
Bpl.Expr funcExpC = new Bpl.NAryExpr(f.tok, funcIdC, argsC);
Bpl.Expr funcExpT = new Bpl.NAryExpr(f.OverriddenFunction.tok, funcIdT, argsT);
var funcExpCPossiblyBoxed = funcExpC;
if (ModeledAsBoxType(f.OverriddenFunction.ResultType)) {
funcExpCPossiblyBoxed = BoxIfUnboxed(funcExpCPossiblyBoxed, f.ResultType);
}
builder.Add(TrAssumeCmd(f.tok, Bpl.Expr.Eq(funcExpCPossiblyBoxed, funcExpT)));

//generating assume C.F(ins) == out, if a result variable was given
if (resultVariable != null) {
Bpl.FunctionCall funcIdC = new Bpl.FunctionCall(new Bpl.IdentifierExpr(f.tok, f.FullSanitizedName, TrType(f.ResultType)));
List<Bpl.Expr> argsC = new List<Bpl.Expr>();
// add type arguments
argsC.AddRange(GetTypeArguments(f, null).ConvertAll(TypeToTy));
// add fuel arguments
if (f.IsFuelAware()) {
argsC.Add(etran.layerInterCluster.GetFunctionFuel(f));
}
// add heap arguments
if (f is TwoStateFunction) {
argsC.Add(etran.Old.HeapExpr);
}
if (AlwaysUseHeap || f.ReadsHeap) {
argsC.Add(etran.HeapExpr);
}
// add "ordinary" parameters (including "this", if any)
for (var i = 0; i < implInParams.Count; i++) {
argsC.Add(new Bpl.IdentifierExpr(f.tok, implInParams[i]));
}
Bpl.Expr funcExpC = new Bpl.NAryExpr(f.tok, funcIdC, argsC);

var resultVar = new Bpl.IdentifierExpr(resultVariable.tok, resultVariable);
builder.Add(TrAssumeCmd(f.tok, Bpl.Expr.Eq(funcExpC, resultVar)));
}
Expand Down
31 changes: 31 additions & 0 deletions Test/git-issues/github-issue-2500.dfy
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// RUN: %dafny_0 /compile:0 "%s" > "%t"
// RUN: %diff "%s.expect" "%t"

module A {
trait {:termination false} Trait {
function method TotallyNotZero() : (ret: int)
ensures ret != 0
}
}

module B {

import opened A

class Class extends Trait {
constructor() {}
function method TotallyNotZero() : (ret: int)
// Missing: ensures ret != 0
{
0
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give a version of this example that doesn't use {:termination false}?

}

method Main() {
var asClass: Class := new Class();
var asTrait: Trait := asClass;
assert asClass.TotallyNotZero() == asTrait.TotallyNotZero();
assert false;
print 1 / asClass.TotallyNotZero(), "\n";
}
}
3 changes: 3 additions & 0 deletions Test/git-issues/github-issue-2500.dfy.expect
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
github-issue-2500.dfy(17,20): Error: the function must provide an equal or more detailed postcondition than in its parent trait

Dafny program verifier finished with 2 verified, 1 error