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

[proof engine] Forbid tactic code to raise exceptions #15937

Closed
wants to merge 1 commit into from

Conversation

ejgallego
Copy link
Member

@ejgallego ejgallego commented Apr 21, 2022

Motivated by the recent discussion on Zulip about exceptions and
tactics, we move forward with the porting of tactic code to the
monadic proof engine and forbid tactics to raise non-critical
exceptions.

Instead, tactics calling exceptional code should catch and reify the
exception locally and use the proper proof engine API tclZero.

I think this is a very long due cleanup.

I hope @ppedrot likes this!

As of now, this is a draft, and not yet ready for benchmark. Stdlib
compiles, test suite is close to pass.

This should be combined with the work on PR #XXXX that adds exception
static analysis to Coq's codebase.

Next step is to remove wrap_exceptions, but that will require some
more tweaks as it appears is some critical paths.

(Hint: Click "hide whitespace" on the GH review tab to see relevant changes better)

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 21, 2022

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 21, 2022

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@ppedrot
Copy link
Member

ppedrot commented Apr 21, 2022

Actually, I am not sure I like this. It's part of the spec that enter blocks capture whatever exceptions are raised there. It's a bit fragile (you have to be sure that the exception-raising block is not inadvertently wrapped into a closure) but it also makes the code easier to read and to debug.

What should be totally forbidden is to raise exception from within the monadic layer, which is a different story.

@SkySkimmer
Copy link
Contributor

SkySkimmer commented Apr 21, 2022

This should be combined with the work on PR #XXXX that adds exception
static analysis to Coq's codebase.

What's this about?

@ejgallego
Copy link
Member Author

Actually, I am not sure I like this. It's part of the spec that enter blocks capture whatever exceptions are raised there. It's a bit fragile (you have to be sure that the exception-raising block is not inadvertently wrapped into a closure) but it also makes the code easier to read and to debug.

What should be totally forbidden is to raise exception from within the monadic layer, which is a different story.

Not sure I understand your what you mean in your comment, the idea here is that tactics should indeed never raise and rely on enter and friends having that compat layer, but instead use the proper engine primitive for failure which is tclZero. Maybe we can chat about it in the call / working group? Or would you mind expanding more / going to Zulip?

My main motivation is that I'd like for values of type Proofview.tactic to be 100% pure in the medium-term. I think this is a good idea and would help with many goals (sic). But that is the main point a comptemtion IMHO to decide if this PR is good or not.

There are a few more pieces missing to make this PR a reality tho, related to what @SkySkimmer was asking IMHO we also need:

  • to forbid user_err in any code use by tactics, and instead use the right exception
  • to annontate functions with the exceptions they raise and use a static analyzed (PR incoming)

as to make this robust and realitistic, as we can't hope to make all code fucntional in one go, or to port the pretyper to the monad. We could by the way wrap a few usages here in a monadic API, but that depends on how we feel about code.

Motivated by the recent discussion on Zulip about exceptions and
tactics, we move forward with the porting of tactic code to the
monadic proof engine and forbid tactics to raise non-critical
exceptions.

Instead, tactics calling exceptional code should catch and reify the
exception locally and use the proper proof engine API `tclZero`.

I think this is a very long due cleanup.

I hope @ppedrot likes this!

As of now, this is a draft, and not yet ready for benchmark. Stdlib
compiles, test suite is close to pass.

This should be combined with the work on PR #XXXX that adds exception
static analysis to Coq's codebase.

Next step is to remove `wrap_exceptions`, but that will require some
more tweaks as it appears is some critical paths.
@SkySkimmer
Copy link
Contributor

My main motivation is that I'd like for values of type Proofview.tactic to be 100% pure in the medium-term.

In the case of enter it's the goal -> tactic which raises not the tactic

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Apr 22, 2022

The job library:ci-fiat_crypto_legacy has failed in allow failure mode
ping @JasonGross

@ejgallego
Copy link
Member Author

My main motivation is that I'd like for values of type Proofview.tactic to be 100% pure in the medium-term.

In the case of enter it's the goal -> tactic which raises not the tactic

Yes, how would it be otherwise?

@github-actions github-actions bot added the needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. label May 31, 2022
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Jun 30, 2022

The "needs: rebase" label was set more than 30 days ago. If the PR is not rebased in 30 days, it will be automatically closed.

@coqbot-app coqbot-app bot added the stale This PR will be closed unless it is rebased. label Jun 30, 2022
@coqbot-app
Copy link
Contributor

coqbot-app bot commented Aug 1, 2022

This PR was not rebased after 30 days despite the warning, it is now closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: documentation Documentation was not added or updated. needs: rebase Should be rebased on the latest master to solve conflicts or have a newer CI run. part: proof engine stale This PR will be closed unless it is rebased.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants