Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: Improve tests #5859

Merged
merged 12 commits into from
Nov 17, 2024
Merged

chore: Improve tests #5859

merged 12 commits into from
Nov 17, 2024

Conversation

RustanLeino
Copy link
Collaborator

Overview

This PR improves several test cases in the Dafny test suite to make them relies less on old/deprecated features and to make them more stable. My intent here is to still preserve the goal of each test.

These changes came about from trying to run Dafny with a new version of Z3, but the PR itself does not change the version of Z3 being used.

Details

  • dafny0/SeqFromArray.dfy
    Changed quantified properties into checks for a single arbitrary value.
  • dafny0/TypeConversions.dfy
    Changed one method's use of bv67 to bv7. This is because Z3 has a tough time converting between integers and. large bitvectors. As one data point of the current state of such conversions, I also added to the test file the times required for other bitvector tests.
  • dafny2/SmallestMissingNumber-functional.dfy
    Use opaque, turning off induction, add some inline lemmas (assert), and remove the no longer needed isolate_assertions.
  • dafny4/Lucas-down.legacy.dfy
    Remove the unnecessary use of the legacy flag /arith. Rename the test file to Lucas-down.dfy.
  • dafny4/Lucas-up.legacy.dfy
    Remove the need for /arith, use opaque, turn off automatic induction, and write proofs by hand.
  • dafny4/Primes.dfy
    Use opaque.
  • git-issues/git-issue-2301.dfy
    Disentangle tests by using several methods/functions, and avoid hitting the max-5 error limit (which previously had masked one error).
  • git-issues/git-issue-3855.dfy
    Replace \d with [0-9] to support the test on platforms where sed doesn't understand \d.
  • lambdas/MatrixAssoc.dfy
    Explicitly indicate which variable to do the induction on.
  • triggers/emptyTrigger.dfy
    Change == to <= to avoid making the test do different things if (a new version of) Z3 optimizes quantifiers like exists z :: ... && z == E into ... with z replaced by E.
  • wishlist/sequences-s0-in-s.dfy
    Disentangle tests into separate methods.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@RustanLeino RustanLeino marked this pull request as ready for review October 30, 2024 21:18
@RustanLeino RustanLeino enabled auto-merge (squash) October 30, 2024 21:18

method M4() returns (x: int, n: nat, r: real, even: EvenInt, small: SmallReal, b67: bv67, w: bv32, seven: bv7, noll: bv0)
ensures UseTheVars(x, n, r, even, small, b67, w, seven, noll)
// M4 is checked quickly, but using bv67 instead of bv7 takes forever, and even bv23 takes 40 minutes. See numbers at bottom of this file.
Copy link
Contributor

Choose a reason for hiding this comment

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

could remove one of seven or bv7 from the inputs.

@@ -90,7 +90,7 @@ function SMN''(xs: List<nat>, n: nat, len: nat): nat
SMN''(R, n + llen, len - llen)
}

function Split(xs: List<nat>, b: nat): (List<nat>, List<nat>)
opaque function Split(xs: List<nat>, b: nat): (List<nat>, List<nat>)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this opaque.

However, I don't understand why we need Split_Correct as a separate lemma. I tried adding the requires and ensures clauses of Split_Correct directly into the function and it verifies. So, something to consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

And, its a subjective thing, but when the stated property is the definition of a function and can be proved directly, I think its cleaner to have it in the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I often make the decision based on if the property is one I think all users of the function are going want. If it is, then I add it as a postcondition of the function. Otherwise, I make it into a separate lemma.

In this case, the property stated by Split_Correct is relevant only in situations where the input has no duplicates. Since the Split function doesn't require the no-duplicates property, I think the lemma is best stated separately.

Copy link
Member

Choose a reason for hiding this comment

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

Why are you using opaque here, instead of hide on the consumer side?

I think opaque should only be placed on functions whose behavior is fully defined by their ensures clauses. Here's a PR that applies that principle to all of the standard library: #5781

@@ -194,10 +194,20 @@ lemma Split_Correct(xs: List<nat>, b: nat)
Elements(r.1) == (set x | x in Elements(xs) && b <= x) &&
NoDuplicates(r.0) && NoDuplicates(r.1)
{
var split := Split(xs, b);
Copy link
Contributor

Choose a reason for hiding this comment

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

We wouldn't need this lemma if we incoroprate the comment above.

@@ -206,17 +216,38 @@ lemma Elements_Property(xs: List)
{
}

ghost function IntRange(lo: nat, len: nat): set<nat>
ensures |IntRange(lo, len)| == len
opaque ghost function IntRange(lo: nat, len: nat): (s: set<nat>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider the following function definition and removing FormSet entirely.

opaque ghost function IntRange(lo: nat, len: nat): (s: set<nat>)
  decreases len
  ensures |s| == len
  ensures forall x :: x in s <==> lo <= x < lo + len
{
  if (len == 0) then {}
  else IntRange (lo + 1, len - 1) + {lo}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestions.

@@ -236,22 +267,30 @@ lemma {:isolate_assertions} SMN'_Correct(xs: List<nat>, n: nat, len: nat)
var bound := IntRange(n, half);
Cardinality(Elements(L), bound);
if llen < half {
reveal SMN'();
Copy link
Contributor

Choose a reason for hiding this comment

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

SMN' is not opaque

SMN'_Correct(L, n, llen);
} else {
var s := SMN'(R, n + llen, len - llen);
var s := SMN'(xs, n, len);
var t := SMN'(R, n + llen, len - llen);
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps don't need to define t here. Two options: (i) remove the definition and replace t by s in the assert by below, (ii) move the definition of t into the assert by.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it into the assert by.

@@ -48,14 +49,20 @@ lemma BitSize(i: nat, n: nat)

// An easy-to-read name for the expression that checks if a number
// is even.
ghost predicate EVEN(n: nat)
opaque ghost predicate EVEN(n: nat)
Copy link
Member

Choose a reason for hiding this comment

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

Again, try to avoid introducing opaque function in our tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

@keyboardDrummer keyboardDrummer Nov 15, 2024

Choose a reason for hiding this comment

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

Because opaque functions are more cumbersome to use then regular ones. They need to be revealed any time you want the body to be visible, and are hidden in contexts even where no reduction in resource count is needed.

You can see the code size advantage of avoiding opaque functions in the refactoring in this PR: #5781

typerSniper
typerSniper previously approved these changes Nov 15, 2024
@RustanLeino RustanLeino merged commit b998ed9 into dafny-lang:master Nov 17, 2024
22 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants