Why is Dafny thinking that this incorrect algorithm is correct?

Question

The following array reversing code is "proven correct" with Dafny but it clearly isn't correct. What am I doing wrong?

A counter example is the array:

var a = new int[4] {1,3,5,7};

with the expected result {7,5,3,1} and the actual result {1,3,3,1}:

// A method reversing an array

predicate is_sint32(x : int) { -0x8000_0000 <= x < 0x8000_0000 }

newtype {:nativeType "int"} sint32 = x | -0x8000_0000 <= x < 0x8000_0000

method reverse(a: array<sint32>)
    requires is_sint32(a.Length)
    modifies a;
    ensures forall i :: 0 <= i < a.Length ==> a[i] == old(a)[a.Length - 1 - i];
{
    var i       := 0 as sint32;
    var aLength := a.Length as sint32;

    while i < aLength
        invariant 0 <= i && i <= aLength
        invariant forall k :: 0 <= k < i ==> a[k] == old(a)[aLength - 1 - k];
    {
        a[aLength - 1 - i] := a[i];

        i := i + 1;
    }
}

I found a solution inspired by the great answer from James Wilcox. Adding here for future reference:

method reverse(a: array<int>)
    modifies a;
    ensures forall i :: 0 < i < a.Length ==> a[i] == old(a[a.Length - 1 - i]);
{
    var i := 0;

    while i < a.Length / 2
        invariant 0 <= i <= a.Length / 2
        invariant forall k :: 0 < k < i ==> a[k] == old(a[a.Length - 1 - k])
        invariant forall k :: i <= k < a.Length - i ==> a[k] == old(a[k])
        invariant forall k :: 0 <= k < i ==> a[a.Length - 1 - k] == old(a[k])
    {
        var v0 := a[i];
        var v1 := a[a.Length - 1 - i];

        a[i]                := v1;
        a[a.Length - 1 - i] := v0;

        i := i + 1;
    }
}

And here is the solution using C# native types for optimum performance when generating C# code:

predicate is_sint32(x : int) { -0x8000_0000 <= x < 0x8000_0000 }

newtype {:nativeType "int"} sint32 = x : int | -0x8000_0000 <= x < 0x8000_0000

method reverse(a: array<sint32>)
    requires is_sint32(a.Length)
    modifies a;
    ensures forall i :: 0 < i < a.Length ==> a[i] == old(a[a.Length - 1 - i]);
{
    var aLength     := a.Length as sint32;
    var aLengthDiv2 := aLength / 2;
    var i           := 0 as sint32;

    while i < aLengthDiv2
        invariant 0 <= i <= aLengthDiv2
        invariant forall k :: 0 < k < i ==> a[k] == old(a[aLength - 1 - k])
        invariant forall k :: i <= k < aLength - i ==> a[k] == old(a[k])
        invariant forall k :: 0 <= k < i ==> a[aLength - 1 - k] == old(a[k])
    {
        var v0 := a[i];
        var v1 := a[aLength - 1 - i];

        a[i]               := v1;
        a[aLength - 1 - i] := v0;

        i := i + 1;
    }
}

score 3 · Accepted Answer · answered Aug 27 '22 at 10:16

The problem is old(a)[...] does not mean what you think. See the reference manual section on old expressions.

The short version is that old only affects "heap dereferences", which include field accesses (as in old(x.f)) and array accesses (as in old(a[i])).

Since your uses of old do not contain any field or array accesses inside the parentheses, they are not doing anything. Your postcondition is equivalent to:

ensures forall i :: 0 <= i < a.Length ==> a[i] == a[a.Length - 1 - i]

(simply deleting the old), which says that after this method runs, a is guaranteed to be a palindrome. Your code does satisfy this specification, but it's not the specification you intended to write.

And thanks again because it helped me find the solution => I learned a lot. — mbrodersen, Aug 27 '22 at 13:15

Why is Dafny thinking that this incorrect algorithm is correct?

1 Answers1