bevyengine / bevyengine/bevy

Lifetime bounds differ between `WorldQuery` and `SystemParam`, making `rustc` grumpy

Open
#9,808 2 comments 4 reactions 1 assignee Claimed by @james7132 View on GitHub
A-ECS C-Bug D-Complex P-High
Dominant language
Rust
Stars
48.2k
Forks
4.8k
Avg merge
3d 22h
Merged PRs (30d)
161

Description

# The problem

Currently, [`SystemParam`](https://dev-docs.bevyengine.org/bevy/ecs/system/trait.SystemParam.html) is only implemented for `Query` types that are `'static`: these types [cannot contain any temporary references](https://doc.rust-lang.org/rust-by-example/scope/lifetime/static_lifetime.html#trait-bound).

The [key](https://dev-docs.bevyengine.org/bevy/ecs/prelude/struct.Query.html#impl-SystemParam-for-Query%3C'_,+'_,+Q,+F%3E) [code](https://github.com/bevyengine/bevy/blob/8192ac6f1e572601b4ada08fa2f82114e07f8aec/crates/bevy_ecs/src/system/system_param.rs#L162) is:
```rust
impl SystemParam for Query<'_, '_, Q, F>{
// Actual code
}
```

By contrast, no equivalent bound exists on [`WorldQuery`](https://dev-docs.bevyengine.org/bevy/ecs/query/trait.WorldQuery.html).

The net effect of this is that:

```rust
fn foo<'a>() {
let _: Query<&'a Transform>;
}
```

compiles, since `&'a Transform` implements `WorldQuery`, no matter what the actual lifetime `'a` is.

However, this is not a valid `SystemParam`, *unless* that lifetime is actually static (aka `'a: 'static`).

# Why this matters

So, we've seen users run into pain here within Bevy: see #7447 and #8192. Avoiding this footgun would be inherently good!

But the much larger problem comes when Rust [wants to improve how implied bounds](https://github.com/rust-lang/rust/pull/109763) are computed. This, in a truly unprecedented fashion, [breaks Bevy](https://github.com/rust-lang/rust/pull/109482#issuecomment-1484857537) and effectively nothing else, because of the extremely normal things we do to the type system.

To explain their proposed changes:

1. When writing Rust programs, you don't always have to explicitly write out the exact lifetimes that are needed.
2. Instead, the compiler can sometimes infer what lifetimes *must* exist in order for your program to function: this is called "implied bounds".
3. However, the current approach to doing this is pretty ad-hoc, and underspecified.
4. In particular, the existing design uses trait solver flavored logic to do this in some cases (which Bevy hits) by examining the trait impls used.
5. This is both sketchy, and is in conflict with some more sensible implied bounds work that is currently missing.
6. So they want to change how it works!

The consequence of that change (if both Bevy and rustc hold their courses) is that Bevy users will get a very confusing, un-silenceable and unactionable lint (at times). In the future, this would be on the path to become a true compiler error.

What times? Well, @BoxyUwU did some digging, and discovered that the lint fires in exactly two places in our code base: both on our two internal uses of `ParamSet`. Experimenting more, this triggers on *any* use of `ParamSet` that involves queries with a `Q` or `F` type that have lifetimes that are not known to be static. In Bevy, that means `&T` and &mut T` query types.

The reason for this gets back to the problem at the top of this issue. Somewhere, we're currently relying on these implied bounds to effectively transfer those `'static` lifetime requirements down into the `Query`, via the power of trait magic. So when `rustc` stops relying on trait information for implied bounds, generic types that combine `WorldQuery` and `SystemParam` with unconstrained lifetimes fall afoul of the new rules.

# How can we fix this?

As @BoxyUwU and I see it, there are two fundamental approaches by which we could fix this.

1. Make `WorldQuery` more '`static`, by adding bounds everywhere.
2. Make `WorldQuery`'s `SystemParam` impl not require 'static

Either way, the discrepancy disappears, and we stop relying on implied bounds from trait impls to get the two parts to play nice.

Approach 1 is likely to improve end user ergonomics when working with custom `SystemParam`. There's a small chance that it regresses ergonomics in end user code (very bad!), by for example requiring users to write out `&'static Transform` in their queries. It also feels "more correct": using non static lifetimes in `WorldQuery` types doesn't seem to ever be correct: we're just type-punning with references.

This also may not work without [help from `rustc`](https://discord.com/channels/691052431525675048/749335865876021248/1151894107057295441): `trait WorldQuery: 'static` in combination with `Query` *should* imply that `Q` and `F` are always `'static`, but it's not clear that it currently does.

Approach 2 would be nicely targeted, and bring the impl into line with other implementations of `SystemParam` and how we implement `WorldQuery` (none of which requires `'static`). However, it may not work, or require complex unsafe code to get working.

# How do I test if my fix worked?

You will need to:
1. Create a branch of Bevy with your proposed fix.
2. Get the correct version of `rustc`, with the [proposed PR](https://github.com/rust-lang/rust/pull/109763) included.
3. Set up your rustup toolchain so then it links to your local rustc build, following the contributing guide below.
4. Build your branch with `cargo +stage1 build`.

To get the correct version of `rustc`, follow [their contributing guide](https://rustc-dev-guide.rust-lang.org/index.html). Alternatively, you may be able to pull in a cached version more easily using[rustup-toolchain-install-master] (https://github.com/kennytm/rustup-toolchain-install-master).

@lcnr warns me that this PR is somewhat stale (from May 2023), and should probably be rebased. If that happens, you'll want to test with the rebased version instead to get more accurate results.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.