Skip to content
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

Restrictive AssetPath From impls cause hard-to-diagnose compile errors #10478

Closed
SludgePhD opened this issue Nov 9, 2023 · 2 comments · Fixed by #10823
Closed

Restrictive AssetPath From impls cause hard-to-diagnose compile errors #10478

SludgePhD opened this issue Nov 9, 2023 · 2 comments · Fixed by #10823
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior

Comments

@SludgePhD
Copy link
Contributor

SludgePhD commented Nov 9, 2023

Bevy version

0.12 or git

What you did

Use AssetServer methods that take impl Into<AssetPath<'a>> as parameters.

What went wrong

error[E0597]: `fileset` does not live long enough
   --> crates/artifice_editor/src/files.rs:120:30
    |
96  |         .show(egui_context, |ui| {
    |                             ---- value captured here
...
120 |                 for path in &fileset.files {
    |                              ^^^^^^^ borrowed value does not live long enough
...
133 |                         let handle = server.load_untyped(relpath);
    |                                      ---------------------------- argument requires that `fileset` is borrowed for `'static`
...
145 | }
    |  - `fileset` dropped here while still borrowed

error[E0521]: borrowed data escapes outside of function
   --> crates/artifice_editor/src/files.rs:133:38
    |
55  |     mut fileset: Local<FileSet>,
    |     -----------
    |     |
    |     `fileset` is a reference that is only valid in the function body
    |     has type `bevy::prelude::Local<'1, FileSet>`
...
133 |                         let handle = server.load_untyped(relpath);
    |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                      |
    |                                      `fileset` escapes the function body here
    |                                      argument requires that `'1` must outlive `'static`

This diagnostic is difficult to decipher: the impl Into<AssetPath<'a>> argument makes it look like the method wouldn't have any lifetime constraints, and the diagnostics don't refer to the actual reason those constraints exist (so arguably this is a rustc bug, but bevy might want to fix it too, since what it does is somewhat odd here).

The real cause of these errors is that AssetPath has several From impls that are restricted to 'static lifetimes:

  • impl From<&'static Path> for AssetPath<'static>
  • impl From<&'static str> for AssetPath<'static>

Those are chosen by rustc and are what cause the argument to be forced to &'static Path, instead of &'a Path.

It is possible to fix this by making the From impls fully generic, and by moving the "static lifetime optimization" to dedicated methods (right now the AssetPath API works exactly the other way around).

@SludgePhD SludgePhD added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 9, 2023
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Assets Load files from disk to use for things like images, models, and sounds and removed S-Needs-Triage This issue needs to be labelled labels Nov 9, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Nov 9, 2023
@pop
Copy link

pop commented Dec 9, 2023

Thank you for the explanation! Is there any workaround? This feels like a breaking change...

I hit this error when trying to upgrade my project from Bevy 0.11 to 0.12.1 using Rust Nightly (1.76.0-nightly (978722961 2023-12-06)) on Linux and Windows.

EDIT:
I have reproduced the issue in a (I think) minimal example (collapsed because the code snippets are very long):

Compiles in Bevy 0.11

src/main.rs

use bevy::{
    asset::{AssetLoader, LoadContext, LoadedAsset},
    gltf::Gltf,
    prelude::*,
    reflect::{TypePath, TypeUuid},
    utils::BoxedFuture,
};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_asset::<Foo>()
        .init_asset_loader::<FooLoader>()
        .add_systems(Startup, startup)
        .add_systems(Update, thing.run_if(run_once()))
        .run();
}

fn startup(mut commands: Commands, server: Res<AssetServer>) {
    let handle: Handle<Foo> = server.load("foo.txt");
    commands.insert_resource(FooHandle(handle));
}

fn thing(server: Res<AssetServer>, foos: Res<Assets<Foo>>) {
    let handle = server.load("foo.txt");
    let foo = foos.get(&handle).unwrap();
    let _: Handle<Gltf> = server.load(foo.0.as_str());
}

#[derive(TypePath, TypeUuid)]
#[uuid = "e2f7d9ab-c484-423b-991c-14e0450d2708"]
struct Foo(String);

#[derive(Resource)]
struct FooHandle(Handle<Foo>);

#[derive(Default)]
struct FooLoader;

impl AssetLoader for FooLoader {
    fn load<'a>(
        &'a self,
        bytes: &'a [u8],
        load_context: &'a mut LoadContext,
    ) -> BoxedFuture<'a, Result<(), bevy::asset::Error>> {
        Box::pin(async move {
            let result = String::from_utf8(bytes.to_vec()).unwrap_or(String::from("uhoh!"));
            load_context.set_default_asset(LoadedAsset::new(Foo(result)));
            Ok(())
        })
    }

    fn extensions(&self) -> &[&str] {
        &["txt"]
    }
}

assets/foo.txt:

path/to/file.gltf

Note: foo.txt stores the path to another asset which we load/reload at runtime for development. This feel contrived when I write it out and there may be a better way to achieve the goal of dynamic asset loading.

Diff
diff --git a/src/main.rs b/src/main.rs
index c035ae4..43556c1 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,16 +1,16 @@
 use bevy::{
-    asset::{AssetLoader, LoadContext, LoadedAsset},
+    asset::{AssetLoader, AsyncReadExt, LoadContext, io::Reader},
     gltf::Gltf,
     prelude::*,
-    reflect::{TypePath, TypeUuid},
+    reflect::TypePath,
     utils::BoxedFuture,
 };

 fn main() {
     App::new()
         .add_plugins(DefaultPlugins)
-        .add_asset::<Foo>()
-        .init_asset_loader::<FooLoader>()
+        .init_asset::<Foo>()
+        .register_asset_loader(FooLoader)
         .add_systems(Startup, startup)
         .add_systems(Update, thing.run_if(run_once()))
         .run();
@@ -23,12 +23,11 @@ fn startup(mut commands: Commands, server: Res<AssetServer>) {

 fn thing(server: Res<AssetServer>, foos: Res<Assets<Foo>>) {
     let handle = server.load("foo.txt");
-    let foo = foos.get(&handle).unwrap();
+    let foo = foos.get(handle).unwrap();
     let _: Handle<Gltf> = server.load(foo.0.as_str());
 }

-#[derive(TypePath, TypeUuid)]
-#[uuid = "e2f7d9ab-c484-423b-991c-14e0450d2708"]
+#[derive(Asset, TypePath)]
 struct Foo(String);

 #[derive(Resource)]
@@ -38,19 +37,25 @@ struct FooHandle(Handle<Foo>);
 struct FooLoader;

 impl AssetLoader for FooLoader {
+    type Asset = Foo;
+    type Settings = ();
+    type Error = std::io::Error;
+
     fn load<'a>(
         &'a self,
-        bytes: &'a [u8],
-        load_context: &'a mut LoadContext,
-    ) -> BoxedFuture<'a, Result<(), bevy::asset::Error>> {
+        reader: &'a mut Reader,
+        _settings: &'a Self::Settings,
+        _load_context: &'a mut LoadContext,
+    ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
         Box::pin(async move {
-            let result = String::from_utf8(bytes.to_vec()).unwrap_or(String::from("uhoh!"));
-            load_context.set_default_asset(LoadedAsset::new(Foo(result)));
-            Ok(())
+            let mut bytes = Vec::new();
+            reader.read_to_end(&mut bytes).await?;
+            let result = String::from_utf8(bytes).unwrap_or(String::from("uhoh!"));
+            Ok(Foo(result))
         })
     }

     fn extensions(&self) -> &[&str] {
         &["txt"]
     }
-}
+}
Fails to compile in Bevy 0.12.1
use bevy::{
    asset::{AssetLoader, AsyncReadExt, LoadContext, io::Reader},
    gltf::Gltf,
    prelude::*,
    reflect::TypePath,
    utils::BoxedFuture,
};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .init_asset::<Foo>()
        .register_asset_loader(FooLoader)
        .add_systems(Startup, startup)
        .add_systems(Update, thing.run_if(run_once()))
        .run();
}

fn startup(mut commands: Commands, server: Res<AssetServer>) {
    let handle: Handle<Foo> = server.load("foo.txt");
    commands.insert_resource(FooHandle(handle));
}

fn thing(server: Res<AssetServer>, foos: Res<Assets<Foo>>) {
    let handle = server.load("foo.txt");
    let foo = foos.get(handle).unwrap();
    let _: Handle<Gltf> = server.load(foo.0.as_str());
}

#[derive(Asset, TypePath)]
struct Foo(String);

#[derive(Resource)]
struct FooHandle(Handle<Foo>);

#[derive(Default)]
struct FooLoader;

impl AssetLoader for FooLoader {
    type Asset = Foo;
    type Settings = ();
    type Error = std::io::Error;

    fn load<'a>(
        &'a self,
        reader: &'a mut Reader,
        _settings: &'a Self::Settings,
        _load_context: &'a mut LoadContext,
    ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
        Box::pin(async move {
            let mut bytes = Vec::new();
            reader.read_to_end(&mut bytes).await?;
            let result = String::from_utf8(bytes).unwrap_or(String::from("uhoh!"));
            Ok(Foo(result))
        })
    }

    fn extensions(&self) -> &[&str] {
        &["txt"]
    }
}
Which fails to compile with the following errors:
   Compiling assets-repro-0-12 v0.1.0 (D:\Projects\src\localhost\assets-repro-0-12)
error[E0597]: `foos` does not live long enough
  --> src\main.rs:26:15
   |
24 | fn thing(server: Res<AssetServer>, foos: Res<Assets<Foo>>) {
   |                                    ---- binding `foos` declared here
25 |     let handle = server.load("foo.txt");
26 |     let foo = foos.get(handle).unwrap();
   |               ^^^^------------
   |               |
   |               borrowed value does not live long enough
   |               argument requires that `foos` is borrowed for `'static`
27 |     let _: Handle<Gltf> = server.load(foo.0.as_str());
28 | }
   |  - `foos` dropped here while still borrowed

error[E0521]: borrowed data escapes outside of function
  --> src\main.rs:26:15
   |
24 | fn thing(server: Res<AssetServer>, foos: Res<Assets<Foo>>) {
   |                                    ----
   |                                    |
   |                                    `foos` is a reference that is only valid in the function body
   |                                    has type `bevy::prelude::Res<'1, bevy::prelude::Assets<Foo>>`
25 |     let handle = server.load("foo.txt");
26 |     let foo = foos.get(handle).unwrap();
   |               ^^^^^^^^^^^^^^^^
   |               |
   |               `foos` escapes the function body here
   |               argument requires that `'1` must outlive `'static`

Some errors have detailed explanations: E0521, E0597.
For more information about an error, try `rustc --explain E0521`.
error: could not compile `assets-repro-0-12` (bin "assets-repro-0-12") due to 2 previous errors

I would love some guidance on this. It is not a common pattern in my project but this is a hard blocker on upgrading to Bevy 0.12 and I cannot for the life of me figure out a workaround.

Another Edit: In my particular case this was solved by using asset dependencies. Instead of storing the asset path on Foo struct I store the Handle<T> and reference the handle directly.

For example...
#[derive(Asset, TypePath)]
struct Foo(Handle<Gltf>);
// ...
    fn load<'a>(
        &'a self,
        reader: &'a mut Reader,
        _settings: &'a Self::Settings,
        load_context: &'a mut LoadContext,
    ) -> BoxedFuture<'a, Result<Self::Asset, Self::Error>> {
        Box::pin(async move {
            let mut bytes = Vec::new();
            reader.read_to_end(&mut bytes).await?;
            let asset_path = String::from_utf8(bytes)?;
            let handle = load_context.load(asset_path);
            Ok(Foo(handle))
        })
    }

@johnbchron
Copy link
Contributor

The backing implementation here is a CowArc, which is built to deduplicate strings as much as possible. If you pass in a borrowed type, i.e. a &str, the CowArc will intentionally not clone the value.

The lifecycle of a handle is determined at runtime, so references that a handle owns need to be 'static, including its asset path. This is why the .load() function needs an owned value or a 'static &str in most cases.

I cannot for the life of me figure out a workaround.

Turn the &str into a String with String::to_owned(), and use that in .load().

The reason we take impl Into<AssetPath<'a>> instead of String or the like is because the CowArc implementation is highly optimized for working with string literals, which are 'static &str.

@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Feb 2, 2024
@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Feb 2, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 19, 2024
…ns over Lifetime (#10823)

# Objective

- Fixes #10478

## Solution

Generalised `From/Into` implementations over `&str` and `Option<&str>`
for `AssetSourceId` and `AssetPath` across all lifetimes, not just
static. To maintain access to the `'static`-only specialisation, these
types (and `CowArc`) now include an `as_static` method which will apply
the specialisation.

```rust
// Snipped from `AssetApp`
fn register_asset_source(
    &mut self,
    id: impl Into<AssetSourceId<'static>>,
    //                          ^^^^^^^
    //                          | as_static is only available for 'static lifetimes
    source: AssetSourceBuilder,
) -> &mut Self {
    let id = id.into().as_static();
    //          ^^^^^^ ^^^^^^^^^
    //          |      | Specialized (internally storing CowArc::Static)
    //          | Generic Into (internally storing CowArc::Borrowed)
    
    // ...
}
```

This post-fix specialisation is available here because the actual
specialisation performed is only a marker for if/when modification or
ownership is required, making the transform a very cheap operation. For
cleanliness, I've also added `from_static`, which wraps this behaviour
in a clean shorthand designed to replace `from` calls.

---

## Changelog

- Generalised the following implementations over a generic lifetime:
  - `From<&'static str> for AssetSourceId<'static>`
  - `From<Option<&'static str>> for AssetSourceId<'static>`
  - `From<&'static str> for AssetPath<'static>`
  - `From<&'static Path> for AssetPath<'static>`
- Added `as_static` specialisation to:
  - `CowArc`
  - `AssetSourceId`
  - `AssetPath`
- Added `from_static` specialised constructor to:
  - `AssetSourceId`
  - `AssetPath`

## Migration Guide

In areas where these implementations where being used, you can now add
`from_static` in order to get the original specialised implementation
which avoids creating an `Arc` internally.

```rust
// Before
let asset_path = AssetPath::from("my/path/to/an/asset.ext");

// After
let asset_path = AssetPath::from_static("my/path/to/an/asset.ext");
```

To be clear, this is only required if you wish to maintain the
performance benefit that came with the specialisation. Existing code is
_not_ broken by this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants