Returning Err() with a custom error type and a message gives error E0716

So, I'm having a hell of a time trying to get this audio decoding module to work.
My audio decoder has various functions for decoding various audio formats (right now its Ogg/Vorbis, CAF/ALAC and FLAC). I'm trying to make each function return a Result<>. My custom error type looks like this:

type Result<T> = std::result::Result<T, AudioError>;

#[derive(Debug, Clone, Copy)]
pub struct AudioError(&'static str);

impl fmt::Display for AudioError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{}", self.0)?;
        Ok(())
    }
}

impl error::Error for AudioError {
    fn source(&self) -> Option<&(dyn error::Error + 'static)> {
        None
    }
}

I'm trying to do something like the following:

fn decode_flac(filename: &'static str) -> Result<Arc<StaticSource>> {
    use claxon::FlacReader;
    let mut reader = match FlacReader::open(filename) {
        Ok(r) => r,
        Err(e) => {
            return Err(AudioError(
                format!("Could not open {}: {}", filename, e).as_str(),
            ))
        }
    };
    let bps = reader.streaminfo().bits_per_sample as f32;
    let mut samples: Vec<f32> = Vec::new();
    for sample in reader.samples() {
        match sample {
            Ok(s) => {
                // We need to dequantize the sample to get its 32-bit floating point equivalent
                let dequantized_sample = {
                    let range = bps.exp2();
                    ((2.0 * (s as f32) + range) / (range - 1.0)) - 1.0
                };
                samples.push(dequantized_sample);
            }
            Err(e) => {
                return Err(AudioError(
                    format!("Audio: flac: decoding error: {}: {}", filename, e,).as_str(),
                ))
            }
        }
    }
    let src = match create_source(
        samples,
        reader.streaminfo().sample_rate,
        reader.streaminfo().channels,
    ) {
        Ok(s) => s,
        Err(e) => {
            return Err(AudioError(
                format!("Cannot generate source: {}", e).as_str(),
            ))
        }
    };
    let src = Arc::new(src);
    match SRCS.write() {
        Ok(mut s) => s.push((filename, src.clone())),
        _ => panic!("Internal data storage for sources is poisoned!"),
    }
    Ok(src.clone())
}

However, the borrow checker is claiming that one of the variables is dropped (when I'm really returning it). This is probably because of how I'm declaring my storage mechanism. As a side note, I want to store an internal list of filenames and sources without having to either create new internal lists every time I need to do that or passing all of these around. So if someone could point me to a more idiomatic way of doing this, please do. This is how I'm creating the Vec<>:

use alto::*;
use lazy_static::*;
use std::convert::TryInto;
use std::error;
use std::fmt;
use std::fs::File;
use std::path::Path;
use std::sync::{Arc, RwLock};

lazy_static! {
    static ref SRCS: RwLock<Vec<(&'static str, Arc<StaticSource>)>> = RwLock::new(Vec::new());
    // ...
}

So:

  1. how would I fix this &'static lifetime problem when storing a Vec<> like this?
  2. Is there a more idiomatic way of storing an internal list like this without having to pass all sorts of copies around? Even if its "zero-cost," I would rather avoid it, especially since its not meant to be accessed publicly.

You can change

static ref SRCS: RwLock<Vec<(&'static str, Arc<StaticSource>)>> = RwLock::new(Vec::new());

to

static ref SRCS: RwLock<Vec<(String, Arc<StaticSource>)>> = RwLock::new(Vec::new());

Then you need to allocate whenever you create a new source, but that should be fine.


What does create_source look like, can we see more of the source code?


Passing around the list is probably the best way to do this. You could wrap up the list in a new type and use that to create the sources.

struct AudioSources { ... }

impl AudioSources {
    fn add_source(&self, file_name: &str) -> Result<Arc<StaticSource>> { ... }
}

Certainly.

fn create_source(samples: Vec<f32>, samplerate: u32, channels: u32) -> Result<StaticSource> {
    // There's a lot of code duplication here, but there's not much we can do about it right now.
    let buf = match channels {
        1 => match CTX.new_buffer::<Mono<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        2 => match CTX.new_buffer::<Stereo<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        4 => match CTX.new_buffer::<McQuad<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        5 => match CTX.new_buffer::<Mc51Chn<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        6 => match CTX.new_buffer::<Mc61Chn<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        7 => match CTX.new_buffer::<Mc71Chn<f32>, _>(samples, samplerate.try_into().unwrap()) {
            Ok(b) => b,
            Err(e) => return Err(AudioError(format!("{}", e).as_str())),
        },
        c => panic!("Attempted to create an unsupported channel layout of {}", c),
    };
    let buf = Arc::new(buf);
    let mut src = match CTX.new_static_source() {
        Ok(s) => s,
        Err(e) => return Err(AudioError(format!("{}", e).as_str())),
    };
    src.set_buffer(buf).unwrap();
    Ok(src)
}

If there's some way to avoid code duplication (I don't think so) and the comment in the code is wrong, please let me know. Even though I've worked with Rust for probably a year now I'm still learning it. :slight_smile:

Rule of thumb: &'static str means string literal. you can theorically make it from Box::leak() but it's not recommended at all, as it leaks the memory.

&str is a reference to memory buffer that contains UTF-8 encoded text. String literals are embeded directly in the executable binary, and String ownes its growable heap buffer like Vec<u8>. And of course, the buffer String contains will be deallocated when the String goes out of scope.

1 Like

I know, but I can't remove &'static and just make it &str because the compiler demands a lifetime and that just makes everything really messy (I think it would anyway, even if I changed &'static str to String). Right?

You can add

use std::string::ToString;

struct AudioError(String);

impl<T: ToString> From<T> for AudioError {
    fn from(err: T) {
        AudioError(e.to_string())
    }
}

Then you can use the ? operator like so.

I would recommend that you switch your error type to a enum that contains all the types of ways you can error, but you can do that once your finalizing your api. This makes it easier to deal with errors because you don't have to parse strings to figure out where an error came from.

As @Hyeonu said, &str is for temporaries or literals, more generally &_ is for temporaries or literals. So you should use String instead of &str if you need it to persist. You can always convert a &str to a String using to_string or to_owned. String has no lifetimes attached to it, so it can live as long as you need it to.

Yeah, I'll definitely change the errors to an enumeration sometime.
As for changing it to a struct, passing it around all over the place gets on my nerves purely because that's another thing that I need to keep track of. (I'm trying to implement this in a game that I've just started as an experiment, for context.) I would therefore need to keep track of a ton of references (perhaps even some mutable ones) in every structure I make, which just seems like an unnecessary amount of copying and cloning. It just seems alien to me since I started Rust from a C/C++/Python/... background, and I guess that while I've gotten most of the concepts the idea of passing a ton of stuff around that may or may not be zero-cost to every function I call that may need to interact with it just seems like a ridiculous waste of memory. Is that the only good/idiomatic way of doing things or am I just not in the right mindset?
Edit: I know, in C you had to do a similar thing. But in C you can just pass pointers around. I could pass references, but I may need to modify it, so then I'd need to make it a mutable one.
I'll probably get used to that way of doing things eventually, it just will take me a bit to understand completely.
Edit2: I think I understand (I read this article). Now I just have to figure out (exactly) how I would implement that. Thanks for the help guys! :slight_smile:

Note: Passing around Vec<_> doesn't actually clone any of the data in the Vec<_>, just the triple of pointer, length, and capacity, so it is very cheap. If you do call clone on the Vec<_> that will clone the stuff in the vec.

Rust makes keeping track of references manageable, although getting use to lifetimes will be a challenge coming from languages like C, C++ and Python because they offer no help in keeping track of lifetimes. (either it is the programmer's problem or the runtime will do it for you and Rust is sort of in between).

One thing to make tracking lifetimes easier is to think of references as compile time locks, you can read about that here: https://limpet.net/mbrubeck/2019/02/07/rust-a-unique-perspective.html

So, after reading that article you linked to, would this be a good idea?

  1. Keep the statics as is: private.
  2. Create a struct, maybe called SoundManager or SourceManager.. Implement methods on that struct to manage the internal source list.
  3. Make a function available to retrieve a mutable and immutable manager instance, and in functions that require either, call that function to create one.

Would that work and be idiomatic?