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

Rewrite execProofMethod #650

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

felixlinker
Copy link
Contributor

This PR essentially rewrites execProofMethod and, following from that, applies some other changes. Overall, this PR should be "idempotent", i.e., Tamarin should function exactly as before. I personally find the code cleaner than before, but this is a matter of taste.

The intention behind this PR is to factor out changes to Tamarin that I did as part of a research project with @cascremers. They are necessary for that project, but we would like to check that they indeed do not change Tamarin's behavior. But still, I think that some benefit to the added clarity of the code (more deletions than insertions).

Some notes on the changes:

  • I added a new proof method Finished Result that replaces Solved, Contradiction and Unfinishable. I think it makes sense to reflect in the type system which kind of methods mean that no further proving needs to be done.
  • I do not check in execProofMethod whether the applied method is valid in the given constraint system. In most cases, these checks are redundant because they come from the system. Instead, I implemented checkAndExecProofMethod that is now used whenever one must check validity, e.g., upon loading a theory.
  • I deleted the annotateWith(Diff)Systems functions. They performed tasks fully redundant with execProofMethod. First, you would apply a proof method and delete the resulting constraint system, then you would annotate it again.
  • I now always simplify after applying a proof method. This makes the every now and then appearing "simplify" steps obsolete. To maintain backwards compatibility, I do not anymore return Nothing when simplify results in the same constraint system in execProofMethod. If I were to do that, applying a redundant simplify step (e.g., while loading older proofs) would invalidated the proof.
  • execProofMethod now checks for all ways in which a proof could be finished. Therefore, applyProverAtPath need not "guess" anymore which other provers we maybe should try to apply.

Fast regression tests pass in about the same time as before on my machine. Only difference is that there are slightly fewer steps which results from the now dropped simplify steps.

I'd appreciate a thorough review and am curious about any feedback!

@felixlinker
Copy link
Contributor Author

PS: I strongly suggest to merge by squashing and rebasing all commits. The commits themselves don't compile individually. I keep commits separate to be able to merge things back together in the end.

@cascremers
Copy link
Member

We'll look asap, thanks!
Note the only options are "squash and merge" or "rebase and merge" -- and I presume you want the first of these.

Copy link
Contributor

@niklasmedinger niklasmedinger left a comment

Choose a reason for hiding this comment

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

Hi Felix,

thanks a lot for your work! Much appreciated.

Since I only recently got into the code base, I can't give much feedback. I'm only wondering whether execProofMethod can now be infallible because checkAndExecProofMethod exists now.

There's a small copy-paste error regarding the comments of Result. Apart from that I think the PR looks good!


-- @execMethod rules method se@ applies the @method@ to the sequent under the
-- assumption that it is sound to apply the method and that the @rules@ describe
-- all rewriting rules in scope.
--
-- NOTE that the returned systems have their free substitution fully applied
-- and all variable indices reset.
execProofMethod :: ProofContext
-> ProofMethod -> System -> Maybe (M.Map CaseName System)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that execProofMethod no longer performs any sanity checks and relies on checkAndExecProofMethod for the checks, can the return type be changed to M.Map CaseName System?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that doesn't work because getting the induction cases can still fail.

contradiction c = (Contradiction (Just c), "")
execMethods = mapMaybe execMethod
execMethod (m, expl) = do
cases <- execProofMethod ctxt m sys
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a call to checkAndExecProofMethods now that execProofMethod no longer checks whether the method is valid? Or do we rely on execProofMethod returning Nothing if the execution fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rankProofMethods computes all applicable methods itself, i.e., they must be valid (or the computation is buggy, but this shouldn't be fixed by checking that they are valid imo). Thus, checking whether it's valid is not necessary. At this point in the code, it cannot come from an "external" source such as a loaded proof file.

@felixlinker
Copy link
Contributor Author

felixlinker commented Jun 19, 2024

We'll look asap, thanks! Note the only options are "squash and merge" or "rebase and merge" -- and I presume you want the first of these.

I mean the first of these, yes, but technically, you could also squash commits without rebasing ☝️🤓 (Doesn't make much sense, though.)

@jdreier
Copy link
Member

jdreier commented Jul 16, 2024

My quick merge seems not to have worked (although compilation fails on different file), I guess @felixlinker needs to help here.

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.

None yet

4 participants