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

Question mark as early returns #559

Merged
merged 12 commits into from
Mar 12, 2024
Merged

Question mark as early returns #559

merged 12 commits into from
Mar 12, 2024

Conversation

W95Psp
Copy link
Collaborator

@W95Psp W95Psp commented Mar 7, 2024

This PR adds a few phases:

  • Drop_needless_returns;
  • Simplify_question_marks;
  • Simplify_match_return;
  • Simplify_hoisting.

Essentially, this PR is about question marks:

  • Simplify_question_marks goes through the AST and detects matches that looks like question marks (just as Reconstruct_question_marks, which is now deprecated since we want to drop it, see Remove Reconstruct_question_marks phase and Question_mark nodes from the AST #567)
  • such ? matches are produced by rustc and are quite complicated (it involves https://doc.rust-lang.org/std/ops/trait.Try.html): Simplify_question_marks simplify them into simple Err/Ok (or None/Some) matches;
  • the last branch of those ?-matches are early returns: Simplify_match_return does some inlining so that those early returns are pushed to exit position, becoming non-early returns (note this mechanism doesn't handle every case);
  • Drop_needless_returns removes useless return statements;
  • Simplify_hoisting removes extra hoistN let-bindings.

This PR fixes:

@franziskuskiefer
Copy link
Member

Please add an issue and put it on the board.

@jschneider-bensch
Copy link
Collaborator

Not a review, but I've tried this out for the ProVerif backend, basically copying the phase configuration from the F* backend and it works. (#564 is the result)

@W95Psp W95Psp force-pushed the question-mark-as-early-returns branch from 32dc2e8 to 7533949 Compare March 12, 2024 09:37
@W95Psp W95Psp marked this pull request as ready for review March 12, 2024 09:37
@W95Psp
Copy link
Collaborator Author

W95Psp commented Mar 12, 2024

@jschneider-bensch I rebased my PR, can you rebase yours onto that qustion-mark-as-early-returns branch?

That's great that those patches work for you in the PV backend 😀

Copy link
Member

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

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

Thanks!

You are adding phases. Where is the documentation for the phases? As there's probably none yet, this is a good point to start. Please add docs to https://hacspec.org/hax with placeholders for the other phases and some infos on the ones you added here (an extended version of what you have in the PR is enough).

It should also answer questions like which other cases in "note this mechanism doesn't handle every case"?

@W95Psp W95Psp force-pushed the question-mark-as-early-returns branch from 99f6d3c to 8d203fc Compare March 12, 2024 14:19
@W95Psp
Copy link
Collaborator Author

W95Psp commented Mar 12, 2024

So, I had to do a bit of stupid macro things in OCaml, but now we have documentation for phases gathered in one place: https://hacspec.org/hax/engine/hax-engine/Hax_engine/Phases/index.html (this will be up to date whenever the PR is merged)

@W95Psp W95Psp force-pushed the question-mark-as-early-returns branch from 8d203fc to 4ce4fbc Compare March 12, 2024 14:31
@W95Psp W95Psp force-pushed the question-mark-as-early-returns branch from 4ce4fbc to f3e21a3 Compare March 12, 2024 14:33
@W95Psp W95Psp enabled auto-merge March 12, 2024 14:37
@W95Psp W95Psp added this pull request to the merge queue Mar 12, 2024
Merged via the queue into main with commit 3855c88 Mar 12, 2024
11 checks passed
@W95Psp W95Psp deleted the question-mark-as-early-returns branch March 12, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants