-
Notifications
You must be signed in to change notification settings - Fork 5
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
Added sealing of functions in traits #19
Conversation
fixes #18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking the time to submit a PR!
I've done a first pass, it's looking good but there's still work to do.
Asides from the comments I left through the code, I ask you:
- Rebase/merge the new master into your changes, I've updated some things that will hopefully make developer experience more consistent.
- Replace new strings using line breaks with
concat!
— it formats much better and I find it more legible (especially in narrower windows). - Add tests using both
partial
anderased
.
0e47787
to
875acc3
Compare
One thing I am not happy about is that the way to make a sealed function callable is by adding a wrapper function that calls the sealed function. But since this wrapper function is not sealed, and (in my current implementation) has the normal name and arguments. This means it is possible (in fact it is quite easy to do accidentally) to overwrite the wrapper that is used to call the sealed implementation. |
@TheLazyDutchman I was reviewing your code and going through the examples when I noticed the following the following code does not work: #[sealed(partial)]
pub trait A {
#[seal(uncallable)]
fn sealed() {}
fn lol(&self) {
Self::sealed();
}
} Now, it makes sense that it doesn't! The issue is, it needs to work. As a library author, you need to be able to write the previous code (with, of course, the This works when done by hand because you're in full control of the names, but not with macros (as Rust doesn't have the same abilities as F#). I see 3 possible ways out of this:
|
I think we can add doc attrs to document that token needs to be used, and then have a use statement for the token with the same publicity of the sealed module. This way you can just use Since this directly puts |
I'm not being able to visualize your proposal. Can you show an example of how the final code would look like? |
Where This doesn't change that users need to add the token, but at least they don't need to use the path of the module, and we can change the doc comments of the function to signify that they need the token in order to call it. |
Another approach to making functions private is to put them in a subtrait, although I think it has slight semantic differences. |
@TheLazyDutchman I wasn't too clear when asking for an example. I meant, how would an end-user write the code? |
#[sealed]
pub trait MyTrait {
#[seal(uncallable)]
fn sealed() {}
fn unsealed() {
Self::sealed(Token);
}
} They still need to add the token, but we have a use statement for it, so |
I am not sure if it is possible to get the full semantics of a sealed function without adding an argument (at least that is what was done in the blog). The problem is that when doing it manually, you know what is going on, and this library would be hiding some of it (which is a good thing), but it cannot hide everything, which means it is unclear to the user where this token is coming from. I think this can be somewhat fixed by adding documentation (both for this library, and for the function that we are sealing). |
I was also unsatisfied with the But since this library normally already does exactly that for all methods, it would create two completely distinct behaviours, so I think it might be better as a separate library. The new trick would be:
|
This PR adds the ability to seal specific functions.
This is done through adding the
#[seal]
attribute on the function.This makes the entire function private.
If you want the function to be callable but not implementable, you need to add
callable
to theseal
attribute.Since sealing the trait is semantically somewhat equivalent to adding
#[seal(callable)]
to all its functions.To allow users more freedom, I added a
partial
argument to the trait attribute.This attribute removes the seal from the trait itself, so that the user has almost full freedom in what functions are implementable and what are callable.
Possible future additions