Day 6: Oh! Error code for coder!

Sidequest: facet-kdl

Coincidentally, while searching for the kdl docs, I found that there is another derive-based KDL crate: facet-kdl. This is part of the larger facet ecosystem championed by Amos AKA fasterthanlime. It was their articles on Rust compile times that introduced me to the costs of procedural macros, particularly those based on syn. I alluded to this problem in day 3, in fact. I recommend reading their website if you are curious, but in short: the idea is that many uses of procedural macros only exist because of a need to know the overall “shape” of a struct or enum. Facet implements a single lightweight derive macro that stores a representation of that state within the type system, and an ecosystem of crates then use the shape to do things like deserialization. This means we only pay the derive macro cost once, leading to faster builds.

Unfortunately, I wasn’t able to get facet-kdl crate to work in the way I’d like. Like knus, it is based on kdl under the hood, and exposes parsing errors using miette. miette is a lovely error reporting crate that produces human-readable parsing errors, like the one we saw yesterday:

  × identifiers cannot be used as arguments
   ╭─[test.kdl:3:22]
 2 │             defaults {
 3 │                 keep all
   ·                      ─┬─
   ·                       ╰── unexpected identifier
 4 │             }
   ╰────
  help: consider enclosing in double quotes ".."

The logic to create these visuals is a bit involved, though, with numerous dependencies, and so the docs recommend only enabling them in the top-level crate. This means that crates with miette integration expose types that implement IntoDiagnostic, relying on the top-level application to also take a dependency on miette, but with the fancy feature enabled. Each crate therefore exposes enough information to create fancy error messages, but the logic to actually display the messages is limited to the application.

Unfortunately, for reasons I couldn’t figure out, this didn’t work for me. The version of miette used by the parsing crate and the application needs to be exactly the same. Imagine how confusing and error-prone it would be if your code had to work with an arbitrary different version of its own types! To do this, I ran cargo tree to see the version that facet-kdl dependend on, and added a dependency for that version in Séance. However, I continued to get an error message that seemed to suggest that the versions didn’t match:

error[E0277]: the trait bound `KdlError: Diagnostic` is not satisfied
   --> src/main.rs:120:74
    |
120 |     facet_kdl::from_str(file_content).map_err(|diag| miette::Report::new(diag))
    |                                                      ------------------- ^^^^ the trait `Diagnostic` is not implemented for `KdlError`
    |                                                      |
    |                                                      required by a bound introduced by this call
    |
note: there are multiple different versions of crate `miette` in the dependency graph
   --> .cargo/registry/src/index.crates.io-1949cf8c6b5b557f/miette-7.6.0/src/protocol.rs:20:1
    |
 20 | pub trait Diagnostic: std::error::Error {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this is the required trait
    |
   ::: src/main.rs:4:5
    |
  4 | use http_cache_reqwest::{
    |     ------------------ one version of crate `miette` used here, as a dependency of crate `ssri`
...
 12 | use miette::{IntoDiagnostic, Context};
    |     ------ one version of crate `miette` used here, as a direct dependency of the current crate
    |
   ::: .cargo/registry/src/index.crates.io-1949cf8c6b5b557f/facet-kdl-0.29.0/src/lib.rs:35:1
    |
 35 | pub struct KdlError {
    | ------------------- this type doesn't implement the required trait
    |
   ::: .cargo/registry/src/index.crates.io-1949cf8c6b5b557f/miette-5.10.0/src/eyreish/context.rs:12:5
    |
 12 |     pub trait Diag {
    |     -------------- this is the found trait
    = help: you can use `cargo tree` to explore your dependency tree

but I could see in the source code that it did implement Diagnostic. This stumped me, so I gave up on the crate and went back to using kdl directly.

…that is, it stumped me until writing this very post. You see, I’ve just realized that the miette::Diagnostic implementation was only added last week. There is no published version of the crate that actually provides it. The incorrect version error message was a total red herring, probably generated by Rust just because it noticed that a cross-crate trait was missing, and I had multiple versions.

I’ve been bitten by these sorts of mistakes before, and so out of habit I will try to check the git tag for the version that I am actually using. I tried this at the time, but there was no tag for the currently published version in the repo, so I went off of main instead. While writing this, I thought to check the docs.rs copy of the source, and found that the latest published version is very early and is missing miette integration. That explains it.

Unfortunately, this also means that there isn’t a published version that supports miette as I’d like. Once the crate matures a bit more, I may switch to it, but for now, I’m happy continuing to use kdl directly.

Using kdl directly

After that puzzling build issue, the actual work of implementing the config parsing is quite mundane. I decided to drop everything but the feed <url> API for the MVP implementation, meaning I bypass most of the complexity we saw last time.

fn parse_config(file_content: &str) -> miette::Result<Config> {
    let doc: KdlDocument = file_content.parse()?;
    let mut feeds = Vec::with_capacity(doc.nodes().len());
    for node in doc.nodes() {
        let name = node.name().value();
        if name == "feed" {
            let [url_entry] = node.entries() else {
                return Err(miette!(
                    code = "seance::config::feed::too_many_args",
                    help = "try replacing with a single URL instead, eg `feed \"https://example.com\"`",
                    "feed only accepts a single argument"
                ));
            };

            let KdlValue::String(url_str) = url_entry.value() else {
                return Err(miette!(
                    code = "seance::config::feed::bad_arg_type",
                    help = "try replacing with a quoted URL instead, eg `feed \"https://example.com\"`",
                    "expected feed argument to be a string"
                ));
            };

            let Ok(url) = Url::parse(url_str) else {
                return Err(miette!(
                    code = "seance::config::feed::bad_url",
                    help = "try replacing with a quoted URL instead, eg `feed \"https://example.com\"`",
                    "expected argument to be a URL"
                ));
            };

            feeds.push(Feed { url });
        } else {
            return Err(miette!(
                code = "seance::config::unknown_node",
                "unknown node {name}",
            ));
        }
    }

    Ok(Config { feeds })
}

I also added unit tests for most of this behaviour, most of which I’ll omit for brevity, but here’s an example:

#[test]
fn feed_url_must_be_string() {
    assert_parse_error_code(
        "feed 1",
        "seance::config::feed::bad_arg_type"
    );
}

This is how I often write tests: short utility functions based primarily on value equality. You could make an argument from the certain philosophies of test-driven-development that this misses the point. Some claim that a primary benefit of unit tests is that it forces you to use your own API, and if that is painful, you should instead rework it. It’s an interesting idea, but I don’t think that applies in this case. I’m not writing a library, I’m writing an application, and therefore it is better for my tests to live closer to the experience of a user. I’m not totally convinced by my own counter-argument here, though. I could instead write parse_config in a more controlled format, say using my own Error type, as miette recommends for libraries, and assert on strict equality on that type. I may rewrite to do that in the future, or go even further and try something like quickcheck, but for now, some tests are better than none.

Learning how to learn

As a closing note, I first heard the idea that the purposes of unit tests is to force you to use your own API from (as I recall) an episode of The Bike Shed, fairly early in my career. A while later, I listened to another episode (potentially with the same hosts), who advocated for “DAMP” tests, standing for descriptive and meaningful phrases. This meant building up your own small set of test-only functions so that your tests read like your intent (closer to the example I gave above). I remember being quite confused by this at the time, given that these two ideas are not really compatible. In retrospect, I consider that an important early nudge towards an idea that would take a few more years to settle in for me: memorizing principles and best practices only gets you so far; to continue to learn, you have to know the underlying reasons for doing things, and consider whether they apply in any given situation. Becoming a collector of trivia that you can wield with the weight of best practice is a seductive rut, but one I’m happy to have since escaped. I’d later learn that this is meant moving beyond the novice stage in the Dreyfus model of skill acquisition.