Review/audit phantom type usage

[ebfull]

• Do we gain enough from the phantom types to use them? What do we gain? What do we lose?
• Should they be tuple structs or enums? enums for now, structs later if necessary.
• The State trait might need to be marked unsafe to follow the conventions of other libraries. done

[trombonehero]

I'm just learning Rust, so it's possible that my issue is due to a lack of understanding rather than a shortcoming of the phantom types model, but...

I'm attempting to write a pcap client that will either open a dumpfile or else configure a live capture interface and open it. I then want to process the resulting pcap::Capture using methods defined on impl<T: Activated> Capture<T>, à la:

let mut x:&Capture<Activated> = file.or_else(device.and_then(Capture::open));
x.run();

(or, alternatively, to write a function that could do the above logic and return a boxed Capture<Activated> so that the caller wouldn't have to care where the Capture came from)

However, it seems that I can't take a reference to a Capture<Activated>:

src/main.rs:101:15: 101:34 error: the trait `core::marker::Sized` is not implemented for the type `pcap::Activated` [E0277]
src/main.rs:101     let mut x:&Capture<Activated>;
                              ^~~~~~~~~~~~~~~~~~~
src/main.rs:101:15: 101:34 note: `pcap::Activated` does not have a constant size known at compile-time
src/main.rs:101     let mut x:&Capture<Activated>;
                              ^~~~~~~~~~~~~~~~~~~

So instead, I'm writing a enum type to unify the two kinds of Capture:

enum Capture {
    File(Capture<Offline>),
    Device(Capture<Active>),
}

impl Capture {
    fn run(&mut self) {
        match self {
            &mut Capture::File(ref mut f) => { /* ... */ f.next() /* ... */ },
            &mut Capture::Device(ref mut d) => { /* ... */ d.next() /* ... */ },
        }
    }
}

This seems pretty redundant, given that the pcap crate already tries to unify the usage of Capture<Active> and Capture<Offline>. So, am I just misunderstanding the pcap crate (or Rust itself)? Or is there no way to box or reference a Capture<Activated>?

[ebfull]

Thank you very much for your feedback. That's absolutely something I would need to support to make this worthwhile ergonomically.

Capture<T: State> has another implicit Sized bound on T, which I can remove. However, I would need to implement CoerceUnsized for Capture somehow. Two problems: it's on nightly (I can feature gate I suppose) and CaptureUnsized doesn't appear to work properly with PhantomData (yet?).

If you're using nightly anyway and feeling brave, give this branch a shot. (See the included tests as proof of the functionality you want. :))

[ebfull]

eddyb pointed out to me... Capture<Activated> isn't a DST. Doh!

I've implemented the feature you want in the latest version which I've published (0.4.1).

If cap is a Capture<Active> or a Capture<Offline>, use cap.into() to turn it into a Capture<Activated>. :)

[ebfull]

The current API design competes against a design which would probably involve an internal "CaptureBox" which contains the Unique<pcap_t> and impls Drop. We'd make InactiveCapture, ActiveCapture and OfflineCapture types which contain that box and pass ownership of it around as necessary. (Bonus, no need to transmute, which is necessary with the phantom types we have now.) ActiveCapture and OfflineCapture would impl Activated and that trait would generically provide for their functionality as opposed to InactiveCapture. This is basically the current scheme but without phantom types.

The advantages to that design are that less symbols need to be imported by the end user sometimes. There wouldn't really be any code reuse and it might be simpler to understand what's going on and where to start. (After all, inference makes constructing the captures in the first place kind of magic. I also reordered the impls in the source so they would come out the way I wanted to in the docs...) You could just Box to interact with it as a trait object like Box<Activated> if you want, if it's object safe. Probably.

It's starting to feel more compelling to use that design than to keep the current one, but I have to spec it out to be sure.

The only other design would be a single Capture which mimics the upstream API, but allows for more mistakes at runtime. I don't think this is a good idea.

[trombonehero]

Thanks for your response!

So, I've managed to write the following function, which seems to be what I want:

fn open_source(args: &Args) -> Result<pcap::Capture<pcap::Activated>, pcap::Error> {
    let source = args.arg_source.as_ref();

    pcap::Capture::from_device(source)
        .map(|d| d.promisc(args.flag_promiscuous)
                  .rfmon(args.flag_promiscuous)
                  .snaplen(args.flag_snaplen)
                  .timeout(args.flag_timeout))
        .and_then(|d| d.open())
        .map(|mut c| c.into())
        .or(
            std::fs::metadata(source)
                .map_err(|e| pcap::Error::PcapError(format!["{}", e]))
                .and_then(|f|
                    if f.is_file() {
                        pcap::Capture::from_file(source)
                            .map(|mut c| c.into())
                    } else {
                        Err(pcap::Error::PcapError(
                                format!["{} is not a file or interface", source]))
                    }
                )
        )
}

(apologies for potentially terrible / non-idiomatic Rust, I'm just learning)

This compiles ok — I suppose this is due to Capture now having the type parameter T: State + ?Sized? Anyhow, I can call open_source, but when I try to use it I get some unexpected results:

src/main.rs:92:31: 92:54 error: no method named `filter` found for type `&mut pcap::Capture<pcap::Activated>` in the current scope
src/main.rs:92     capture.map(|ref mut c| c.filter(args.arg_filter));
                                             ^~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of closure expansion
src/main.rs:92:17: 92:54 note: expansion site
src/main.rs:92:31: 92:54 note: the method `filter` exists but the following trait bounds were not satisfied: `pcap::Activated : core::marker::Sized`, `pcap::Capture<pcap::Activated> : core::iter::Iterator`, `&mut pcap::Capture<pcap::Activated> : core::iter::Iterator`, `pcap::Activated : core::marker::Sized`, `pcap::Capture<pcap::Activated> : core::iter::Iterator`

So now the compiler is complaining that pcap::Activated doesn't implement core::marker::Sized (among other things)... am I reading that correctly?

[ebfull]

I just updated pcap to 0.4.2 which should now allow you to do that.

By the way, your code is very nice Rust -- nicer than I write usually.

edit: Oh, I should elaborate on what's going on. .filter() is provided by an impl of Capture<T: Activated>, and although Activated (the trait) implements itself, there is an implicit Sized bound on virtually everything in Rust, which you must opt out of. I forgot to do this in my last update. Whoops!

[trombonehero]

Ah, that's great, thanks! My new open_capture function looks like:

fn open_capture(args: &Args) -> PcapResult {
    let source = args.arg_source.as_ref();

    let capture:PcapResult = pcap::Capture::from_device(source)
        .map(|d| d.promisc(args.flag_promiscuous)
                  .rfmon(args.flag_promiscuous)
                  .snaplen(args.flag_snaplen)
                  .timeout(args.flag_timeout))
        .and_then(|d| d.open())
        .map(|c| c.into())
        .or(
            std::fs::metadata(source)
                .map_err(|e| pcap::Error::PcapError(format!["{}", e]))
                .and_then(|f|
                    if f.is_file() {
                        pcap::Capture::from_file(source)
                            .map(|c| c.into())
                    } else {
                        Err(pcap::Error::PcapError(
                                format!["{} is not a file or interface", source]))
                    }
                )
        )
        ;

    capture.and_then(|mut c| {
        try![c.filter(&args.flag_filter)];
        Ok(c)
    })
}

and it almost works. :) I can open capture files, but there's an issue opening live interfaces... I'm quite certain that it's my fault, however; I think your library API is now very easy to use succinctly. Thank you!

[aldanor]

This could be probably closed now?

[trombonehero]

Probably could be, yup.