Option to put generated context selectors into a submodule

[LPGhatguy]

First, thanks for making SNAFU, I'm really enjoying its ergonomics!

I started a new project and tried to create a couple nested errors, but hit a name collision between the selectors that SNAFU was generating and one of my existing structs.

My inner module looks like this:

pub struct Config {
    // ...
}

#[derive(Debug, Snafu)]
pub enum ConfigError {
    // ... 
}

impl Config {
    pub fn load() -> Result<Self, ConfigError> {
        // ...
    }
}

My outer module imports a couple things from this struct and exports a new error type that wraps ConfigError:

use crate::{Config, ConfigError};

#[derive(Debug, Snafu)]
pub enum SyncError {
    Config { // oh no, a name collision!
        source: ConfigError,
    }
}

pub fn sync() -> Result<(), SyncError> {
    let config = Config::load().context(Config)?; // Collision here, too!

    // ...
}

The Snafu derive macro on SyncError tries to define a struct named Config as the context selector, which ends up colliding with the struct Config that I imported before!

One solution for this particular case I thought of was to put my SNAFU error into its own mod block to scope everything the macro defines. Combined with #[snafu(visibility)] and re-exporting the error type, this seems to work:

use crate::Config;

mod error {
    use crate::config::ConfigError;
    use snafu::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility = "pub(super)")]
    pub enum SyncError {
        Config {
            source: ConfigError,
        },
    }
}

pub use error::SyncError;

pub fn sync() -> Result<(), SyncError> {
    let config = Config::load().context(error::Config)?;

    // ...
}

Another way we could solve this is by adding a configuration option like #[snafu(mod = "error")] to instruct SNAFU to wrap all of its context selectors into a module with the given name.

Does this feature make sense? Is there a better solution to this particular problem that I'm not seeing?

Thanks!

[shepmaster]

Yes, I think such an option does make sense. I expect that most people who have run into the problem have done as you suggest: put it in a module "by hand" and perform the relevant exports.

I think that your general sketch of the resulting code also is the good default when this is opted-in-to: create a module (name chosen by user), put all the context selectors in that module, then mark them as pub(super). The error itself stays where it is.

There will undoubtedly be some nuances that are run into during development, but this seems like it's in a good state to start work on.

Would you be interested in taking a crack at it?

[HeapUnderfl0w]

i actually wanted to suggest the same thing, main difference i would prefer is it to be named something like namespace (because you are basically "namespacing" the variants)

[LPGhatguy]

@shepmaster Sorry I haven't gotten back sooner! I'm interested in helping build this, but after using SNAFU in more projects I wonder if it's the right solution to the name-collision problem.

Does it make sense for SNAFU to generate the context selectors with a name that doesn't match the enum variants directly? Could we introduce an enum-variant attribute to change the generated selector name?

SNAFU could automatically name your selectors in a way that isn't your enum variant verbatim (which would obviously be a breaking change), SNAFU could name the selector FooContext or FooCtx.

For manual names, it could be reasonable to let the user specify:

use crate::{Config, ConfigError};

#[derive(Debug, Snafu)]
pub enum SyncError {
    #[snafu(selector = ConfigContext)]
    Config {
        source: ConfigError,
    }
}

pub fn sync() -> Result<(), SyncError> {
    let config = Config::load().context(ConfigContext)?; // All good now!

    // ...
}

This is a smaller hammer than namespacing all the context selectors. Alternatively, we could implement this feature as proposed, and then encourage users to then re-export their selectors. That would look like this:

use crate::{Config, ConfigError};

#[derive(Debug, Snafu)]
#[snafu(mod = "error")]
pub enum SyncError {
    Config {
        source: ConfigError,
    }
}

// We can't glob-import the context selectors if we need to rename any of them.
use error::Config as ConfigContext;

pub fn sync() -> Result<(), SyncError> {
    let config = Config::load().context(ConfigContext)?; // All good now!

    // ...
}