error on incorrect implied bounds in wfcheck except for Bevy dependents

[jackh726]

Rebase of #109763

Additionally, special cases Bevy ParamSet types to not trigger the lint. This is tracked in #119956.

Fixes #109628
Fixes #109799

[jackh726]

@bors try

[bors]

⌛ Trying commit b4485be with merge 1f34c1d...

[bors]

☀️ Try build successful - checks-actions
Build commit: 1f34c1d (1f34c1de16b990f2dfcb25ad6528e0fbc4c3092a)

[jackh726]

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-109482-1/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-118553 created and queued.
🤖 Automatically detected try build 1f34c1d
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-118553 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-118553 is completed!
📊 33 regressed and 0 fixed (428 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[jackh726]

@bors try

[bors]

⌛ Trying commit b4e3421 with merge a1ab930...

[bors]

☀️ Try build successful - checks-actions
Build commit: a1ab930 (a1ab9307c2d2e67595eb3bb22fd707ccd1f71e33)

[jackh726]

@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-109482-1/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-118553-1 created and queued.
🤖 Automatically detected try build a1ab930
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-118553-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-118553-1 is completed!
📊 0 regressed and 0 fixed (428 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[jackh726]

Great, now for everything.

@craterbot check

[craterbot]

👌 Experiment pr-118553-2 created and queued.
🤖 Automatically detected try build a1ab930
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[rust-timer]

Finished benchmarking commit (a34faab): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.2%, 1.6%]	30
Regressions ❌ (secondary)	0.4%	[0.2%, 0.6%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.2%, -0.8%]	6
All ❌✅ (primary)	0.7%	[0.2%, 1.6%]	30

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.9%	[0.5%, 7.2%]	3
Regressions ❌ (secondary)	0.8%	[0.8%, 0.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[0.5%, 7.2%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.0%, 1.3%]	2
Regressions ❌ (secondary)	2.1%	[2.1%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.0%, 1.3%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 665.735s -> 666.175s (0.07%)
Artifact size: 308.27 MiB -> 308.36 MiB (0.03%)

[alice-i-cecile]

FYI, MIRI runs just started failing for us after this was merged:

https://github.com/bevyengine/bevy/actions/runs/7578583138/job/20641401467

We'll turn it off as a mandatory CI check for now, and look at prioritizing a fix (but probably not backporting).

[jackh726]

Interesting, an MCVE would be good. I'll try to look into it.

[jackh726]

I unsuccessfully tried to reproduce the CI error:

pub trait WorldQuery {}
impl<T> WorldQuery for &T {}

impl<T> WorldQuery for & mut T {}

pub struct Query<Q: WorldQuery>(Q);

pub trait SystemParam {
   type State;
}
impl<Q: WorldQuery + 'static> SystemParam for Query<Q> {
   type State = ();
}


impl<P0, P1> SystemParam for (P0, P1)
where
   P0: SystemParam,
   P1: SystemParam,
{
   type State = (<P0 as SystemParam>::State, <P1 as SystemParam>::State);
}

pub struct ParamSet<T: SystemParam>(T) where T::State: Sized;

struct A;

fn sys(mut _set: ParamSet<(Query<&mut A>, Query<&A>)>) {}

fn main() {}

[jackh726]

I'm guessing it has to do with the rest of the test function.

reference for later:

run_system

impl IntoSystem<...> for F where F: SystemParamFunction<...>

impl SystemParamFunction<...> for Func

Probably need to pull out those impls into the test.

[lcnr]

pub trait WorldQuery {}
impl<T> WorldQuery for &T {}

impl<T> WorldQuery for & mut T {}

pub struct Query<Q: WorldQuery>(Q);

pub trait SystemParam {
   type State;
}
impl<Q: WorldQuery + 'static> SystemParam for Query<Q> {
   type State = ();
}


impl<P0, P1> SystemParam for (P0, P1)
where
   P0: SystemParam,
   P1: SystemParam,
{
   type State = (<P0 as SystemParam>::State, <P1 as SystemParam>::State);
}

pub struct ParamSet<T: SystemParam>(T) where T::State: Sized;

struct A;

fn sys(_set1: ParamSet<(Query<&mut A>, Query<&A>)>) {
    let _: ParamSet<_> = _set1;
}

fn main() {}

I think the error happens because MIR borrowck uses the new implied bounds computation instead of the old one. So while WF check passes, MIR borrowck does not.

[alice-i-cecile]

We're getting new users reporting that this error is surfacing in clean builds of Bevy on nightly. Swapping back to stable fixed it.

I can reproduce it both when building Bevy itself, and in an empty project with a dependency on Bevy 0.12.1, with:

rustc 1.77.0-nightly (25f8d01fd 2024-01-18)
binary: rustc
commit-hash: 25f8d01fd8bda339612d0c0a8844173a09205f7c
commit-date: 2024-01-18
host: x86_64-pc-windows-msvc
release: 1.77.0-nightly
LLVM version: 17.0.6

The error:

   Compiling bevy_transform v0.12.1
error: lifetime may not live long enough
  --> C:\Users\alice\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_transform-0.12.1\src\systems.rs:14:1
   |
14 | / pub fn sync_simple_transforms(
15 | |     mut query: ParamSet<(
16 | |         Query<
17 | |             (&Transform, &mut GlobalTransform),
   | |              - let's call the lifetime of this reference `'1`
...  |
26 | |     mut orphaned: RemovedComponents<Parent>,
27 | | ) {
   | |_^ requires that `'1` must outlive `'static`

error: lifetime may not live long enough
  --> C:\Users\alice\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_transform-0.12.1\src\systems.rs:14:1
   |
14 | / pub fn sync_simple_transforms(
15 | |     mut query: ParamSet<(
16 | |         Query<
17 | |             (&Transform, &mut GlobalTransform),
   | |                          - let's call the lifetime of this reference `'2`
...  |
26 | |     mut orphaned: RemovedComponents<Parent>,
27 | | ) {
   | |_^ requires that `'2` must outlive `'static`

error: lifetime may not live long enough
  --> C:\Users\alice\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_transform-0.12.1\src\systems.rs:14:1
   |
14 | / pub fn sync_simple_transforms(
15 | |     mut query: ParamSet<(
   | |     --------- has type `ParamSet<'_, '_, (Query<'_, '_, (&transform::Transform, &mut global_transform::GlobalTransform), (Or<(Changed<transform::Transform>, Added<global_transform::GlobalTransform>)>, Without<Parent>, Without<Children>)>, Query<'_, '_, (bevy_ecs::change_detection::Ref<'3, transform::Transform>, &mut global_transform::GlobalTransform), (Without<Parent>, Without<Children>)>)>`
16 | |         Query<
17 | |             (&Transform, &mut GlobalTransform),
...  |
26 | |     mut orphaned: RemovedComponents<Parent>,
27 | | ) {
   | |_^ requires that `'3` must outlive `'static`

error: lifetime may not live long enough
  --> C:\Users\alice\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bevy_transform-0.12.1\src\systems.rs:14:1
   |
14 | / pub fn sync_simple_transforms(
15 | |     mut query: ParamSet<(
16 | |         Query<
17 | |             (&Transform, &mut GlobalTransform),
...  |
24 | |         Query<(Ref<Transform>, &mut GlobalTransform), (Without<Parent>, Without<Children>)>,
   | |                                - let's call the lifetime of this reference `'4`
25 | |     )>,
26 | |     mut orphaned: RemovedComponents<Parent>,
27 | | ) {
   | |_^ requires that `'4` must outlive `'static`

error: could not compile `bevy_transform` (lib) due to 4 previous errors

[BoxyUwU]

I don't understand how this wasn't caught. did nobody try to compile bevy with this PR to make sure nothing broke?

[jackh726]

It was missed because the Bevy test that was previously added didn't adequately cover this. I made changes (to move MIR borrowck to non-compat) after the crater run, and that's what caused the regression.

This happens and is why we have nightly.

[jackh726]

The problem will be fixed after #120123 lands (which should be in the next few hours).

[nnethercote]

@jackh726, @lcnr: some non-trivial perf regressions occurred. Were they expected? Can they be mitigated?

[jackh726]

If I had to guess, between this and #120123, perf is a bit of a wash.

[Kobzol]

On diesel, this mostly just undone the wins from #120123:

The other regressions look genuine, but since this was a correctness fix, I'll go ahead and mark this as triaged.

@rustbot label: +perf-regression-triaged