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

Added sealing of functions in traits #19

Closed
wants to merge 10 commits into from

Conversation

TheLazyDutchman
Copy link

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 the seal 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

  • Allow users to specify the name of the inner version of a wrapped function.
  • Allow users to specify the name of the token type that is used to make functions sealed.
  • Add use statement for the token, to make it nicer to call the sealed function.

@TheLazyDutchman
Copy link
Author

fixes #18

Copy link
Owner

@jmg-duarte jmg-duarte left a 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 and erased.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@TheLazyDutchman
Copy link
Author

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.

@jmg-duarte
Copy link
Owner

@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 Self::sealed(Token) instead).

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:

  • Rewriting every call to the function to use the generated value — very convoluted
  • Figuring out a syntax to make this approach work — possible but may be opaque to the end user, don't let this discourage you though!
  • Documenting the new pattern in the macro — easiest possible solution

@TheLazyDutchman
Copy link
Author

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 Token instead of _sealed_my_trait::Token everywhere where you are allowed to use the sealed trait.

Since this directly puts Token in scope it means we should allow the user to specify their own name for token, to avoid name collisions. (or instead of Token we make it MyTraitToken)

@jmg-duarte
Copy link
Owner

I'm not being able to visualize your proposal. Can you show an example of how the final code would look like?

@TheLazyDutchman
Copy link
Author

pub(super) _sealed_my_trait {
    pub trait Seal {}
    pub struct MyTraitToken;
}
pub(super) use _sealed_my_trait::MyTraitToken;

Where pub(super) is just to showcase what would happen if users specify a pub in the sealed attribute.

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.

@TheLazyDutchman
Copy link
Author

Another approach to making functions private is to put them in a subtrait, although I think it has slight semantic differences.

@jmg-duarte
Copy link
Owner

@TheLazyDutchman I wasn't too clear when asking for an example. I meant, how would an end-user write the code?

@TheLazyDutchman
Copy link
Author

#[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 Token is all they need, not the full path.
And we add to the documentation of the function that they need to supply the token.

@TheLazyDutchman
Copy link
Author

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).

@jmg-duarte
Copy link
Owner

As far as VSCode is concerned, it doesn't have a parameter, which becomes really misleading for the user.

Screenshot 2023-12-19 at 15 44 00

As you said:

but it cannot hide everything, which means it is unclear to the user where this token is coming from.

I have to thank you for your efforts but I cannot move forward with this PR. I am open to merge a PR documenting the pattern from the blog though as it is indeed a really interesting pattern and still relevant to the people interested in this crate.

@jmg-duarte jmg-duarte closed this Dec 19, 2023
@TheLazyDutchman
Copy link
Author

I was also unsatisfied with the Token trick and the wrapped function, and after thinking some more about it I think I have found a way to make a method be non-implementable but callable without using tokens.

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:

  • create a sealed wrapper type Wrapper<T>
  • impl<T> From<T> for Wrapper<T>
  • change one (or all) argument from e.g String to impl Into<Wrapper<String>> (this requires at least one argument.
  • add a line at the start of the function to rebind the argument name to the wrapped value.

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.

2 participants