This `FromStr` impl feels clunky, is there a better way to write it?

zackw · September 14, 2025, 8:38pm

I don't love this FromStr implementation. It feels non-uniform -- the same basic job of considering one possible parse after another is being done three different ways -- and potentially awkward to extend. How would you write it?

(I sort of want a anti-? operator here, that returns early if the result is Ok.)

const DNS_PORT: u16 = 53;

/// A parsed command line argument specifying one or more DNS
/// resolvers to use.
#[derive(Debug, Eq, PartialEq)]
enum ResolverArg {
    System,
    Quad9,
    Ip(SocketAddr),
}

impl FromStr for ResolverArg {
    type Err = AddrParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        Ok(match s {
            "system" => Self::System,
            "quad9" => Self::Quad9,
            other => if let Ok(addr) = other.parse::<IpAddr>() {
                Ip(SocketAddr::new(addr, DNS_PORT))
            } else {
                Ip(other.parse::<SocketAddr>()?)
            }
        })
    }
}

(N.B. The full program has several more keyword alternatives in the match statement.)

Ddystopia · September 14, 2025, 8:51pm

It looks totally fine to me. If you want it to be more explicit, you may wrap every variant in Ok and use .map in the fallible parse. Additionally, it should be possible to write just other if let Ok(addr) = other.parse::<IpAddr>() => ..., maybe you would like it more.

Anyway, it should be trivial for anyone to read through the 10 line piece of code and extend it. It probably isn't worth the effort to make it "more beautiful", just move on to more important tasks.

steffahn · September 14, 2025, 8:53pm

Arguably, the error handling isn’t quite correct, is it? You only return the error from unsuccessfully trying to parse as a SocketAddr which fails to acknowledge to the user that an IpAddr, or the strings system or quad9 would have been valid alternatives.

quinedot · September 14, 2025, 9:04pm

Potential semantic changes aside, I prefer this... but I think it's mostly style preference.

impl FromStr for ResolverArg {
    type Err = AddrParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        let this = match s {
            "system" => Self::System,
            "quad9" => Self::Quad9,
            other => match other.parse::<IpAddr>() {
                Ok(addr) => Self::Ip(SocketAddr::new(addr, DNS_PORT)),
                Err(_) => Self::Ip(other.parse()?),
            }
        };
        Ok(this)
    }
}

::<IpAddr> isn't even required, but is more clear IMO.

throwable-one · September 14, 2025, 9:14pm

I would add s.trim() to make sure it ignores spaces)

steffahn · September 14, 2025, 9:17pm

With a zero-size custom error type, and a helper function because the type annotations for convincing the compiler of choosing dyn Fn… are annoying otherwise, this is something I could come up with:

struct ResolverArgParseError;

fn try_alternatives<T>(alts: &[&dyn Fn() -> Option<T>]) -> Option<T> {
    alts.iter().find_map(|f| f())
}

impl FromStr for ResolverArg {
    type Err = ResolverArgParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        try_alternatives(&[
            &|| (s == "system").then(|| Self::System),
            &|| (s == "quad9").then(|| Self::Quad9),
            &|| Some(Ip(SocketAddr::new(s.parse().ok()?, DNS_PORT))),
            &|| Some(Ip(s.parse().ok()?)),
        ])
        .ok_or(ResolverArgParseError)
    }
}

of if you prefer .map(…) over use of ? here:

struct ResolverArgParseError;

fn try_alternatives<T>(alts: &[&dyn Fn() -> Option<T>]) -> Option<T> {
    alts.iter().find_map(|f| f())
}

impl FromStr for ResolverArg {
    type Err = ResolverArgParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        try_alternatives(&[
            &|| (s == "system").then(|| Self::System),
            &|| (s == "quad9").then(|| Self::Quad9),
            &|| s.parse().ok().map(|ip| Ip(SocketAddr::new(ip, DNS_PORT))),
            &|| s.parse().ok().map(Ip),
        ])
        .ok_or(ResolverArgParseError)
    }
}

Edit: Actually… it could be worse I guess, without the helper function:

struct ResolverArgParseError;

impl FromStr for ResolverArg {
    type Err = ResolverArgParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        <[&dyn Fn() -> _]>::iter(&[
            &|| (s == "system").then(|| Self::System),
            &|| (s == "quad9").then(|| Self::Quad9),
            &|| Some(Ip(SocketAddr::new(s.parse().ok()?, DNS_PORT))),
            &|| Some(Ip(s.parse().ok()?)),
        ])
        .find_map(|f| f())
        .ok_or(ResolverArgParseError)
    }
}

Edit2: Another way, if you looove your combinators

struct ResolverArgParseError;

impl FromStr for ResolverArg {
    type Err = ResolverArgParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        None.or_else(|| (s == "system").then(|| Self::System))
            .or_else(|| (s == "quad9").then(|| Self::Quad9))
            .or_else(|| s.parse().ok().map(|ip| Ip(SocketAddr::new(ip, DNS_PORT))))
            .or_else(|| s.parse().ok().map(Ip))
            .ok_or(ResolverArgParseError)
    }
}

Edit3: The combinators can also handle the original logic of keeping the last errors, e.g. like this

impl FromStr for ResolverArg {
    type Err = AddrParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        None.or_else(|| (s == "system").then(|| Self::System))
            .or_else(|| (s == "quad9").then(|| Self::Quad9))
            .or_else(|| s.parse().ok().map(|ip| Ip(SocketAddr::new(ip, DNS_PORT))))
            .ok_or(())
            .or_else(|()| s.parse().map(Ip))
    }
}

steffahn · September 14, 2025, 10:02pm

By the way, ? can absolutely actually (already) do this^[1] with a small helper type with (unstable) Try trait implementations:

feel free to skip the definitions and read on to see the usage first

#![feature(try_trait_v2)]

use std::ops::ControlFlow;
use std::ops::FromResidual;
use std::ops::Try;

#[allow(non_camel_case_types)]
struct alt<T>(T);

impl<T> Try for alt<Option<T>> {
    type Output = ();
    type Residual = alt<(T,)>;
    fn from_output((): ()) -> Self {
        alt(None)
    }
    fn branch(self) -> ControlFlow<alt<(T,)>, ()> {
        match self.0 {
            Some(x) => ControlFlow::Break(alt((x,))),
            None => ControlFlow::Continue(()),
        }
    }
}

impl<T> FromResidual<alt<(T,)>> for alt<Option<T>> {
    fn from_residual(alt((x,)): alt<(T,)>) -> Self {
        alt(Some(x))
    }
}

impl<T> FromResidual<alt<(T,)>> for Option<T> {
    fn from_residual(alt((x,)): alt<(T,)>) -> Self {
        Some(x)
    }
}

impl<T, E> Try for alt<Result<T, E>> {
    type Output = E;
    type Residual = alt<(T,)>;
    fn from_output(e: E) -> Self {
        alt(Err(e))
    }
    fn branch(self) -> ControlFlow<alt<(T,)>, E> {
        match self.0 {
            Ok(x) => ControlFlow::Break(alt((x,))),
            Err(e) => ControlFlow::Continue(e),
        }
    }
}

impl<T, E> FromResidual<alt<(T,)>> for alt<Result<T, E>> {
    fn from_residual(alt((x,)): alt<(T,)>) -> Self {
        alt(Ok(x))
    }
}

impl<T, E> FromResidual<alt<(T,)>> for Result<T, E> {
    fn from_residual(alt((x,)): alt<(T,)>) -> Self {
        Ok(x)
    }
}

Now we can simply write:

struct ResolverArgParseError(AddrParseError, AddrParseError);

impl FromStr for ResolverArg {
    type Err = ResolverArgParseError;
    fn from_str(s: &str) -> Result<Self, Self::Err> {
        alt((s == "system").then(|| Self::System))?;
        alt((s == "quad9").then(|| Self::Quad9))?;
        let e1 = alt(s.parse().map(|ip| Ip(SocketAddr::new(ip, DNS_PORT))))?;
        let e2 = alt(s.parse().map(Ip))?;
        Err(ResolverArgParseError(e1, e2))
    }
}

(playground)

and even keep both errors quite conveniently not that that’s necessarily super useful for this particular use case with AddrParseError, but anyway…

on nightly ↩︎

nerditation · September 15, 2025, 10:02am

I think the "clunky" feel comes from different abstraction levels being mixed together. what if you extract piece of code into smaller units, does it feel better?

//...
match s {
    "system" => Ok(Self::System),
    "quad9" => Ok(Self::Quad9),
    s => parse_socket_addr(s).map(Self::Ip),
}
//...

fn parse_socket_addr(s: &str) -> Result<SocketAddr, AddrParseError> {
    IpAddr::from_str(s)
        .map(|ip| SocketAddr::new(ip, DNS_PORT))
        .or_else(|_err| SocketAddr::from_str(s))
}

Schard · September 15, 2025, 11:27am

Why not use serde?

use serde::Deserializer;
use serde::de::Error;
use serde::{Deserialize, Serialize};
use std::net::SocketAddr;

const DNS_PORT: u16 = 53;

fn deser_socket_addr<'de, D>(deserializer: D) -> Result<SocketAddr, D::Error>
where
    D: Deserializer<'de>,
{
    let string = String::deserialize(deserializer)?;

    if let Ok(addr) = string.parse() {
        Ok(SocketAddr::new(addr, DNS_PORT))
    } else {
        string.parse()
    }
    .map_err(Error::custom)
}

#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
enum ResolverArg {
    System,
    Quad9,
    #[serde(untagged)]
    Ip(#[serde(deserialize_with = "deser_socket_addr")] SocketAddr),
}

fn main() {
    let json = r#"["system", "quad9", "127.0.0.1", "127.0.0.1:8080"]"#;
    let resolver_args: Vec<ResolverArg> = serde_json::from_str(json).expect("Invalid JSON");
    dbg!(resolver_args);
}

You can also wrap the socket address in a new-type:

use serde::Deserializer;
use serde::de::Error;
use serde::{Deserialize, Serialize};
use std::net::AddrParseError;
use std::net::SocketAddr;
use std::ops::Deref;
use std::str::FromStr;

const DNS_PORT: u16 = 53;

#[derive(Debug, Serialize)]
struct SockAddr(SocketAddr);

impl FromStr for SockAddr {
    type Err = AddrParseError;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        if let Ok(addr) = s.parse() {
            Ok(Self(SocketAddr::new(addr, DNS_PORT)))
        } else {
            s.parse().map(Self)
        }
    }
}

impl<'de> Deserialize<'de> for SockAddr {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        String::deserialize(deserializer)?
            .parse()
            .map_err(Error::custom)
    }
}

impl Deref for SockAddr {
    type Target = SocketAddr;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
enum ResolverArg {
    System,
    Quad9,
    #[serde(untagged)]
    Ip(SockAddr),
}

fn main() {
    let json = r#"["system", "quad9", "127.0.0.1", "127.0.0.1:8080"]"#;
    let resolver_args: Vec<ResolverArg> = serde_json::from_str(json).expect("Invalid JSON");
    dbg!(resolver_args);
}

nerditation · September 15, 2025, 11:53am

actually, inspired by your code, I come up an idea to design an Either type in place of Result, and because of Left and Right is relative neutral compared to Ok and Err, we can have a ? and "anti ? operator" at the same time ^[1]!

impl FromStr for ResolverArg {
	type Err = AddrParseError;
	fn from_str(s: &str) -> Result<Self, Self::Err> {
		Self::parse(s).into_result()
	}
}

impl ResolverArg {
	/// default convention, i.e. Error is Left, Ok is Right
 	/// so if `?` is used this function, it is the "normal" try operator
	fn parse(s: &str) -> Either<AddrParseError, Self> {
		match s {
			"system" => Either::Right(Self::System),
			"quad9" => Either::Right(Self::Quad9),
			// flip it!
			s => Self::parse_socket_addr(s).flip().map_right(Self::Ip),
		}
	}

	/// reversed convention, i.e. Ok is Left, Error is Right
	/// so this function has "anti"-try operator
	fn parse_socket_addr(s: &str) -> Either<SocketAddr, AddrParseError> {
		// anti try operator in action!
		// short-circuit the Left (i.e. Ok) case, and unwrap the Right (i.e. Err) case
		let _err = IpAddr::from_str(s)
			.ok_into_left()
			.map_left(|ip| SocketAddr::new(ip, DNS_PORT))?;
		SocketAddr::from_str(s).ok_into_left()
	}
}

full code: Rust Playground

and as a bonus, making code more confusing to read -- am I to the left or right?... ↩︎

Schard · September 15, 2025, 12:12pm

nerditation · September 15, 2025, 12:19pm

I'm aware of that crate, but I can't implement Try for foreign types.

steffahn · September 15, 2025, 12:39pm

I think you’re using Either here more like ControlFlow is conceptually thought of in the standard library. In particular, you are trying to stay a way from denoting either variant as “error”, but you do not actually make them fully neutral, given the - arbitrary - choice to “propagate Left(A), unwrap Right(B)”.

I think this isn’t the best choice for Either; the either crate seems to agree, and instead opts to offer a set of two try macros, so that each unwrap+propagation site can explicitly choose one way or the other.

The ControlFlow enum already offers a type that is meant to be exactly that: conceptionally generic & neutral (especially not calling anything an “error”) except for the fact that the Break-type one is ?-propagated.

nerditation · September 15, 2025, 1:26pm

that's correct, but I don't particularly like the vocabulary, it feels awkward to rename a function return type from Result<T, E> to ControlFlow<E, T>. I guess it's just a matter of getting used to it?

yes, this is somewhat arbitrary, but not really. I feel the try operator is kind of like do-notation, so I choose to follow the familiar convention in Haskell where Either a is instance of Monad. ^[1]

Instance Monad (Either a) where
    Left a  >>= _ = Left a
    Right b >>= f = f b

I agree that's better design and is more flexible, but I'm just playing around with the postfix try operator, only if we had different flavor of try operators... ^[2]

I tried not to mention the m-word as I heard it is scary ↩︎
time to add a "bang" operator .! to rust: the anti-? opeartor that short-circuits Ok instead of Err ↩︎

Topic		Replies	Views
Is there actually a semantic difference between `FromStr` and `TryFrom<&str>`?	16	3355	July 19, 2023
Various beginner questions (enums, from_str, overloading) code review	14	1103	February 27, 2022
Cleanest way to unwrap layers of Option/Result? help	6	2842	January 12, 2023
Error: distinct uses of `impl Trait` result in different opaque types help	12	4751	November 3, 2020
Trait that automatically implies several From implementations? help	19	1882	April 11, 2022

This `FromStr` impl feels clunky, is there a better way to write it?

Related topics