rhaiscript / rhai

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

`NativeCallContext` in `export_module` results in a "hidden lifetime parameters in types are deprecated" warning #864

Closed ltabis closed 3 weeks ago

ltabis commented 3 months ago

Passing a NativeCallContext in a function defined in a module exported via the export_module attribute generates the "hidden lifetime parameters in types are deprecated" warning.

To reproduce:

#[rhai::plugin::export_module]
pub mod my_module {
    #[rhai_fn(global)]
    pub fn my_func(ctx: NativeCallContext) {
       // ...
    }

Compiler diagnostic:

warning: hidden lifetime parameters in types are deprecated
   --> main.rs:xx:xx
    |
xxx |         ctx: NativeCallContext,
    |              ^^^^^^^^^^^^^^^^^ expected lifetime parameter
    |
    = note: requested on the command line with `-W elided-lifetimes-in-paths`
help: indicate the anonymous lifetime
    |
xxx |         ctx: NativeCallContext<'_>,
    |                               ++++

I guess that is because the macro does expect that NativeCallContext is declared as is, could it be possible to have a variant with the elided lifetime ?

schungx commented 3 months ago

Not sure about this because NativeCallContext has multiple lifetime parameters...

This is the first time I see this warning so I'll have to check it out...

schungx commented 3 months ago

Seems like it is a flag rust_2028_idioms that turns on this lint. With it, you much explicitly specify the lifetime parameter.

I personally don't find it useful but that seems to be the case if that flag is turned on.

ltabis commented 3 months ago

Seems like it is a flag rust_2018_idioms ...

Yes, sorry for not pointing that out. What's annoying is that it needs to be allowed at crate level, but if you do not want to bother with it I can simply allow the warning.

Should this be kept open ? Thank you for your time.

schungx commented 3 months ago

I personally prefer not to write up unnecessary lifetime parameters for code clarity. However it nows seems to go against recommendations....

Lets keep this open. I'll see later to make sure all generated code are good. Then user code can follow their own style.

Maybe the docs should be updated to have all of those <'_> in?

ltabis commented 3 months ago

Maybe the docs should be updated to have all of those <'_> in?

I'm not sure it's necessary ... when users write the type the compiler should warn them (if they have the lint enabled) so I don't think it is needed to update the docs everywhere. My only problem was with the export_plugin proc macro since if you write the elided lifetime the macro do not recognize the syntax. As I said in my first comment I guess a quick fix would be to have a variant with the elided lifetime that the macro can recognize. (along the one without the lifetime of course, so that users that do not use the lint are not impacted)

schungx commented 3 months ago

Always put lifetimes in for generated code is ok. That's the best anyway.

Not like anybody will read them...

schungx commented 3 months ago

OK, I find out that codegen will fail to parse NativeCallContext<'_> because of the lifetime parameter. Therefore I consider this a bug.. It is a simple fix and will go into the next release.

Thanks!

schungx commented 3 weeks ago

New version of codegen released.

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