This only warns on the code App::new();. If you add plugins or systems, then the warning won't be displayed even if you don't call run().

adding must_use on most functions in App doesn't work either:

The following code snippet will complain but shouldn't

let mut app = App::new();
app.add_plugins(DefaultPlugins);
app.run();

As @mockersf just mentioned, the #[must_use] is messing up a lot of Bevy code.
What if I instead add a custom Drop implementation that throws a warning if the App is dropped without being run?

Hmm. A custom drop impl would be interesting. I'm curious to see what that looks like.

Clever! Can you add a test to verify that this keeps working?

Clever! Can you add a test to verify that this keeps working?

Does "this keeps working" mean that the message is printed, or does it mean that it doesn't panic?

That the message is printed in the cases where it should be, and not when the app is run or updated.

That the message is printed in the cases where it should be, and not when the app is run or updated.

Thanks!

Afraid I don't have anything more useful to add, but...wow, you all are awesome. Didn't expect filing my first Bevy issue would get a PR with multiple (clever!) iterations in about half a day. You rock, Bevy rocks, and Rust rocks.

I found two issues:

1. On my end, the warn! code runs but doesn't print anything on screen. Maybe the logging plugin must be configured somehow.
2. It emits false positives for apps with a single iteration, like the hello_world example.

On my end, the warn! code runs but doesn't print anything on screen. Maybe the logging plugin must be configured somehow.

That might be troublesome. I'm pretty sure the Bevy log plugin has to be added or you won't get the warning. Maybe this isn't a big deal because most people will have added the DefaultPlugins by the time they forget to run the app.

@Nilirad did you add any plugins in your test where you didn't get the warning?

Maybe a println! could do the job? But it's inconsistent with the rest of the codebase. Also I'm not confortable with panicking.

@Nilirad did you add any plugins in your test where you didn't get the warning?

No, I just stripped the .run() from the hello_world example.

Maybe we can check if there's already a tracing subscriber, and if there isn't, we println, but if there is, we log.

Maybe we can check if there's already a tracing subscriber, and if there isn't, we println, but if there is, we log.

Great idea!

The error message does appear in the hello world example, but it comes even when the .run() is included, for some reason.

Is it possible to remove the flag for the release profile? It's essentially a debug check and not a soundness one.

Is it possible to remove the flag for the release profile? It's essentially a debug check and not a soundness one.

Also a great idea!

I would rather not have a check for logger, and a fallback to println. It adds a lot of complexity to something that is a nice to have, and most people should have a logger anyway

I would rather not have a check for logger, and a fallback to println. It adds a lot of complexity to something that is a nice to have, and most people should have a logger anyway

It only does that if the tracing feature is enabled for the crate. I just added some comments to make it easier to understand. Hopefully that helps!

It only does that if the tracing feature is enabled for the crate.

Then it's wrong... logging doesn't depend on the trace feature

Then it's wrong... logging doesn't depend on the trace feature

LogPlugin uses tracing under the hood. Disabling the trace feature would imply that you don't want to use tracing. All of the existing tracing-based functionality in bevy_app uses the same #[cfg(feature = "trace")]

The trace feature is not enabled by default, and is used to control all the additional spans that are added to be able to trace executions.

Logs are using the tracing crate even without the trace feature

@mockersf Thanks for clarifying it for me!

I don't like that it's behind debug_assertions

Do you really want to ship 8 bytes of bloat to production? 🙃

I don't like that it's behind debug_assertions

Do you really want to ship 8 bytes of bloat to production? 🙃

debug_assertions detects optimised/not optimised, it has nothing to do with production. If the user is following pretty much any of the Bevy guide around here, either they will run in release or with dependency optimised and the warning will be disabled

either they will run in release or with dependency optimised and the warning will be disabled

That's a good point. If we want users to actually see the message, we should have that enabled even for release mode.

Almost nobody can actually play games without optimizations I think.

Then it could be enabled in dev profile instead. So the check is included even if dependencies are optimized. But we're really talking about a very negligible amount of data/instructions.

EDIT: Apparently there is no way to detect the build profile in conditional compilation, so the check must be done even in every build.

EDIT 2: Actually, the Cargo book shows that debug assertions are always enabled by default in the dev profile, independently from optimization level. A developer should really not debug in release profile though, but just tweak the dev profile...

I got rid of the #[cfg(debug_assertions)] anyway. The performance penalty for an extra variable assignment is negligible, and rustc will likely optimize this code for release builds.

bors r+

bors r- (Waiting on reviews; this isn't urgent and it'll mess up the change log a bit).

Canceled.

I get no warning message in breakout.rs by just removing .run(). Using the debugger, I can see that the !self.has_run path is taken, but the rest of the code is behind a closure, so I cannot investigate more.

it just occurred to me that if this PR worked, the message would be logged twice as there are two apps created (and dropped): the main app and the renderer app.

With that, I don't think implementing drop for app is the correct solution

I got rid of the part that checked if there was a logger. It didn't work and it wasn't very popular to begin with.

it just occurred to me that if this PR worked, the message would be logged twice as there are two apps created (and dropped): the main app and the renderer app.

That does happen, but I don't think it's really a problem. It's mostly just a cosmetic issue.

This will always throw a warning as neither run or update is ever called on the rendering sub-app.

the proper way to fix this would to use the must_use flag, but that would mean changing how the app can be built. I think I would be in favour, but I would prefer to view it in action before being sure...