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

feat: add impl-level exporting QoL macro #707

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

Conversation

HoloTheDrunk
Copy link

@HoloTheDrunk HoloTheDrunk commented May 12, 2024

This adds a new macro that allows for more concise exporting of a lot of functions.
Currently in a sort of PoC state, a lot can and will be improved.

Why ?

Having to edit multiple places when making simple changes to an API is tedious and a source of error.

How ?

#[derive(rune::Any)]
struct MyStruct(#[rune(get)] usize);

// Creates a function that returns an array of all the function metadata
#[rune::item_impl]
impl MyStruct {
    #[rune(export)]
    pub fn succ(&self) -> Self {
        Self(self.0 + 1)
    }
}

Roadmap

  • Proper documentation
  • Non-instance function handling

I had to add the extra-traits syn feature, if you know of a way to do the comparison on crates/rune-macros/src/item_impl.rs:70 without it that'd be pretty cool.

@udoprog udoprog added the enhancement New feature or request label May 12, 2024
@HoloTheDrunk
Copy link
Author

HoloTheDrunk commented May 20, 2024

If I haven't missed anything, the PR should be ready for review whenever you've got time :)
edit: oops, I had in fact forgotten things

Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

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

Hi!

I'm not entirely convinced this pattern is preferable as it stands. The function metadata macros are already complex as it is, and adding another layer should be well motivated. I'll have to think about it some more.

To note on the current implementation, you should probably prefer having it implement a trait on the type being implemented (like InstallWith) rather than the bare exporter and list functions. You should prefer using an approach where module installer functions should belong to the module rather than the type being exported, since this is the pattern for installing everything else.

Since it is a type, maybe something like this:

let mut module = /* module being declared */;
module.ty_with_exports::<T>();
module.ty::<T>().with_exports();

That either which requires the Exports trait or whatever you opt to call it to be implemented and the appropriate trait methods to install all exported items will be called.

let docs = reparsed.docs;
let arguments = reparsed.arguments;
let meta_kind = syn::Ident::new(
["function", "instance"][reparsed.takes_self as usize],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit too clever, prefer just using an if statement instead.

@@ -77,6 +78,54 @@ pub fn function(
output.into()
}

/// Create a function to export all functions marked with the `#[rune(export)]` attribute within a module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

within an impl block, not a module?

@HoloTheDrunk
Copy link
Author

HoloTheDrunk commented Jun 3, 2024

Hi! Thank you for the review and sorry for the delay.

The main motivation for grouping things like this is to avoid the need to have a human error-prone process when altering a type's API as you jump between definition and export locations. Most projects will likely have pretty simple export configurations so this method also streamlines the experience when trying out the library for example, leaving the specifics for later (e.g. I originally expected keep to be the default behaviour for function exports and almost dropped the idea of using Rune when it wasn't mentioned anywhere simply due to the added maintenance time cost).

Though using a trait would be much cleaner, it'd make it impossible to export multiple impl blocks using this method; I don't know how much of a problem that is as I usually don't split my impls but I don't have any data on how common/useful that practice is to others.
If it's not considered a concern for the scope of Rune I'll gladly replace everything with a trait and a corresponding module function.

I'll make those changes within the a couple of days if/when I get your go on this :)

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

Successfully merging this pull request may close these issues.

2 participants