Clean up pseudo-downcasting from VpnProvider supertrait to subtraits with better solution

[jamesmcm]

The idea was that we could then pass trait objects to the functions that use the VPN provider objects with dynamic dispatch (i.e. as Box<dyn VpnProvider>, and then check if the actual struct implements the necessary WireguardProvider or OpenVpnProvider traits depending on the command line arguments, and return an error if not.

The problem is that there is no way to try to downcast from a supertrait trait object to subtrait. That is we want something like:

fn myfn(obj: Box<dyn VpnProvider>) -> anyhow::Result<()> {
    let as_wireguard: Box<dyn WireguardProvider> = try_downcast<WireguardProvider>(obj)?;
    // ...
}

But no such function exists. As far as I can tell, the downcast-rs crate is for downcasting to concrete types, not subtrait trait objects. This forum post offers some possibilities using std::mem::transmute but it is unsafe, and not guaranteed to be stable (note the need for #[repr(C)] for example).

For now I worked around this by using the fact that we have the enum from the command line arguments, so we can use that to try to generate the trait object we want. But this is a painful workaround as we have a fair amount of boilerplate code and some extra allocation of trait objects.

Relevant code:

vopono/src/providers/mod.rs

Lines 40 to 80 in 014cd42

	// Do this since we can't downcast from Provider to other trait objects
	impl VpnProvider {
	pub fn get_dyn_provider(&self) -> Box<dyn Provider> {
	match self {
	Self::PrivateInternetAccess => Box::new(pia::PrivateInternetAccess {}),
	Self::Mullvad => Box::new(mullvad::Mullvad {}),
	Self::TigerVpn => Box::new(tigervpn::TigerVPN {}),
	Self::ProtonVpn => Box::new(protonvpn::ProtonVPN {}),
	Self::MozillaVpn => Box::new(mozilla::MozillaVPN {}),
	Self::Custom => unimplemented!("Custom provider uses separate logic"),
	}
	}
	pub fn get_dyn_openvpn_provider(&self) -> anyhow::Result<Box<dyn OpenVpnProvider>> {
	match self {
	Self::PrivateInternetAccess => Ok(Box::new(pia::PrivateInternetAccess {})),
	Self::Mullvad => Ok(Box::new(mullvad::Mullvad {})),
	Self::TigerVpn => Ok(Box::new(tigervpn::TigerVPN {})),
	Self::ProtonVpn => Ok(Box::new(protonvpn::ProtonVPN {})),
	Self::MozillaVpn => Err(anyhow!("MozillaVPN only supports Wireguard!")),
	Self::Custom => Err(anyhow!("Custom provider uses separate logic")),
	}
	}
	pub fn get_dyn_wireguard_provider(&self) -> anyhow::Result<Box<dyn WireguardProvider>> {
	match self {
	Self::Mullvad => Ok(Box::new(mullvad::Mullvad {})),
	Self::MozillaVpn => Ok(Box::new(mozilla::MozillaVPN {})),
	Self::Custom => Err(anyhow!("Custom provider uses separate logic")),
	_ => Err(anyhow!("Wireguard not implemented")),
	}
	}
	pub fn get_dyn_shadowsocks_provider(&self) -> anyhow::Result<Box<dyn ShadowsocksProvider>> {
	match self {
	Self::Mullvad => Ok(Box::new(mullvad::Mullvad {})),
	Self::Custom => Err(anyhow!("Start Shadowsocks manually for custom provider")),
	_ => Err(anyhow!("Shadowsocks not supported")),
	}
	}
	}

[jdm]

What if the VpnProvider trait has a as_wireguard(&self) method that returns Option<&WireguardProvider>?

[Lej77]

I played around with making a macro that auto implements a trait that has methods like fn as_wireguard(&self) -> Option<&WireguardProvider>. My macro uses the impls crate to determine if a type implements a trait and calls totally different methods based on if a type implements a trait. This allows the code to only attempt casting to a trait when a type actually implements a trait. The trait testing only works on concrete types not on generics so we still need a macro not a blanket implementation. Here is the code:

pub trait ProviderCast {
    fn as_dyn_openvpn_provider(&self) -> Option<&dyn OpenVpnProvider>;
    fn into_dyn_openvpn_provider(
        self: Box<Self>,
    ) -> Result<Box<dyn OpenVpnProvider>, Box<dyn Provider>>;

    fn as_dyn_wireguard_provider(&self) -> Option<&dyn WireguardProvider>;
    fn into_dyn_wireguard_provider(
        self: Box<Self>,
    ) -> Result<Box<dyn WireguardProvider>, Box<dyn Provider>>;

    fn as_dyn_shadowsocks_provider(&self) -> Option<&dyn ShadowsocksProvider>;
    fn into_dyn_shadowsocks_provider(
        self: Box<Self>,
    ) -> Result<Box<dyn ShadowsocksProvider>, Box<dyn Provider>>;
}

/// This wrapper helps the [`impl_provider_cast`] macro to implement the
/// [`ProviderCast`] trait.
#[allow(non_camel_case_types)]
#[doc(hidden)]
pub(crate) struct impl_provider_cast_wrapper<T>(T);
// The implementation for when a trait is NOT implemented for a type.
impl<T> impl_provider_cast_wrapper<[T; 0]> {
    pub fn cast<U, E, F: FnOnce(T) -> E>(value: T, f: F) -> Result<U, E> {
        Err(f(value))
    }
    pub fn cast_ok<U, F: FnOnce(T) -> T>(_: T, _: F) -> Option<U> {
        None
    }
}
// The implementation for when a trait is implemented. We could require more from
// the type via more strict trait bounds in the where clauses.
impl<T> impl_provider_cast_wrapper<[T; 1]> {
    pub fn cast<U, E, F: FnOnce(T) -> U>(value: T, f: F) -> Result<U, E> {
        Ok(f(value))
    }
    pub fn cast_ok<U, F: FnOnce(T) -> U>(value: T, f: F) -> Option<U> {
        Some(f(value))
    }
}
macro_rules! _impl_provider_cast {
    ($self_type:ty) => {
        impl $crate::providers::ProviderCast for $self_type {
            fn as_dyn_openvpn_provider(
                &self,
            ) -> Option<&dyn $crate::providers::OpenVpnProvider> {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::OpenVpnProvider)
                        as usize],
                >::cast_ok::<&dyn $crate::providers::OpenVpnProvider, _>(self, |v| v)
            }
            fn into_dyn_openvpn_provider(
                self: Box<Self>,
            ) -> Result<
                Box<dyn $crate::providers::OpenVpnProvider>,
                Box<dyn $crate::providers::Provider>,
            > {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::OpenVpnProvider)
                        as usize],
                >::cast::<
                    Box<dyn $crate::providers::OpenVpnProvider>,
                    Box<dyn $crate::providers::Provider>,
                    _,
                >(self, |v| v)
            }

            fn as_dyn_wireguard_provider(
                &self,
            ) -> Option<&dyn $crate::providers::WireguardProvider> {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::WireguardProvider)
                        as usize],
                >::cast_ok::<&dyn $crate::providers::WireguardProvider, _>(self, |v| v)
            }
            fn into_dyn_wireguard_provider(
                self: Box<Self>,
            ) -> Result<
                Box<dyn $crate::providers::WireguardProvider>,
                Box<dyn $crate::providers::Provider>,
            > {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::WireguardProvider)
                        as usize],
                >::cast::<
                    Box<dyn $crate::providers::WireguardProvider>,
                    Box<dyn $crate::providers::Provider>,
                    _,
                >(self, |v| v)
            }

            fn as_dyn_shadowsocks_provider(
                &self,
            ) -> Option<&dyn $crate::providers::ShadowsocksProvider> {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::ShadowsocksProvider)
                        as usize],
                >::cast_ok::<&dyn $crate::providers::ShadowsocksProvider, _>(self, |v| v)
            }
            fn into_dyn_shadowsocks_provider(
                self: Box<Self>,
            ) -> Result<
                Box<dyn $crate::providers::ShadowsocksProvider>,
                Box<dyn $crate::providers::Provider>,
            > {
                $crate::providers::impl_provider_cast_wrapper::<
                    [_; ::impls::impls!($self_type: $crate::providers::ShadowsocksProvider)
                        as usize],
                >::cast::<
                    Box<dyn $crate::providers::ShadowsocksProvider>,
                    Box<dyn $crate::providers::Provider>,
                    _,
                >(self, |v| v)
            }
        }
    };
}
// This export allows the macro to be used before the location where it is declared.
pub(crate) use _impl_provider_cast as impl_provider_cast;

The Provider trait could then add the ProviderCast trait as a supertrait:

/// The base trait for any VPN provider
pub trait Provider: ProviderCast {
    fn alias(&self) -> String;

    fn default_protocol(&self) -> Protocol;

    fn provider_dir(&self) -> anyhow::Result<PathBuf> {
        Ok(vopono_dir()?.join(self.alias()))
    }
}

which would require that all providers use the macro to actually implement it. The different providers would be hidden from this complexity since they only need to implement the trait via the macro like so:

crate::provider::impl_provider_cast!(Mullvad);

The VpnProvider enum could then generate providers like so:

// Do this since we can't downcast from Provider to other trait objects
impl VpnProvider {
    pub fn get_dyn_provider(&self) -> Box<dyn Provider> {
        match self {
            Self::PrivateInternetAccess => Box::new(pia::PrivateInternetAccess {}),
            Self::Mullvad => Box::new(mullvad::Mullvad {}),
            Self::TigerVpn => Box::new(tigervpn::TigerVPN {}),
            Self::ProtonVpn => Box::new(protonvpn::ProtonVPN {}),
            Self::MozillaVpn => Box::new(mozilla::MozillaVPN {}),
            Self::Custom => unimplemented!("Custom provider uses separate logic"),
        }
    }

    pub fn get_dyn_openvpn_provider(&self) -> anyhow::Result<Box<dyn OpenVpnProvider>> {
        Self::get_dyn_provider(self)
            .into_dyn_openvpn_provider()
            .map_err(|provider| anyhow!("{} doesn't supports OpenVPN!", provider.alias()))
    }

    pub fn get_dyn_wireguard_provider(&self) -> anyhow::Result<Box<dyn WireguardProvider>> {
        Self::get_dyn_provider(self)
            .into_dyn_wireguard_provider()
            .map_err(|provider| anyhow!("Wireguard not implemented for {}", provider.alias()))
    }

    pub fn get_dyn_shadowsocks_provider(&self) -> anyhow::Result<Box<dyn ShadowsocksProvider>> {
        if let Self::Custom = self {
            Err(anyhow!("Start Shadowsocks manually for custom provider"))
        } else {
            Self::get_dyn_provider(self)
                .into_dyn_shadowsocks_provider()
                .map_err(|provider| {
                    anyhow!("Shadowsocks not supported for {}", provider.alias())
                })
        }
    }
}

The complexity of this solution might or might not be worth it in your case.

[drmason13]

My intuition, is to not use a super trait at all, have the different providers impl Provider, with sensible defaults.

Looking a bit at the code I noticed that the wireguard trait requires methods that return an anyhow error (since it might fail). We could have a default impl return the error "this provider does not provide a wireguard implementation". This is after all essentially what is done using the enum currently, correct me if I'm wrong!

That way everything would work with dyn Provider and providers would specify a complete interface when implementing the (now much larger) trait. Anything they can't/won't provide, leave the default implementation.

I hope to be able to prove with a pull request that this works, but feel free to tackle yourself if you like :)

[jamesmcm]

@drmason13 I thought of doing it like that, but it means adding a bit of boilerplate to every provider implementation. It also means it's a bit less clear of the division between what is needed for OpenVPN and what is needed for Wireguard (for the providers that provide only one).

That said, the number of possible protocols shouldn't really change, so it might be the best solution.

[Lej77]

I released a crate cast_trait_object that basically generalizes the macro I proposed previously. I also searched around for other similar crates and wrote a section in the docs about them which you might want to check out.

[jamesmcm]

Thanks! I'll take a look.