Send and Sync errors guideline should call attention to trait objects

[dtolnay]

reqwest::Error::get_ref returns Option<&(Error + 'static)> which is an error that is not Send and Sync. This is easy to miss and should be noted in C-SEND-SYNC-ERR.

[Kixunil]

Why should errors be Send and Sync?

[dtolnay]

It is explained in the guideline. https://github.com/brson/rust-api-guidelines#c-send-sync-err

[sfackler]

They also can't be put into an io::Error.

[dtolnay]

I filed #87 to include io::Error in the justification.

[Kixunil]

@dtolnay That justification would be applicable to all types. "All types should be Send and Sync so they can be passed between threads." Why errors are special? Also sharing error in Arc doesn't seem very useful to me. (I've never seen anyone doing it and I can't imagine real-life situation in which anyone would do it.)

TBH I hate io::Error and I'd deprecate it in a heartbeat. But I admit it's good justification considering it already exists.

There is another thing I dislike about this guidance. Consider this example:

impl Config {
    fn load(file_name: &str) -> Result<Self, ConfigError> {
        // ...
    }
    // ...
}

This is how it'd look like with this guideline However I believe ConfigError should contain reference to file name so the message can be "cannot open file /foo/bar: File doesn't exist" instead of "cannot open file: File doesn't exist". Which file?!

Allocating new String might be unnecessary. I suggest these kinds of errors should have .to_owned() method returning e.g. ConfigError<'static> to support cases mentioned in the rationale for this guideline.

[sfackler]

Non-'static errors are incredibly painful to work with. 99% of the time you're going to be passing the error up to the calling function somehow. Having to do Config::load(the_path).map_err(|e| e.to_owned()? instead of Config::load(the_path) seems pretty bad when multiplied across every result-returning function anywhere. Why is it concerning to allocate a String? Is failure to parse a configuration a particularly hot code path?

[Kixunil]

Well, extension trait for Result would help. Config::load(the_path).to_owned()? doesn't seem that bad to me.

It may not be hot path in case of Config but there might be other, more interesting cases.