rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.77k stars 178 forks source link

Broken `export_modules` macro and/or `NativeCallContext` #892

Closed VorpalBlade closed 3 months ago

VorpalBlade commented 3 months ago

The following code creates a warning with respect to rust2018 idioms:

#[export_module]
pub(crate) mod properties {
    /// Get a property
    pub fn get(context: NativeCallContext, name: ImmutableString) -> Dynamic {
        // TODO
        Dynamic::from(())
    }
}

The following warning:

warning: hidden lifetime parameters in types are deprecated
  --> crates/mycrate/src/plugins/properties.rs:16:25
   |
16 |     pub fn get(context: NativeCallContext, name: ImmutableString) -> Dynamic {
   |                         ^^^^^^^^^^^^^^^^^ expected lifetime parameter
   |
help: indicate the anonymous lifetime
   |
16 |     pub fn get(context: NativeCallContext<'_>, name: ImmutableString) -> Dynamic {
   |                                          ++++

However following that suggestion creates a compilation error:

error[E0277]: the trait bound `rhai::NativeCallContext<'_>: Clone` is not satisfied
    --> crates/konfigkoll_script/src/plugins/properties.rs:16:25
     |
16   |     pub fn get(context: NativeCallContext<'_>, name: ImmutableString) -> Dynamic {
     |                         -----------------^^^^
     |                         |
     |                         the trait `Clone` is not implemented for `rhai::NativeCallContext<'_>`
     |                         required by a bound introduced by this call
     |
note: required by a bound in `rhai::Dynamic::cast`
    --> /home/arvid/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rhai-1.19.0/src/types/dynamic.rs:1631:26
     |
1631 |     pub fn cast<T: Any + Clone>(self) -> T {
     |                          ^^^^^ required by this bound in `Dynamic::cast`

It seems something is quite broken (probably in the proc macro?)

VorpalBlade commented 3 months ago

In addition #[allow(elided_lifetimes_in_paths)] is apparently only settable at a crate level (I tried to set it for just the plugin module), so that makes it pretty much unusable for me.

Since I forgot in the above report:

VorpalBlade commented 3 months ago

Seems this was supposed to be fixed in #864 but isn't? Since I run the latest version. Or maybe you only published the main crate and not the fixed proc macro?

schungx commented 3 months ago

Thats true... It is supposed to have been fixed.

Can you do a cargo update?

VorpalBlade commented 3 months ago

cargo update didn't help, no

schungx commented 3 months ago

Hhhhmmm..... I cannot reproduce it. It does not show an error for me.

VorpalBlade commented 3 months ago

I have made a very small reproducer: rhai_reproducer.tar.gz

error[E0277]: the trait bound `rhai::NativeCallContext<'_>: Clone` is not satisfied
    --> src/main.rs:6:22
     |
6    |     pub fn test(ctx: NativeCallContext<'_>) -> u32 {
     |                      -----------------^^^^
     |                      |
     |                      the trait `Clone` is not implemented for `rhai::NativeCallContext<'_>`
     |                      required by a bound introduced by this call
     |
note: required by a bound in `rhai::Dynamic::cast`
    --> /home/arvid/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rhai-1.19.0/src/types/dynamic.rs:1631:26
     |
1631 |     pub fn cast<T: Any + Clone>(self) -> T {
     |                          ^^^^^ required by this bound in `Dynamic::cast`

warning: unused variable: `ctx`
 --> src/main.rs:6:17
  |
6 |     pub fn test(ctx: NativeCallContext<'_>) -> u32 {
  |                 ^^^ help: if this is intentional, prefix it with an underscore: `_ctx`
  |
  = note: `#[warn(unused_variables)]` on by default

For more information about this error, try `rustc --explain E0277`.
warning: `rhai_reproducer` (bin "rhai_reproducer") generated 1 warning
error: could not compile `rhai_reproducer` (bin "rhai_reproducer") due to 1 previous error; 1 warning emitted
VorpalBlade commented 3 months ago

Interestingly the following fails with a different reason:

pub struct Bar {
    pub x: i64,
}

#[export_module]
pub(crate) mod dummy {
    pub type Foo = Bar;

    pub fn test(ctx: NativeCallContext<'_>, foo: &mut Foo) -> u32 {
        42
    }
}

This fails with:

error: function parameters other than the first one cannot be passed by reference
  --> src/main.rs:12:50
   |
12 |     pub fn test(ctx: NativeCallContext<'_>, foo: &mut Foo) -> u32 {
   |                                                  ^

According to the manual NativeCallContext should be the first parameter. But so should &mut T for any method. Seems there is a (documentation only?) conflict here.

schungx commented 3 months ago

Certainly it fails when I loaded your repo. However, I can't get it to fail on any of my own projects... which is extremely strange...

schungx commented 3 months ago

OK, I'm a total idiot. Of course my own projects work, because they point to my local repo.

I forgot to release the new version of rhai_codegen.

Which I'll do right away.

VorpalBlade commented 3 months ago

I hope a small tip isn't unwelcome: I don't know what your release pipeline looks like, but I have somewhat recently started using release-plz (not to be confused with release-please which also exists). It integrates generating changelog, cargo-semver-checks, and many other steps in automating the release process.

I also notice when checking https://lib.rs/~schungx/dash that there are a couple of minor issues listed for rhai, including that the most recent release doesn't match a commit in the repository. Using an automated workflow for releases (where everything is done in CI) helps prevent such issues.

schungx commented 3 months ago

I am not too familiar with these CI tools myself, and I understand that there are multiple ways of approaching this.

My own fear is just that the setup can be so complicated that I may not know what to do come release time... or if things go wrong.

Welcome a PR to set things up as you think would be helpful.

schungx commented 3 months ago

Closing this for now. Feel free to reopen if there are further issues.