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

Allow deriving protocols with Any #587

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

Conversation

ModProg
Copy link
Contributor

@ModProg ModProg commented Jul 25, 2023

Usage

#[derive(Any, Clone, Display, Debug)]
#[rune(item = ::std::env, protocols(STRING_DISPLAY, STRING_DEBUG, TRY=Self::branch))]
#[display("{}", value.as_deref().unwrap_or_default())]
#[debug("{name}={:?}", value.as_deref().unwrap_or_default())]
struct Var {
    name: String,
    value: Option<String>,
}

impl Var {
    fn branch(self) -> Result<Self, Result<(), Error>> {
        if self.value.is_some() {
            Ok(self)
        } else {
            Err(Err(anyhow!(
                "Environment variable `{}` not set.",
                self.name
            )))
        }
    }
}

fixes #583

@ModProg
Copy link
Contributor Author

ModProg commented Jul 25, 2023

One thing to decide is whether to require the Self:: as protocols are likely often associated functions.

This would mean that:

  1. external functions need a qualification i.e. super::something or similar
  2. functions in the containing module would need to be specified with self::something.

So this would boil down, would we rather require Self on associated functions or self on non-associated functions.

@ModProg
Copy link
Contributor Author

ModProg commented Jul 25, 2023

This now also includes 0adc1bb, which is related but not equivalent for associated functions.

#[derive(Any, Clone)]
#[rune(item = ::std::env)]
#[rune(functions(Self::is_set))]
struct Var {
    name: String,
    value: Option<String>,
}

impl Var {
    #[function]
    fn is_set(&self) -> bool {
        self.value.is_some()
    }
}

@ModProg
Copy link
Contributor Author

ModProg commented Jul 25, 2023

Also, very much WIP, if you agree overall, I'll add the missing traits. And we'll also need documentation and tests.

@udoprog
Copy link
Collaborator

udoprog commented Jul 26, 2023

Thanks!

I'm out now! I'll check it out when I get back next week.

@udoprog udoprog changed the title allow to derive protocols with Any Allow deriving protocols with Any Jul 29, 2023
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.

Great feature, thank you!

Documentation and tests and I'd consider it done, even if we need to add more protocols later on.

The one nit I have about syntax is that I'd probably prefer the use #[rune_derive(..)] and #[rune_functions(..)] over #[rune(protocols(..)) and #[rune(functions(..))]. I'm not a huge fan of nested attributes.

Regarding naming, what do you say?

crates/rune-macros/src/context.rs Outdated Show resolved Hide resolved
crates/rune-macros/src/context.rs Outdated Show resolved Hide resolved
crates/rune-macros/src/context.rs Show resolved Hide resolved
crates/rune-macros/src/context.rs Outdated Show resolved Hide resolved
crates/rune-macros/src/context.rs Outdated Show resolved Hide resolved
@udoprog udoprog added the enhancement New feature or request label Jul 29, 2023
@ModProg
Copy link
Contributor Author

ModProg commented Jul 30, 2023

The one nit I have about syntax is that I'd probably prefer the use #[rune_derive(..)] and #[rune_functions(..)] over #[rune(protocols(..)) and #[rune(functions(..))]. I'm not a huge fan of nested attributes.

whatever you prefer, I don't have a strong feeling there.

@ModProg
Copy link
Contributor Author

ModProg commented Jul 31, 2023

@udoprog what is your opinino on this?

One thing to decide is whether to require the Self:: as protocols are likely often associated functions.

This would mean that:

  1. external functions need a qualification i.e. super::something or similar

  2. functions in the containing module would need to be specified with self::something.

So this would boil down, would we rather require Self on associated functions or self on non-associated functions.

@udoprog
Copy link
Collaborator

udoprog commented Jul 31, 2023

Seems fine. For the way it's implemented you should even be able to omit self:: if the function is unambiguous.

@ModProg
Copy link
Contributor Author

ModProg commented Jul 31, 2023

Seems fine. For the way it's implemented you should even be able to omit self:: if the function is unambiguous.

currently yes the question was about changing this, i.e. making single ident function names be associated by default and only resolve paths relatively to the module.

i.e. "the_fn" would be "Self::the_fn" and "self::the_fn" would be "self::the_fn", i.e. the containing module.

Currently it is "Self::the_fn" is "Self::the_fn" and "the_fn" would resolve to "self::the_fn" i.e. the containing module.

@udoprog
Copy link
Collaborator

udoprog commented Jul 31, 2023

Oh I see. Well, then no. Let them resolve to whatever path they're part of? They're declared within an impl that sees Self, so that ends up being a legal path. I don't see the reason why you'd want to make it more explicit.

/// fn second(it: Struct) -> usize {
/// it.1
/// }
/// ```
pub use rune_macros::Any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@udoprog The docs for the Any macro are actually not shown. Not sure if that is due to being reexported again at the crate root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT the only way around that, is moving this reexport in the crate root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Welp, time to move!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what would be your preference? Add the documentation in rune-macros or in rune's lib.rs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Move the re-export and document it. Otherwise you have a circular dev-dependency for any doc tests that uses rune which rust-analyzer does not like. And all the documentation in one place.

@udoprog
Copy link
Collaborator

udoprog commented Aug 12, 2023

PR looks to be in pretty good shape with a few things that can be done later. Would you be OK with me going over, clean a few things up and merging it?

@ModProg
Copy link
Contributor Author

ModProg commented Aug 12, 2023

PR looks to be in pretty good shape with a few things that can be done later. Would you be OK with me going over, clean a few things up and merging it?

Sounds good.

IIRC one thing that needs fixing is the String type in DISPLAY_STRING. Either, rune needs a rune::__private::String that maps to the correct String dependent on if std is present. Otherwise, one could also have a feature in rune-macros for std.

@Roba1993
Copy link
Contributor

What is blocking this feature right now? I would be really interested for this functionality.

@udoprog
Copy link
Collaborator

udoprog commented Mar 13, 2024

Rebasing, cleaning up the PR, and bandwidth.

I'll have another stint working on Rune eventually, and then I plan to pick it up again. But if you're interested, please go ahead!

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.

Support derive(Any) type protocols
3 participants