Consider alternate designs for traits and APIs for stream isolation

This Tuesday, @eta, @Diziet, and I talked about different possible interfaces for the new stream isolation APIs. This ticket exists mainly to record our discussion for future reference. See also #414 (closed) for other stream isolation followups.

(cc @trinity-1686a)

Right now there are two traits used for isolation: Isolation and IsolationHelper. The Isolation trait applies to all possible stream isolation values, and is used internally to hold "the real underlying isolation setting of a stream or circuit". The IsolationHelper trait is used to implement a particular type-oriented kind of isolation: every type of IsolationHelper is incompatible with Isolations of any other type. We expect that users will (almost?) always want to just implement IsolationHelper.

The questions we were trying to answer basically come down to:

• If users are only expected to implement IsolationHelper, is it necessary for Isolation to be public?
• Should the user-facing API use generics or trait objects?

IIRC, we all generally agreed about these points:

• It would be better not to expose more traits than necessary.
• It would be good if we do not prevent ourselves, down the road, from making an accessor to get retrieve a stream's isolation setting.
• Adding generic parameters to StreamPrefs could be at least a bit confusing...
• ... but making users think about dyn Isolation could also be at least a bit confusing.
• We should make sure that users can understand what isolation is, how it works, and how to use it.
• ... but we shouldn't make users think they need more knowledge than they really do.

We didn't all agree on these points:

• Whether generic parameters on StreamPrefs or trait objects are more confusing.
• Whether it's important to specify how isolation works "mathematically", and therefore wise to expose the Isolation trait.
• Whether exposing the Isolation trait will confuse users who might otherwise have been happy with IsolationHelper.
• Whether we might someday want to have Isolation objects that aren't also IsolationHelper.
• Whether it is more confusing to let the user know that there is downcasting doing on behind the scenes, or more confusing to make the user wonder how Isolation actually works.
• Whether to rename Isolation to AbstractIsolation and IsolationHelper to Isolation, or something like that.
• How important it is to be able to pass a pre-existing Box<dyn Isolation> as the isolation for a new stream.

---

Here are the designs that we thought about:

proposal A

pub struct StreamPrefs {
    iso: Box<dyn Isolation>,
}
impl StreamPrefs {
    fn set_isolation<T: Isolation>(&mut self, iso: T);
    // or maybe
    fn set_isolation(&mut self, iso: Box<dyn Isolation>);
    // (or maybe both)
    fn get_isolation(&self) -> &dyn Isolation;
}

// hypothetically later on
impl DataStream {
    fn get_isolation(&self) -> &dyn Isolation;
}

This option avoided (most) generic parameters in the API, at the expense of more generic parameters.

This option is the closest to what we actually did. We refined the API in !418 (merged) to

   fn set_isolation<T>(&mut self, iso: T)
      where T: Into<Box<dyn Isolation>>

proposal B

pub struct StreamPrefs<T: Isolation = IsolationToken> {
    iso: T,
}

impl StreamPrefs {
    fn set_isolation(&mut self, iso: T);
    fn get_isolation(&self) -> &T;
}

struct TorClient<R> {
    ... stream_prefs: StreamPrefs<IsolationToken> ...
}
impl<R> TorClient<R> {
    fn set_prefs(&self, new_prefs: StreamPrefs<IsolationToken>) {...}
    fn connect_with_prefs<TA, T>(&self, target_addr: TA, prefs: StreamPrefs<T>) {...}
}

// hypothetically later on
struct DataStream {
    // TorClient converts the StreamPrefs<T> into the Box<dyn Isolation>
    // when the stream is created
    isolation: Box<dyn Isolation>
}

impl DataStream {
    // does a downcast internally
    fn get_isolation<T: Isolation>(&self) -> Option<&T>;
}

This option avoided trait objects in the API, at the expense of more generic parameters.

proposal C

pub struct StreamPrefs {
    iso: dyn Isolation,
}

impl StreamPrefs {
    fn set_isolation<T: Isolation>(&mut self, iso: T);
    fn get_isolation(&self) -> IsolationRef;
}

struct IsolationRef (
    Box<dyn Isolation>
);

impl Isolation for IsolationRef {..}

Nobody but @nickm liked this one. The idea was to introduce an IsolationRef to hide the existence of dyn.

Proposal D

// Name subject to bikeshed. IsolationBase = Isolation from A.
struct IsolationRef(Box<dyn IsolationBase>);

impl<T: Isolation> From<T> for IsolationRef { ... }
impl IsolationRef {
    fn downcast(&self) -> Option<&T>;
}

impl StreamPrefs {
    fn set_isolation(&mut self, iso: IsolationRef);
    fn get_isolation(&self) -> &IsolationRef;
}

// hypothetically later on
struct DataStream {
    isolation: IsolationRef
}

impl DataStream {
    fn get_isolation(&self) -> &IsolationRef;
}

---

All of the proposals above were disliked by at least one person, except for "Proposal B" which we disliked by two people.