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

Bogus safety violation checking if a set is a subset of Nat. #2960

Merged
merged 7 commits into from
Sep 26, 2024

Conversation

lemmy
Copy link
Contributor

@lemmy lemmy commented Aug 22, 2024

Fixes Github issue #2948
#2948

@lemmy
Copy link
Contributor Author

lemmy commented Aug 22, 2024

5724d0d causes Apalache to report a proper error message: SetInRule is not implemented for type PowSet[Set(Int)] (found in {1, 2, 3, 4} ∈ SUBSET Nat).

@lemmy lemmy force-pushed the mku-gh2948 branch 3 times, most recently from 2fc1591 to b2ca837 Compare September 2, 2024 14:25
@lemmy
Copy link
Contributor Author

lemmy commented Sep 2, 2024

@thpani @konnov c69c355 and 830caa7 cause Apalache to no longer hit this code path for the given inputs (the expressions are rewritten elsewhere). Would you like to add the safeguard below anyway? If you do, I'll update the tests to expect the corresponding error message.

https://github.com/apalache-mc/apalache/pull/2960/files#diff-455ee0089c33ede12710ae233adedc17473d8ac7cc6a2ca7246f127a7482512aR58-R60

@konnov
Copy link
Collaborator

konnov commented Sep 3, 2024

@thpani @konnov c69c355 and 830caa7 cause Apalache to no longer hit this code path for the given inputs (the expressions are rewritten elsewhere). Would you like to add the safeguard below anyway? If you do, I'll update the tests to expect the corresponding error message.

https://github.com/apalache-mc/apalache/pull/2960/files#diff-455ee0089c33ede12710ae233adedc17473d8ac7cc6a2ca7246f127a7482512aR58-R60

Hey Markus! The Apalache encoding was not designed to deal with infinite sets. The only exception (a hack?) was made for Int and Nat, for the case of introducing an integer constant like \E x \in Int: P in an action. Since both OOPSLA19 and TACAS23 encodings are using arenas, it is expected that a set cell carries an overapproximation of the set.

I have two thoughts about this:

  • If you are trying to deal with infinite sets in the bmcmt rewriting rules, it is, most likely, too late in the rewriting process. These rules have non-trivial invariants about arenas (which are not really documented). For what you are doing, OptimizationPass seems to be the right place.
  • We could probably extend an encoding for infinite sets, though in practice what one probably needs the most is generating bounded-size sets that may contain unbounded integers. Developing a consistent encoding is a focused effort. I feel like hacking the current encoding here and there may lead to very unexpected results in model checking.

@lemmy
Copy link
Contributor Author

lemmy commented Sep 3, 2024

@konnov This PR does not aim to extend the encoding. Instead, it ensures that Apalache throws an error when an infinite set is encountered in a context where it isn't properly handled.

@konnov
Copy link
Collaborator

konnov commented Sep 4, 2024

Ah, sorry, I missed that. Yes, this makes sense to add. Thanks a lot!

Copy link
Collaborator

@konnov konnov left a comment

Choose a reason for hiding this comment

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

A safeguard would definitely help!

@lemmy lemmy marked this pull request as ready for review September 6, 2024 01:40
@lemmy
Copy link
Contributor Author

lemmy commented Sep 6, 2024

This is ready for review and merging. Although commits c69c355 and 830caa7 prevent any of the three encodings from reporting a bogus invariant violation for expressions like {1, 2, 3} \in SUBSET Nat (due to rewriting), this update ensures that Apalache generates a clear error message when handling infinite sets, should the rewrites ever be removed in the future.

Copy link
Collaborator

@konnov konnov left a comment

Choose a reason for hiding this comment

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

This makes sense! Thanks for adding the safeguards

@konnov
Copy link
Collaborator

konnov commented Sep 18, 2024

I cannot add an entry in .unchanged, since it's your PR. Could you add one?

@lemmy
Copy link
Contributor Author

lemmy commented Sep 18, 2024

What should that entry be?

@konnov
Copy link
Collaborator

konnov commented Sep 18, 2024

What should that entry be?

Oh, just one line describing the nature of the change, see https://github.com/apalache-mc/apalache/blob/main/CONTRIBUTING.md#how-to-record-a-change

I guess, it should technically go into bug-fixes.

@lemmy
Copy link
Contributor Author

lemmy commented Sep 18, 2024

done

Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

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

LGTM

@thpani thpani enabled auto-merge (squash) September 26, 2024 13:37
@thpani thpani merged commit 169d142 into apalache-mc:main Sep 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants