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

Wrong conjunct mentioned in "related location" under a quantifier #2211

Closed
wilcoxjay opened this issue Jun 6, 2022 · 8 comments · Fixed by #4187
Closed

Wrong conjunct mentioned in "related location" under a quantifier #2211

wilcoxjay opened this issue Jun 6, 2022 · 8 comments · Fixed by #4187
Labels
area: error-reporting Clarity of the error reporting has-workaround: no There are no known workarounds part: verifier Translation from Dafny to Boogie (translator)

Comments

@wilcoxjay
Copy link
Collaborator

wilcoxjay commented Jun 6, 2022

Consider this Dafny program.

predicate P1(a: int)
predicate P2(a: int)

predicate Q(s: seq<int>) {
  forall i | 0 <= i < |s| ::
    (P1(s[i]) || false) && (P2(s[i]) || false)  // Line 6
}

method M(s: seq<int>)
  requires forall i | 0 <= i < |s| :: P1(s[i])
  // requires forall i | 0 <= i < |s| :: P2(s[i])
  ensures Q(s)
{}

The method M has an error because it is missing the commented-out precondition about P2. But Dafny reports the following error misleading messages:

issue2211.dfy(13,0): Error: A postcondition might not hold on this return path.
issue2211.dfy(12,10): Related location: This is the postcondition that might not hold.
issue2211.dfy(5,2): Related location
issue2211.dfy(6,14): Related location

Dafny program verifier finished with 2 verified, 1 error

The last "Related location" points to the first conjunct (the 14th character on the 6th line), but in fact the first conjunct is true and the problem is with the second conjunct.

On the other hand, Dafny behaves correctly when not under a quantifier, as in this example:

predicate P1(a: int)
predicate P2(a: int)

predicate Q(s: seq<int>) {
  |s| > 0 && P1(s[0]) && P2(s[0])
}

method M(s: seq<int>)
  requires |s| > 0
  requires P1(s[0])
  ensures Q(s)
{}

which generates

issue2211.dfy(12,0): Error: A postcondition might not hold on this return path.
issue2211.dfy(11,10): Related location: This is the postcondition that might not hold.
issue2211.dfy(5,25): Related location

Dafny program verifier finished with 2 verified, 1 error

The 25th character of the 5th line correctly points to the third conjunct, because the first two are true.

This is a relatively serious usability issue, and I feel like it is a regression that used to work?

I reproduced this error on the main branch.

@MikaelMayer
Copy link
Member

Thanks for reporting this issue !
Did it behave differently with a previous version of Dafny?

@wilcoxjay
Copy link
Collaborator Author

Not sure. Like I said, I have a feeling that this used to work, because it is somewhat fundamental to my workflow when I use Dafny, and I would have noticed it before if it was broken. But these days I only use Dafny to answer stack overflow questions, so it's hard for me to know exactly when it broke.

I suppose I will try to find some time to do a git bisect.

@MikaelMayer
Copy link
Member

I confirm that the behavior was the same in Dafny 3.1.0

@MikaelMayer
Copy link
Member

And you don't need || false, the error is the same.

@MikaelMayer MikaelMayer added part: verifier Translation from Dafny to Boogie (translator) area: error-reporting Clarity of the error reporting has-workaround: no There are no known workarounds labels Jun 6, 2022
@MikaelMayer
Copy link
Member

If you split the forall, I confirm that the error reporting works as expected.

(forall i | 0 <= i < |s| ::
    P1(s[i])) 
  && (forall i | 0 <= i < |s| ::  // Related location 3
    P2(s[i]))  // Related location 4

What you demonstrate is clearly an error in error reporting.

@wilcoxjay
Copy link
Collaborator Author

And you don't need || false, the error is the same.

Right. I should have mentioned that I put the || false in there solely to disambiguate between the error location being at the beginning of the line (which you might think means "the whole line") versus inside the first conjunct.

@wilcoxjay
Copy link
Collaborator Author

Ok, I used Clément's new script to try all previous releases, and it turns out I was wrong. This behavior goes back to at least v1.9.9.

@wilcoxjay
Copy link
Collaborator Author

Slightly simpler example

predicate P1(a: int)
predicate P2(a: int)

method M()
  requires forall i :: P1(i)
  //requires forall i :: P2(i)
  ensures forall i ::
      (P1(i) || false)  // related location
    && (P2(i) || false)
{}

MikaelMayer added a commit that referenced this issue Jun 15, 2023
MikaelMayer added a commit that referenced this issue Jul 6, 2023
…rouped quantifiers (#4187)

This PR fixes #2211
After investigation it looks like the following expression
```dafny
forall i | 0 <= i < |s| :: // Normal token
    && P1(s[i])
    && P2(s[i])
```
was split in two while searching for triggers 
```dafny
forall i | 0 <= i < |s| :: // Nested token, outer pointing to "forall", inner pointing to "P1"
    P1(s[i])
forall i | 0 <= i < |s| :: // Nested token, outer pointing to "forall", inner pointing to "P2"
    P2(s[i])
```
But another pass for searching for triggers regroups quantifiers which
have the same bound variables and same triggers. Since the two
expressions have the same trigger s[i], they are regroupped with an AND
back to:
```dafny
forall i | 0 <= i < |s| :: // Nested token, outer pointing to "forall", inner pointing to "P1"
    P1(s[i]) && P2(s[i])
```
As you can see, the problem of the regrouping is that it takes the token
of the first element of the group, which had an indication that it
pointed to P1.
The last related location of #2211 thus was an artefact of the token
being copied without paying attention to that detail. Since we regroup
the tokens, there is no nested token there to have so this PR removes
nested tokens when regrouping comprehensions to avoid giving false
information.

<small>By submitting this pull request, I confirm that my contribution
is made under the terms of the [MIT
license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: error-reporting Clarity of the error reporting has-workaround: no There are no known workarounds part: verifier Translation from Dafny to Boogie (translator)
Projects
None yet
2 participants