KeYProject / key

KeY Theorem Prover for Deductive Java Verification
https://key-project.org
Other
41 stars 24 forks source link

Combine equalsMod... methods #3386

Closed tobias-rnh closed 5 months ago

tobias-rnh commented 8 months ago

Intended Change

There were four different functions concerned with equality of terms other than the usual equals, namely equalsModRenaming, equalsModTermLabels, equalsModIrrelevantTermLabels and equalsModProofIrrelevancy. Now, there is a new Interface EqualsModProperty, which is extended by Term, with a single method equalsModProperty that takes two parameters: the object to be compared and one of the new type TermProperty, that implements the desired behavior of what to be ignored during the equality check. The changes can be found in a new package called equality in the logic package of key.core. The TermPropertys are designed as a singleton and can be accessed through their respective static constants, e.g. IRRELEVANT_TERM_LABELS_PROPERTY in IrrelevantTermLabelsProperty. Term now no longer implements the Interface EqualsModProofIrrelevancy. Therefore it might be a problem if you wanted to use terms in an EquivalenceDirectedGraph.

Type of pull request

Ensuring quality

The contributions within this pull request are licensed under GPLv2 (only) for inclusion in KeY.

tobias-rnh commented 7 months ago

While testing the behaviour of the old equalsModProofIrrelevancy for terms to move it to a TermProperty, I discovered the following behaviour: If you take an unlabeled term and compare it to the same term with a proof-irrelevant label attached, equalsModProofIrrelevancy returns true, no matter on what term it is called. If however you take two labeled terms, that only have proof-irrelevant labels attached, but differ in the amount of those, equalsModProofIrrelevancy returns false, no matter on what term it is called.

I could imagine that this behaviour is not intended. But just to make sure that I do not break the intended behaviour, I wanted to ask @FliegendeWurst and @WolframPfeifer how equalsModProofIrrelevancy should act in these situations.

If this behaviour is indeed not intended, I would simply change it in my implementation. Otherwise, I will of course keep it this way. Thank you for your time!

FliegendeWurst commented 7 months ago

If however you take two labeled terms, that only have proof-irrelevant labels attached, but differ in the amount of those, equalsModProofIrrelevancy returns false, no matter on what term it is called.

That is indeed a bug. It should return true.

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 59.19118% with 111 lines in your changes are missing coverage. Please review.

Project coverage is 37.88%. Comparing base (8bf8306) to head (438a94d).

Files Patch % Lines
.../uka/ilkd/key/logic/equality/RenamingProperty.java 76.62% 7 Missing and 11 partials :warning:
...de/uka/ilkd/key/speclang/WellDefinednessCheck.java 7.14% 9 Missing and 4 partials :warning:
...ava/de/uka/ilkd/key/smt/AbstractSMTTranslator.java 0.00% 9 Missing :warning:
.../key/speclang/FunctionalOperationContractImpl.java 0.00% 6 Missing :warning:
...d/key/logic/equality/ProofIrrelevancyProperty.java 88.09% 1 Missing and 4 partials :warning:
...ka/ilkd/key/macros/scripts/InstantiateCommand.java 0.00% 5 Missing :warning:
...de/uka/ilkd/key/macros/scripts/RewriteCommand.java 0.00% 2 Missing and 2 partials :warning:
...va/de/uka/ilkd/key/macros/scripts/RuleCommand.java 20.00% 3 Missing and 1 partial :warning:
...e/uka/ilkd/key/rule/AbstractLoopInvariantRule.java 0.00% 4 Missing :warning:
.../java/de/uka/ilkd/key/rule/WhileInvariantRule.java 0.00% 1 Missing and 3 partials :warning:
... and 27 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3386 +/- ## ============================================ + Coverage 37.87% 37.88% +0.01% - Complexity 17079 17082 +3 ============================================ Files 2086 2090 +4 Lines 127465 127485 +20 Branches 21460 21466 +6 ============================================ + Hits 48273 48294 +21 - Misses 73280 73285 +5 + Partials 5912 5906 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tobias-rnh commented 7 months ago

I have again checked that all calls of equalsModProperty use the right property to match the calls to prior equalsMod... methods. Only in TestApplyUpdateOnRigidCondition, equalsModProofIrrelevancy was replaced by equalsModProperty with RENAMING_PROPERTY. This is okay and wanted as it actually should have been equalsModRenaming in the first place.

Apart from that, everything is as it should be and tests are passing.