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

Move trait methods in cyclic dependencies bundling. #1075

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

Conversation

maximebuyse
Copy link
Contributor

Fixes #1068

@maximebuyse maximebuyse requested a review from W95Psp October 30, 2024 11:32
@W95Psp
Copy link
Collaborator

W95Psp commented Oct 30, 2024

Mmh, here the problem is that we assume (in the F* backend!) that a top-level foo exists in the module where a trait with a method foo was defined.
This is true only because of how F* deals with type-classes: we can't "leak" this backend-specific representation globally.

Also, we want to get rid of type-classes in F*, when we will do so, things will break.

I see two options:

  1. I can fix F* upstream to make that include Foo {my_typeclass} by default includes also the methods
  2. we can hack something in the F* backend

What do you think? Would (2) be easy enough? I won't have time to do (1) before my holidays.

@maximebuyse
Copy link
Contributor Author

Mmh, here the problem is that we assume (in the F* backend!) that a top-level foo exists in the module where a trait with a method foo was defined. This is true only because of how F* deals with type-classes: we can't "leak" this backend-specific representation globally.

Also, we want to get rid of type-classes in F*, when we will do so, things will break.

I see two options:

1. I can fix F* upstream to make that `include Foo {my_typeclass}` by default includes also the methods

2. we can hack something in the F* backend

What do you think? Would (2) be easy enough? I won't have time to do (1) before my holidays.

I am not sure (1) would work because then things in the bundle could still refer to the original module creating a cyclic dependency. And if the plan is to get rid of type classes someday I guess (2) makes sense. I'll try to do something like that.

@maximebuyse
Copy link
Contributor Author

Mmh, here the problem is that we assume (in the F* backend!) that a top-level foo exists in the module where a trait with a method foo was defined. This is true only because of how F* deals with type-classes: we can't "leak" this backend-specific representation globally.
Also, we want to get rid of type-classes in F*, when we will do so, things will break.
I see two options:

1. I can fix F* upstream to make that `include Foo {my_typeclass}` by default includes also the methods

2. we can hack something in the F* backend

What do you think? Would (2) be easy enough? I won't have time to do (1) before my holidays.

I am not sure (1) would work because then things in the bundle could still refer to the original module creating a cyclic dependency. And if the plan is to get rid of type classes someday I guess (2) makes sense. I'll try to do something like that.

Actually I think what we want is a modified version of what I implemented in this PR. We want to do the renaming of references to the trait method, we just don't want the includes in the original module for the trait methods.

The problem with this approach is if we translate a library (that contains a trait that gets bundled) and then some other crate that uses the trait from the library. If we translate them with two different applications of hax, then we might refer to the trait methods in the wrong module. But is it possible to translate things independently like that @W95Psp ?

@W95Psp
Copy link
Collaborator

W95Psp commented Oct 30, 2024

Yes, I agree: we want to renaming, but maybe not the includes.
However, aren't the renaming local to the moved items? e.g. only renaming occurs only for items within a bundle?

And yes, definitively, this would be an issue when we extract two different crates where one uses the other.
We do so in libcrux, and we want to support that, generally.

@maximebuyse
Copy link
Contributor Author

Yes, I agree: we want to renaming, but maybe not the includes. However, aren't the renaming local to the moved items? e.g. only renaming occurs only for items within a bundle?

And yes, definitively, this would be an issue when we extract two different crates where one uses the other. We do so in libcrux, and we want to support that, generally.

After discussing this the decision is to go with (2). We'll open an issue to get rid of this once we stop using fstar type classes.

@maximebuyse
Copy link
Contributor Author

@W95Psp I implemented what we discussed. #1078 is the issue to keep track of this hack and remember to get rid of it when possible.

@franziskuskiefer
Copy link
Member

Let's close this one or #1126 but don't spend too much time on it. We should work on #1135 instead to fix the issue in general.

@maximebuyse
Copy link
Contributor Author

Let's close this one or #1126 but don't spend too much time on it. We should work on #1135 instead to fix the issue in general.

I think this one is the best of the two. @W95Psp Let's try to merge this one, and we'll improve as part of #1135

@maximebuyse maximebuyse requested a review from W95Psp November 28, 2024 13:14
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.

Trait methods are not moved by bundling
3 participants