Skip to content

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Sep 18, 2025

Following code:

#[derive(GodotClass)]
#[class(init, base = Node2D)]
struct BreaksClippyNursery {
    field: OnEditor<Gd<Node>>,
    base: Base<Node2D>
}

#[godot_api]
impl BreaksClippyNursery {}

#[godot_api]
impl INode2D for BreaksClippyNursery {
    fn draw(&mut self) {
    }
}

Was tripping clippy::nursery lints.

warning: `if _ { .. } else { .. }` is an expression
  --> src/lib.rs:13:10
   |
13 | #[derive(GodotClass)]
   |          ^^^^^^^^^^
   |
   = note: you might not need `mut` at all
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_let_if_seq
note: the lint level is defined here
  --> src/lib.rs:1:9
   |
1  | #![warn(clippy::useless_let_if_seq)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: this warning originates in the derive macro `GodotClass` (in Nightly builds, run with -Z macro-backtrace for more info)

This one happens when only one OnEditor field is present. Handled with #[allow(...)] (making three versions depending on amount of OnEditor fields would be silly).

warning: temporary with significant `Drop` can be early dropped
  --> src/lib.rs:27:1
   |
27 | #[godot_api]
   | ^^^^^^^^^^^^ temporary `guard` is currently being dropped at the end of its contained scope
   |
   = note: this might lead to unnecessary resource contention
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_tightening
note: the lint level is defined here
  --> src/lib.rs:2:9
   |
2  | #![warn(clippy::significant_drop_tightening)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: this warning originates in the macro `$crate::plugin_add_inner` which comes from the expansion of the attribute macro `godot_api` (in Nightly builds, run with -Z macro-backtrace for more info)

Triggered by plugin registry, let mut guard = ...; guard.do_something(...); handled with #[allow(...)] (the lint is silly and I would like to keep it somewhat readable, thank you).

@Yarwin Yarwin added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Sep 18, 2025
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1317

@Bromeon
Copy link
Member

Bromeon commented Sep 18, 2025

Thanks! Maybe add brief comment about what they solve, or link to this PR. Otherwise looks good to me 🙂

@Yarwin Yarwin force-pushed the address-clippy-lints-in-macros branch from f1e63b2 to 4c0d2e7 Compare September 18, 2025 07:31
@Yarwin
Copy link
Contributor Author

Yarwin commented Sep 18, 2025

Thanks! Maybe add brief comment about what they solve, or link to this PR. Otherwise looks good to me 🙂

I think it won't hurt, albeit stakes are small (keeping dangling #[allow(...)] and git blame (combined with explicitly clippy lint) already has all the info 🤷.

Comment on lines 35 to 36
// Triggered by $body.
#[allow(clippy::significant_drop_tightening)]
Copy link
Member

Choose a reason for hiding this comment

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

The lint docs say:

Searches for elements marked with #[clippy::has_significant_drop] that could be early dropped but are in fact dropped at the end of their scopes. In other words, enforces the “tightening” of their possible lifetimes.

And you mentioned this is caused by the guard. But are you sure we cannot apply this to the code that actually causes this?

let mut guard = $registry.lock().unwrap();
guard.push($plugin);

If not, then // Triggered by $body. should at least be more descriptive. This isn't a common lint, and it has a very non-self-explanatory name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what, at first I thought that writing $registry.lock().unwrap().push($plugin); is not a good call, but now (after checking macro expansion via RustRover) I think it would be valid thing to do – we already do it the same way while registering constants & methods 🤔.

I also merged plugin_add and plugin_add_inner – after changes there is no need for separate inner.

let warning_message =
format! { "godot-rust: OnEditor field {field} hasn't been initialized."};

// Note – triggers `clippy::useless_let_if_seq` lint if only one OnEditor field is present.
Copy link
Member

Choose a reason for hiding this comment

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

Comment should be next to the #[allow].

@Yarwin Yarwin force-pushed the address-clippy-lints-in-macros branch from 4c0d2e7 to 9298387 Compare September 19, 2025 06:50
@Bromeon
Copy link
Member

Bromeon commented Sep 19, 2025

Thanks! 👍

I was first worried because it looked like it would lock more, but it already does lock for every addition, so no changes there. If it ever becomes a performance bottleneck, we can always think about alternatives, but I like the simplicity here 🙂

@Bromeon Bromeon added this pull request to the merge queue Sep 19, 2025
Merged via the queue into godot-rust:master with commit b2243fb Sep 19, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: register Register classes, functions and other symbols to GDScript quality-of-life No new functionality, but improves ergonomics/internals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants