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

Coq translator: fix array literal support #1815

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

eddywestbrook
Copy link
Contributor

This PR addresses issue #1813 by changing the translation of SAW core array literals to use Coq vector literals, rather than using Vector.of_list applied to an array literal. This ensures that the type of the generated vector literal does not depend on its input (which was the problem noted by #1813).

As also suggested in the discussion in that issue, this PR adds output types to Coq Definitions that are generated by translating cryptol modules.

…tput types of Coq Definitions generated from cryptol modules are annotated with the translations of their types
@eddywestbrook eddywestbrook added subsystem: cryptol-saw-core Issues related to Cryptol -> saw-core translation with cryptol-saw-core subsystem: saw-core-coq Issues related to converting saw-core to Gallina for use with the Coq/Rocq theorem prover labels Feb 7, 2023
Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

I haven't examined every change in close detail, but here are a handful of things that jumped out to me after a quick read.

@@ -666,26 +673,29 @@ translateTermToDocWith ::
TranslationReader ->
[String] ->
[String] ->
(Coq.Term -> Doc ann) ->
Term ->
(Coq.Term -> Coq.Term -> Doc ann) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth stating in a comment what these two Terms represent, since it's not obvious unless you stare at the code a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, good point. In fact, there probably needs to be even more haddock as well...

Copy link
Contributor

Choose a reason for hiding this comment

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

Does Haddock now support -- ^ after the arrow correctly?

@@ -102,7 +103,7 @@ From Coq Require Import String.
From Coq Require Import Vectors.Vector.
From CryptolToCoq Require Import SAWCoreScaffolding.
From CryptolToCoq Require Import #{vectorModule}.
Import ListNotations.
Import VectorNotations.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth expanding the Haddock to state why we import VectorNotations here: it's because we rely on this notation when translating array values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense

@eddywestbrook eddywestbrook added the PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run label Feb 8, 2023
@eddywestbrook eddywestbrook merged commit 9acd534 into master Feb 9, 2023
@mergify mergify bot deleted the saw-core-coq/vector-literals branch February 9, 2023 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run subsystem: cryptol-saw-core Issues related to Cryptol -> saw-core translation with cryptol-saw-core subsystem: saw-core-coq Issues related to converting saw-core to Gallina for use with the Coq/Rocq theorem prover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants