Fix unsoundness in QueryIter::sort_by

[chescock]

Objective

QueryIter::sort_by() is unsound. It passes the lens items with the full 'w lifetime, and a malicious user could smuggle them out of the closure where they could alias with the query results.

Solution

Make the sort closures generic in the lifetime parameter of the lens item. This ensures the lens items cannot outlive the call to the closure.

Testing

Added a compile-fail test that demonstrates the unsound pattern.

Migration Guide

The sort family of methods on QueryIter unsoundly gave access L::Item<'w> with the full 'w lifetime. It has been shortened to L::Item<'w> so that items cannot escape the comparer. If you get lifetime errors using these methods, you will need to make the comparer generic in the new lifetime. Often this can be done by replacing named 'w with '_, or by replacing the use of a function item with a closure.

// Before: Now fails with "error: implementation of `FnMut` is not general enough"
query.iter().sort_by::<&C>(Ord::cmp);
// After: Wrap in a closure
query.iter().sort_by::<&C>(|l, r| Ord::cmp(l, r));

query.iter().sort_by::<&C>(comparer);
// Before: Uses specific `'w` lifetime from some outer scope
// now fails with "error: implementation of `FnMut` is not general enough"
fn comparer(left: &&'w C, right: &&'w C) -> Ordering { /* ... */ }
// After: Accepts any lifetime using inferred lifetime parameter
fn comparer(left: &&C, right: &&C) -> Ordering { /* ... */ }

[alice-i-cecile]

Core idea is good, but CI is pointing out a few related problems I think.

[chescock]

A little bit of collateral damage here: sort_by_key can no longer borrow from the lens item. It's theoretically possible to support that, but it's hard to express with Fn traits.

It's still possible to express those sorts using sort_by, so that doesn't make anything impossible, just inconvenient.

[Victoronz]

I think this is likely just the original Rust limitation of sort_by_key/sort_by_cached_key, the closure can only return Copy or unusually long-lived data.

[Victoronz]

Hmm, was this a problem before? I see an iter_many_inner in sort_impl, this should just be iter_many. This was introduced by #15858.
Are these changes still necessary when changing this iter_many to a non-consuming iteration?

[chescock]

Hmm, was this a problem before? I see an iter_many_inner in sort_impl, this should just be iter_many. This was introduced by #15858.
Are these changes still necessary when changing this iter_many to a non-consuming iteration?

Yup, it reproduces before #15858. That PR only changed the implementation of the sort methods, and the compile-fail test here only relies on the signature.

[Victoronz]

How does this fail if just the function pointer is passed?
Unlike with sort_by_key, slice::sort_by is able to accept these without the need to wrap it in a closure.

[chescock]

It failed CI with error: implementation of `FnMut` is not general enough:
https://github.com/bevyengine/bevy/actions/runs/13291587519/job/37113490763#step:6:119

I admit I didn't investigate it too carefully; the rules for when rust infers higher-ranked lifetimes versus specific ones seem very finnicky to me.

[hymm]

found a description of the problem here https://users.rust-lang.org/t/problem-with-lifetimes-in-closure-return-types/79026

TLDR: the lifetime of Ord::cmp has a specific Item<'a> lifetime, while sort_by expects 'a to be hrtb. Doesn't seem fixable without tradeoffs.

[chescock]

Yeah, I bet it'll infer HRTB over lifetime parameters to the function but not to lifetime parameters to the trait or type. That's reasonable, even if I wish it were looser. I remember hitting some of the odd cases mentioned in https://rust-lang.github.io/rfcs/3216-closure-lifetime-binder.html, though, which left me with the impression that higher-ranked closure inference was always a little arbitrary.

[Victoronz]

While the ergonomics regressions hurt a bit, soundness is soundness.

[Victoronz]

Also, this should probably have a migration guide.

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.