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

Opaque extensions #1134

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Opaque extensions #1134

wants to merge 10 commits into from

Conversation

maximebuyse
Copy link
Contributor

@maximebuyse maximebuyse commented Nov 25, 2024

Fixes #1119

We implement this design. The attribute opaque is introduced and allows to drop the content (erase) of functions, types, trait impls, consts, etc. to make them abstract. In this case we produce a type signature and an abstract definition (using assume in fstar). The interface-only include flag (cargo hax into -i '+:.....) allows to erase easily many items. It relies on default rules that say if a selected item should be erased or not (see design doc).

@maximebuyse maximebuyse force-pushed the opaque-extensions branch 2 times, most recently from faa2462 to e1ec170 Compare November 27, 2024 10:24
@maximebuyse
Copy link
Contributor Author

Adding a note here that I noticed while reviewing the test result for tests/cli/interface-only. This previously produced only an fsti file, it now produces only an fst file. This makes sense because it uses the -i '+:' flag but no --interfaces flag for the fstar backend. As we now provide definitions for erased items we get them in an fst file. The definitions can still be discarded using --interfaces +!**. Maybe it would be worth documenting this but I am not sure where (I started with updating the documentation of the -i flag).

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

Thanks, I'm reviewing now.
Some snapshots are conflicting though: can you update them?

@maximebuyse
Copy link
Contributor Author

Thanks, I'm reviewing now. Some snapshots are conflicting though: can you update them?

Done. I also pushed a small commit to allow making a single method opaque inside an impl (regular non-trait impl).

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.

Thanks, that looks good to me :)
I have some minor comments.

I still need to test the PR on snippets of code.

engine/lib/import_thir.ml Outdated Show resolved Hide resolved
engine/lib/import_thir.ml Show resolved Hide resolved
hax-lib/macros/types/src/lib.rs Outdated Show resolved Hide resolved
@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

Thanks, I'm reviewing now. Some snapshots are conflicting though: can you update them?

Done. I also pushed a small commit to allow making a single method opaque inside an impl (regular non-trait impl).

Oh great, that's a nice addition!

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

We also need the opaque attribute to work on traits I think. We forgot that case in the design document.

#[hax_lib::opaque]
trait Foo<X, Y> {
  ...
}

Should be translated to:

assume type foo: Type -> Type -> Type

(and for fsti val foo: Type -> Type -> Type)

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

#[hax_lib::opaque]
fn opaque_function(){}

fn regular_function(){}

Open this code snippet in the playground

Gets translated to:

val opaque_function: Prims.unit -> Prims.unit
assume val opaque_function': Prims.unit -> Prims.unit
let opaque_function = opaque_function'

let regular_function (_: Prims.unit): Prims.unit = ()

I don't think we want the val opaque_function, do we?

@maximebuyse
Copy link
Contributor Author

#[hax_lib::opaque]
fn opaque_function(){}

fn regular_function(){}

Open this code snippet in the playground

Gets translated to:

val opaque_function: Prims.unit -> Prims.unit
assume val opaque_function': Prims.unit -> Prims.unit
let opaque_function = opaque_function'

let regular_function (_: Prims.unit): Prims.unit = ()

I don't think we want the val opaque_function, do we?

We want it in the fsti (if we have an fsti). I can generate it only in interface mode.

@maximebuyse
Copy link
Contributor Author

We also need the opaque attribute to work on traits I think. We forgot that case in the design document.

#[hax_lib::opaque]
trait Foo<X, Y> {
  ...
}

Should be translated to:

assume type foo: Type -> Type -> Type

(and for fsti val foo: Type -> Type -> Type)

Yes that can be nice when we have unallowed &mut in method signatures. I implemented that but maybe we should be careful about an opaque trait that has non-opaque impls. I think it could be nice to somehow enforce this (but maybe it is not a priority.

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

Yes that can be nice when we have unallowed &mut in method signatures. I implemented that but maybe we should be careful about an opaque trait that has non-opaque impls. I think it could be nice to somehow enforce this (but maybe it is not a priority.

Indeed, let's file an issue with a good description then. I think we don't care for this PR.

@maximebuyse
Copy link
Contributor Author

Yes that can be nice when we have unallowed &mut in method signatures. I implemented that but maybe we should be careful about an opaque trait that has non-opaque impls. I think it could be nice to somehow enforce this (but maybe it is not a priority.

Indeed, let's file an issue with a good description then. I think we don't care for this PR.

Yes, let's do that. I am not sure what you described worked by the way. With:

val t_T (v_Self v_U: Type0) : Type0

[@@ FStar.Tactics.Typeclasses.tcinstance]
val impl:t_T u8 u8

fstar complains with:

Instances must define instances of `class` types.
  - Type
    Opaque_impl.t_T Rust_primitives.Integers.u8 Rust_primitives.Integers.u8
    is not a class.

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 27, 2024

Ah, you are right, there's a catch, you need an attribute to mark the val (or assume val) as a typeclass, as follows:

[@@ FStar.Tactics.Typeclasses.tcclass]
val t_T (v_Self v_U: Type0) : Type0

[@@ FStar.Tactics.Typeclasses.tcinstance]
val impl:t_T u8 u8

@maximebuyse
Copy link
Contributor Author

#[hax_lib::opaque]
fn opaque_function(){}

fn regular_function(){}

Open this code snippet in the playground

Gets translated to:

val opaque_function: Prims.unit -> Prims.unit
assume val opaque_function': Prims.unit -> Prims.unit
let opaque_function = opaque_function'

let regular_function (_: Prims.unit): Prims.unit = ()

I don't think we want the val opaque_function, do we?

@W95Psp This is fixed now

@W95Psp
Copy link
Collaborator

W95Psp commented Nov 28, 2024

We chatted quickly about this with @maximebuyse, the opaque traits are not implemented yet, either we implement that here or we open a follow up issue.
It's unlikely I will have time to do more review here by the end of the day, I will do that Monday morning

@maximebuyse
Copy link
Contributor Author

We just noticed a few problems here:

  • For the case of types we should generate something similar as other cases in the fst (assume val with a let alias)
  • The let alias should take and pass the generic arguments otherwise fstar fails.
  • We should have tests for all of that, also with trait bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow impls to be opaque
2 participants