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

Engine: propagate trait generics arguments #751

Merged
merged 6 commits into from
Jul 10, 2024

Conversation

W95Psp
Copy link
Collaborator

@W95Psp W95Psp commented Jul 8, 2024

This PR requires #698.

This PR propagates trait generics arguments throughout the engine.

On the F* output:

  • now the calls to methods will always be method #self_type #trait_generic_arg1 ... #trait_generic_argN #Typeclasses.solve #method_generic_type1 ... #method_generic_typeM method_generic_const1 ... method_generic_constI meth_arg1 ... meth_argJ
  • traits and inductive (enums/structs) should have all their argument explicit

Todo:

  • use the information in the F* backend
  • propagate the information in the proof libs

When undrafted, this PR should fix #719

@W95Psp W95Psp force-pushed the engine-propagate-trait-generics branch from 033f4fb to f5881dd Compare July 9, 2024 07:52
@W95Psp W95Psp marked this pull request as ready for review July 9, 2024 08:26
@W95Psp W95Psp requested a review from mamonet July 9, 2024 08:26
@W95Psp
Copy link
Collaborator Author

W95Psp commented Jul 9, 2024

I think this should work, maybe there are a few things in the library we need to fix, but we will discover those by running hax on various things.

@mamonet can you try this branch on ml-kem?
If you run in anything suspicious related to implcits or something similar, tell me directly!

@franziskuskiefer
Copy link
Member

@W95Psp this doesn't build with the same issue as in the visitor PR

# File "utils/generate_visitors/errors.ml", line 27, characters 29-35:
# 27 |     | UnsupportedCoreType of string
#                                   ^^^^^^

@W95Psp
Copy link
Collaborator Author

W95Psp commented Jul 10, 2024

Ah, yes, thanks @franziskuskiefer, will rebase this PR with generate-visitors to propagate that change

@W95Psp W95Psp force-pushed the engine-propagate-trait-generics branch from 1cabfda to e7b724a Compare July 10, 2024 03:50
@W95Psp W95Psp changed the base branch from main to generate-visitors July 10, 2024 03:50
Base automatically changed from generate-visitors to main July 10, 2024 04:28
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.

lgtm when it works for @mamonet

tests/traits/src/lib.rs Outdated Show resolved Hide resolved
@mamonet
Copy link
Collaborator

mamonet commented Jul 10, 2024

This PR seems to be solving the issue. I went through the instances that I'm aware of, and all of them verified successfully. However, I got one undesired change here https://github.com/cryspen/libcrux/blob/dev_ml_kem_lax/libcrux-ml-kem/proofs/fstar/extraction/Libcrux_ml_kem.Polynomial.fsti#L35-L37
This create an error Expected type Type but t_PolynomialRingElement v_Vector has type i1: Vector.Traits.t_Operations v_Vector -> Type0 in references of t_PolynomialRingElement
Here is the original type https://github.com/cryspen/libcrux/blob/main/libcrux-ml-kem/proofs/fstar/extraction/Libcrux_ml_kem.Polynomial.fsti#L35-L37

@W95Psp
Copy link
Collaborator Author

W95Psp commented Jul 10, 2024

Fantastic!
Thanks for your feedback! I see what it is, give me 10-15 minutes I will push a fix!

@W95Psp
Copy link
Collaborator Author

W95Psp commented Jul 10, 2024

@mamonet, I pushed a commit, can you tell me if that's fixing that regression? 😃

@mamonet
Copy link
Collaborator

mamonet commented Jul 10, 2024

Of course, just 2 minutes to report back

@mamonet
Copy link
Collaborator

mamonet commented Jul 10, 2024

The fix gets t_PolynomialRingElement back to what it is, I think this PR is in a good shape now.

@W95Psp
Copy link
Collaborator Author

W95Psp commented Jul 10, 2024

Ah, that's great news!
I'm gonna rebase the PR to main, and then we can merge it! 🥳

@W95Psp W95Psp force-pushed the engine-propagate-trait-generics branch from 5258952 to a17cd49 Compare July 10, 2024 09:21
@W95Psp W95Psp enabled auto-merge July 10, 2024 09:21
@W95Psp W95Psp added this pull request to the merge queue Jul 10, 2024
Merged via the queue into main with commit 86e2581 Jul 10, 2024
13 checks passed
@W95Psp W95Psp deleted the engine-propagate-trait-generics branch July 10, 2024 09:55
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.

Some class parameters are incorrectly passed
3 participants