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: Error wording closer to semantics #676

Merged
merged 14 commits into from
Jan 14, 2023
2 changes: 1 addition & 1 deletion Source/Concurrency/YieldingProcDuplicator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ private void InjectGate(Action action, CallCmd callCmd, bool assume = false)
else
{
newCmdSeq.Add(CmdHelper.AssertCmd(assertCmd.tok, expr,
$"This gate of {action.proc.Name} might not hold."));
$"This gate of {action.proc.Name} could not be proven."));
}
}

Expand Down
28 changes: 14 additions & 14 deletions Source/Core/ProofObligationDescription.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,75 +32,75 @@ public abstract class ProofObligationDescription {

public class AssertionDescription : ProofObligationDescription
{
public override string SuccessDescription => "This assertion holds.";
public override string SuccessDescription => "this assertion holds";
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going after "proven" to stress out this is about proofs, then why not also "proven" instead of "holds"?
Like "Boogie proved this assertion".

Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for other such patterns.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. Note that the success message is for now only displayed in rare occasions, in some toolings and while hovering Dafny programs in VSCode. Since there is no call to action, "holds" is the same as "was proven", but it's shorter.


public override string FailureDescription => "This assertion might not hold.";
public override string FailureDescription => "this assertion could not be proven";

public override string ShortDescription => "assert";
}

public class PreconditionDescription : ProofObligationDescription
{
public override string SuccessDescription =>
"All preconditions hold for this call.";
"all preconditions hold for this call";

public override string FailureDescription =>
"A precondition for this call might not hold.";
"a precondition for this call could not be proven";

public override string ShortDescription => "precondition";
}

public class RequiresDescription : ProofObligationDescription
{
public override string SuccessDescription =>
"This precondition holds.";
"this precondition holds";

public override string FailureDescription =>
"This is the precondition that might not hold.";
"this precondition could not be proven";

public override string ShortDescription => "requires";
}

public class PostconditionDescription : ProofObligationDescription
{
public override string SuccessDescription =>
"All postconditions hold for this return path.";
"all postconditions hold for this return path";

public override string FailureDescription =>
"A postcondition might not hold on this return path.";
"a postcondition could not be proven on this return path";

public override string ShortDescription => "postcondition";
}

public class EnsuresDescription : ProofObligationDescription
{
public override string SuccessDescription =>
"This postcondition holds.";
"this postcondition holds";

public override string FailureDescription =>
"This is the postcondition that might not hold.";
"this postcondition could not be proven";

public override string ShortDescription => "ensures";
}

public class InvariantEstablishedDescription : AssertionDescription
{
public override string SuccessDescription =>
"This loop invariant holds on entry.";
"this loop invariant holds on entry";

public override string FailureDescription =>
"This loop invariant might not hold on entry.";
"this loop invariant could not be proven on entry";

public override string ShortDescription => "invariant established";
}

public class InvariantMaintainedDescription : AssertionDescription
{
public override string SuccessDescription =>
"This loop invariant is maintained by the loop.";
"this loop invariant is maintained by the loop";

public override string FailureDescription =>
"This loop invariant might not be maintained by the loop.";
"this loop invariant might not be maintained by the loop";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This message still contains the "might not" phrasing, as opposed to "could not be proved". Unfortunately, using that wording makes it pretty awkward: "this loop invariant could not be proved to be maintained by the loop".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about: "this invariant could not be proved within the loop"?
Or, a shorter version of yours, "this invariant could not be proved to be maintained by the loop"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current PR, I used the second one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I like that one, too.


public override string ShortDescription => "invariant maintained";
}
10 changes: 5 additions & 5 deletions Source/UnitTests/ExecutionEngineTests/ExecutionEngineTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ procedure Bad2(y: int)
options.VcsCores = 4;
var engine = ExecutionEngine.CreateWithoutSharedCache(options);

var expected = @"fakeFilename1(3,3): Error: This assertion might not hold.
var expected = @"fakeFilename1(3,3): Error: this assertion could not be proven
Execution trace:
fakeFilename1(3,3): anon0
fakeFilename1(8,3): Error: This assertion might not hold.
fakeFilename1(8,3): Error: this assertion could not be proven
Copy link
Contributor

Choose a reason for hiding this comment

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

I am also wondering if active voice would be nicer in all of these. So something like: "Boogie could not prove this assertion". Personally, I would prefer such active voice to current passive voice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Me too, but after looking at the error messages, the first 4 words of "Boogie could not prove " are useless to know what the error is about, whereas by keeping this assertion as the subject, it's much clearer what's in the focus there.

Execution trace:
fakeFilename1(8,3): anon0

Expand Down Expand Up @@ -119,12 +119,12 @@ procedure Good(y: int)
await task1Writer.DisposeAsync();
await task2Writer.DisposeAsync();
var output = writer.ToString();
var expected = @"fakeFilename1(3,3): Error: This assertion might not hold.
var expected = @"fakeFilename1(3,3): Error: this assertion could not be proven
Execution trace:
fakeFilename1(3,3): anon0

Boogie program verifier finished with 1 verified, 1 error
fakeFilename2(3,3): Error: This assertion might not hold.
fakeFilename2(3,3): Error: this assertion could not be proven
Execution trace:
fakeFilename2(3,3): anon0

Expand Down Expand Up @@ -169,7 +169,7 @@ public async Task LoopInvariantDescriptions()
await engine.ProcessProgram(writer, program1, "fakeFilename");
await writer.DisposeAsync();
var output = writer.ToString();
var expected = @"fakeFilename(10,5): Error: This loop invariant might not be maintained by the loop.
var expected = @"fakeFilename(10,5): Error: this loop invariant might not be maintained by the loop
fakeFilename(10,5): Related message: fake failure
Execution trace:
fakeFilename(5,3): entry
Expand Down