-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Don't panic when duplicate plugins are added #21114
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,12 +144,26 @@ mod sealed { | |
impl<P: Plugin> Plugins<PluginMarker> for P { | ||
#[track_caller] | ||
fn add_to_app(self, app: &mut App) { | ||
#[expect( | ||
clippy::allow_attributes, | ||
reason = "This lint only triggers sometimes, based on the features enabled." | ||
)] | ||
#[allow(unused_variables, reason = "plugin_name is only used with std enabled")] | ||
if let Err(AppError::DuplicatePlugin { plugin_name }) = | ||
app.add_boxed_plugin(Box::new(self)) | ||
{ | ||
panic!( | ||
"Error adding plugin {plugin_name}: : plugin was already added in application" | ||
) | ||
// We cannot use `warn!` here because this may occur before the Bevy logger is initialized. | ||
// eprintln! is used as a fallback here, although it's generally not recommended for libraries to use directly. | ||
// | ||
// However, both `dbg!` and `eprintln!` are only available with the `std` feature. | ||
// This is not a critical error, so we've chosen to simply ignore it in `no_std` environments. | ||
#[cfg(feature = "std")] | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think changing behavior based on the number of fields / size of the typee is confusing / hard to document / hard to understand. Additionally, nothing is stopping a ZST plugin from behaving differently at different times, or feeding on data that might change. I think we either need to embrace "unique plugins registered more than once ignore the configuration from the second plugin and warns" or "double-registering unique plugins aborts app execution". I do think the warning makes sense, as plugins shouldn't need to coordinate / anticipate what other plugins might also register. Of course, the true fix is deferred plugin init, which would embrace the "only one instance, with shared config" approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imo that's fine in this case, since it's not behavior you have to actively think about. It's just an instance of one less panic in a specialized case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Panic at startup is very harmless compared to some during runtime, it is not like you are gonna miss it during development, it is annoying if you have to go to a library maintainer to ask them not to add external plugins, I won't deny, but this is something that is best dealt with the required plugins logic than with what is being proposed here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why bother though? Asking the other plugin to not add a ZST plugin has exactly the same effect in practice as what is proposed here. Imo we have a sensible and safe way to automate ecosystem tedium upstream with a tiny PR. |
||
std::eprintln!( | ||
"Error adding plugin {plugin_name}: plugin was already added in application. | ||
A second copy of this plugin was ignored, and the settings of the first added copy will be used.", | ||
); | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -192,3 +206,43 @@ mod sealed { | |
S | ||
); | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
#[test] | ||
fn duplicate_plugins_does_not_panic() { | ||
use crate::{App, Plugin}; | ||
|
||
struct MyPlugin; | ||
impl Plugin for MyPlugin { | ||
fn build(&self, _app: &mut App) {} | ||
} | ||
|
||
let mut app = App::new(); | ||
app.add_plugins(MyPlugin); | ||
app.add_plugins(MyPlugin); // should not panic | ||
} | ||
|
||
#[test] | ||
fn duplicate_plugin_in_plugin_group_does_not_panic() { | ||
use crate::{App, Plugin, PluginGroup, PluginGroupBuilder}; | ||
|
||
struct MyPlugin; | ||
impl Plugin for MyPlugin { | ||
fn build(&self, _app: &mut App) {} | ||
} | ||
|
||
struct MyPluginGroup; | ||
impl PluginGroup for MyPluginGroup { | ||
fn build(self) -> PluginGroupBuilder { | ||
PluginGroupBuilder::start::<MyPluginGroup>() | ||
.add(MyPlugin) | ||
.add(MyPlugin) // duplicate, should not panic | ||
} | ||
} | ||
|
||
let mut app = App::new(); | ||
app.add_plugins(MyPluginGroup); // should not panic | ||
app.add_plugins(MyPlugin); // should not panic either | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than all of this "pomp and circumstance", can't we just name the bound field
_plugin_name
?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or add a
let _ = plugin_name;
to the end of the scope :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better, learned from @janis-bhm:
#[allow(clippy::allow_attributes, unused_variables, reason = "...")]