When returning a trait object, is the underlying concrete type a part of your public API?


#1

I’ve been thinking a bit about downcasting more than usual, in particular because of the rise of the failure crate. Namely, the failure crate permits iterating over all causes of an error, and potentially inspecting their underlying value via downcasting: https://docs.rs/failure/0.1.1/failure/trait.Fail.html#method.downcast_ref

My core question is this: if I return Box::new(...) as Box<failure::Error>, for example, does the type of ... become part of my public API? In particular, a consumer could write something like this:

if let Some(foo) = err.downcast_ref::<...>() {
    // do something with foo
}

Now, of course, if ... isn’t actually an exported type in my crate, then I don’t have much to fear, because the caller won’t actually be able to write downcast_ref::<...>() since ... isn’t actually visible.

However, this is not the only consideration that is important. Notably, ... could be a public type from one of your dependencies. When this type becomes part of your public API is crucially important, because it impacts how you deal with semver.

Let’s take a concrete example. I’m working on a new crate, and one of its private dependencies is the csv crate. That is, I do not intend to export any of csv's public API in my crate’s public API. However, I am using the failure crate, and I’m very tempted to do things like this:

#[derive(Clone, Debug, Fail)]
pub enum MyError {
    // This error occurs when reading input data.
    Input,
}

fn read_data(path_to_csv: &Path) -> Result<Data, MyError> {
    let mut data = Data::new();
    let mut rdr = csv::Reader::from_path(path_to_csv).context(MyError::Input)?;
    for result in rdr.records() {
        let record = result.context(MyError::Input)?;
        data.push(record[0].to_string()); // or whatever
    }
    Ok(data)
}

This all seems well and good, but with the error I return, it is possible for the caller to retrieve the underlying csv::Error type if they wanted to (by iterating over the causes and downcasting each in turn).

As the writer of this new crate, I’ve been careful not to expose the csv crate as a public dependency. Have I just been thwarted? In particular, if I upgrade my csv dependency to version 2 in a minor/patch release but consumers of my library are still using version 1, then the consumers use of the downcast will now break since v1’s csv::Error is not the same type as v2’s csv::Error.

In the Rust ecosystem, we (quite reasonably) don’t consider every technically possible breaking change as something that requires a semver release. Does something like this fall into the category of “technically breaking, but no semver release required,” perhaps by declaring that types leaked through a trait object do not result in a new public dependency on their own? IMO, this is an interesting question on a few fronts:

  • This particular breaking change does not result in a compilation error, but rather, a runtime condition that fails.
  • Checking the details of error causes is somewhat niche, and therefore the scope of this change, even if it is a breaking change, is probably almost always very small. (For example, I’ve never had the occasion of wanting to look closely at any error other than io::Error.)
  • Preventing the leakage of public errors through the causes returned by the failure crate is possible, but it is definitely not the happy path.

Another interestingly similar, but perhaps orthogonal, point on this matter, is the ease at which someone might define impl From<private_dependency::Error> for MyError in their crate, and thereby also accidentally introducing a new public type and potentially an unintended public dependency. I know I’ve certainly done this myself, but have also never experienced any fallout from it, despite, in practice, ignoring the fact that it was a public dependency. (For example, regex-syntax was very specifically never intended to be a public dependency of regex, but such a From impl exists in the regex public API, and therefore, technically it is. But I have bumped the semver version of regex-syntax several times without ever being punished for it. Perhaps because regex-syntax is so rarely used.)


#2

This is a good question.

Here’s my views: I generally think public visibility and public API should not be confused. There’s many reasons a type should have public visibility, while staying an implementation detail. For example, one of your subcrates exports types publicly, but only to be used by a supercrate. In this case, exports are good practice, but there’s no one keeping users to import the other crates and used these types in another fashion (maybe, to write their own crates). Obviously, visibility can be used for some enforcement, but it’s not enough.

Sadly, rustdoc has no API markers, like other tools (YARD, etc.) have, which I would use for communicating all these things.

So, in general, I would not say that returning a trait object makes the wrapped type public API if it is not intended as such.


#3

I am less confident about this. The thing is, people can be using this “not intentionally public” API entirely by accident. In fact, I picture the opposite scenario from the one presented:

  • sushi privately depends on csv-1.0.
  • User explicitly depends on csv-2.0 and sushi.
  • User downcasts a boxed error to csv::Error, expecting to only get errors from their use of csv. (and this is what happens)
  • sushi updates its csv dependency to csv-2.0 and publishes a minor update.
  • Suddenly, now, the user’s downcast sometimes succeeds for errors from sushi, breaking the user’s code.

Perhaps this still is the user’s fault for falsely assuming that the csv errors come from their use of csv. But can they really be blamed for this?


#4

FWIW, you can get a similar downcast leak from impl Trait:

use std::any::Any;
use std::ops::Range;

fn foo() -> impl Iterator<Item = i32> { 0..10 }

fn main() {
    let iter = foo();
    let r = Any::downcast_ref::<Range<i32>>(&iter);
    println!("{:?}", r);
}

I don’t think this should be considered part of the public API. Whether a trait object or impl Trait, the public intent is clearly an erased type.

I do think your impl From example is public API though, even though you got away with changing it.


#5

*for impl Trait
Personally think you’ve hit an oversight in implementation, maybe more developers could confirm.
get_type_id gives the same; for a simple struct Wrap<T>(T) it gives different as I would expect.

I think the answer to the OP is the same as “Is this a circle” (not sure original source so just search and pick one.)


#6

No, but the code was faulty before the upgrade. I would also not fault the library developer for this.

I find “never break, even in the presence of previous construction mistakes” not a feasible thing to follow.

I agree that it is a problem, but covering the combination of multiple crates of different versions is something I wouldn’t really want to weight on library developers.


#7

I posted an issue on the API guidelines repo tracking this with a brief summary of this thread and some suggestions: https://github.com/rust-lang-nursery/api-guidelines/issues/162

Thanks everyone for the feedback!