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

fix(engine/lib): import associated item projection on generic bounds #765

Merged
merged 12 commits into from
Jul 25, 2024

Conversation

paulmure
Copy link
Contributor

Partially address #549.

The importer in the engine now imports projections defined in hax_frontend_exporter::types::copied::ClauseKind.

This is done by adding a variant to the Ast.Rust.generic_constraint enum to include projections, which necessitates changing the interface of import_clause in engine/lib/import_thir.mli to return Ast.Rust.generic_constraint instead of Ast.Rust.impl_ident.

As a result, all functions that previously expect an Ast.Rust.impl_ident from c_clause_kind now do some additional filtering.

Remaining issues:

@paulmure
Copy link
Contributor Author

@W95Psp, the fstar backend currently fails the traits test as I do not know how this additional feature should be dealt with there. Please let me know if you have any ideas on how to push this forward.

@W95Psp W95Psp self-requested a review July 18, 2024 06:21
@W95Psp
Copy link
Collaborator

W95Psp commented Jul 18, 2024

Hi! Thanks for your PR!

It looks good to me, I can add the missing bits for the F* backend, or we can just filter that out for now (I'm not entirely sure about what is the best encoding in F*). Do you want to take care of the filtering in the F* and Coq backends?

For now, let's not feature-gate this new variant, let's see if that makes sense at some point for some backends.

(What's up with the git history though? 👀)

@paulmure
Copy link
Contributor Author

paulmure commented Jul 18, 2024

Hi! Thanks for your PR!

It looks good to me, I can add the missing bits for the F* backend, or we can just filter that out for now (I'm not entirely sure about what is the best encoding in F*). Do you want to take care of the filtering in the F* and Coq backends?

For now, let's not feature-gate this new variant, let's see if that makes sense at some point for some backends.

(What's up with the git history though? 👀)

The coq backend is surprisingly unaffected by this change. The ssprove backend is though, so I just called Error.unimplemented there.

For the F* backend, I filtered GCProjections out with a TODO comment linking to #785.

The git history was probably because I accidentally merged some changes in main...

Copy link
Collaborator

@W95Psp W95Psp left a comment

Choose a reason for hiding this comment

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

Great, thanks!
Ok, that will be a merge commit anyway, let's merge as is!

@W95Psp W95Psp added this pull request to the merge queue Jul 25, 2024
Merged via the queue into hacspec:main with commit d6cc188 Jul 25, 2024
13 checks passed
@paulmure paulmure deleted the fix-associated-item-projection branch August 2, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants