Implement #[snafu(module)] support

SamWilsn commented 3 years ago

Here's a rough first pass at implementing submodule support. This is my first time in proc-macro land, so apologies if anything is implemented horribly!

There are a couple points that I wanted to discuss before doing too much more work:

Like @shepmaster mentioned in #286, super:: is problematic; would checking for super, and just prepending another super be Good Enough™ ? I think a similar prepend super approach would work for visibility too (possibly adding in).
The visibility of types inside the module has to be at least as restrictive as the module itself, which seems to work the way I have it now, but I'd like another pair of eyes to take a look.
I've somewhat overloaded the visibility attribute to control both the module and the types in the module (to "solve" the previous point.) Would there be much value in controlling the visibility of types in the module separately from the module itself, and if so, how would you like it to look?
Would you be horribly opposed to clearing the suffix (by default) if module is enabled?

shepmaster commented 3 years ago

Thanks! I didn't expect a PR quite so soon 😊.

As a word of caution, I'll probably be a bit slow in reviewing this due to other responsibilities, but I'm excited to read through it.

As a meta question, since you are a new contributor to this code base, how did you find the process of contributing? I'm familiar enough with the code so it's harder for me to make meaningful judgements about its quality and extensibility.

SamWilsn commented 3 years ago

As a meta question, since you are a new contributor to this code base, how did you find the process of contributing? I'm familiar enough with the code so it's harder for me to make meaningful judgements about its quality and extensibility.

Generally I think it's pretty easy to work with. I've seen much worse Rust code bases. Some bits of struck me as slightly weird, like shadowing ContextSelector, but again, I think it's generally good!

One question I had, and it might be documented somewhere, is how do I run the full test suite locally, if I want to make sure I didn't break backwards compatibility?

As a word of caution, I'll probably be a bit slow in reviewing this due to other responsibilities, but I'm excited to read through it.

Absolutely no worries! Take as much time as you need.

shepmaster commented 3 years ago

like shadowing ContextSelector

Hmm, where do I do that?

how do I run the full test suite locally

Heh, I honestly forget all the different kinds of tests; that's what CI is for 😉 The main things I do:

cargo test — catches the majority of cases
for i in compatibility-tests/*; do pushd $i; cargo test; popd; done — you do have to watch a bit to look for the failures though.
Then there are the weird ones like cargo +nightly -Z minimal-versions update which requires all sorts of Very Extra Special Setup.

SamWilsn commented 3 years ago

like shadowing ContextSelector

Hmm, where do I do that?

https://github.com/shepmaster/snafu/blob/42bcc3355dc77e0fc07a098e7b012a9fb308186a/snafu-derive/src/lib.rs#L1251

...

Thanks!

shepmaster commented 3 years ago

FWIW, I pushed some extra commits to your branch fixing some minor nits instead of making you fix them ;-)

SamWilsn commented 3 years ago

I confess to being a bit surprised that there aren't any test failures here. Perhaps the use super::* is enough for our purposes? [...] Do you have any test cases up your sleeve where the current implementation might fail?

I do!

struct BadError {
    // ...
}

mod inner {
    #[snafu(module)]
    enum Error {
        Bad { source: super::BadError }
    }
}

Which expands to, roughly:

struct Bloop {
    // ...
}

mod inner {
    mod error {
        use super::*;

        enum Error {
            Bad { bloop: super::Bloop }
        }

        struct BadSnafu {
            bloop: Into<super::Bloop>,
        }
    }
}

The two super::Bloop types won't correctly resolve. They'd have to be super::super::Bloop.

The visibility of types inside the module has to be at least as restrictive as the module itself, which seems to work the way I have it now, but I'd like another pair of eyes to take a look.

Are you referring to the context selectors as the "types inside the module"?

Yes, exactly.

Why do you say they have to be at least as restrictive (but could be more restrictive?). If they were more restrictive, wouldn't we not be able to use them?

The module could be pub(crate) and the context selectors could be pub(super) and still be useful, I think. In any case, that's what I meant by "more restrictive".

This example illustrates the problem when the visibilities aren't correct, I think, though I might have messed it up a little from memory:

struct Bar;

pub(crate) mod a_module {
    pub struct Foo {
        pub x: crate::Bar,
    }
}

fn main() {
    let x = a_module::Foo {
        x: Bar,
    };
}

playground

One strawman syntax to propose would be #[snafu(module(name, visibility(...)))] or #[snafu(module(name(...), visibility(...)))].

That's exactly what I was thinking, and would be happy to wait on implementing it until someone wants it.

Would you be horribly opposed to clearing the suffix (by default) if module is enabled?

I'm torn! Doing a bit of thinking and asking other folk, I think I'd prefer to maintain consistency and keep the suffix.

Keeping the suffix would mean wrapping an error would look like:

let output = some_risky_operation()
    .context(error::RiskySnafu {
        // ...
    })?;

With error:: there, I definitely feel like Snafu is redundant, but I don't want to be too pushy about it.

This means that people would be able to easily switch to using a module with a use the_name::*.

You think it'll be common for people to switch existing code to the module mode?

SamWilsn commented 3 years ago

Hey! Just wanted to check to see if we're waiting on me for anything. If not, no rush!

shepmaster commented 3 years ago

we're waiting on me for anything

Nothing in particular, just life happening.

The two super::Bloop types won't correctly resolve.

So, what do you feel we should do? We could make the statement that it's a known limitation for now, :shipit: and potentially improve it in the future (yay for tests helping us not slide backwards). Maybe create an issue for people to weigh in on as they use it to provide feedback.

We could even scrape public code that uses SNAFU later on to see how they use it.

common for people to switch existing code to the module mode

I'm not sure, but I think "yes" in a few cases/ways:

I have an enum with { A, B } and then create a new enum with { B, C }. I'd have to move at least one of them, but would probably move both for consistency.
"Switching" as in mentally changing gears. If the macro behaves differently for modules and non-modules beyond just modules, it might be confusing. I'm a bit gunshy because this has been the hands-down most confusing aspect of SNAFU so far.

With error:: there, I definitely feel like Snafu is redundant,

I think you are right, which is why I would want to continue to allow people to remove it. There are (admittedly obscure) cases where the suffix might be useful still, such as when someone glob-imports the enum variants and the context selectors (ignoring both natural namespacing tools).

SamWilsn commented 3 years ago

The two super::Bloop types won't correctly resolve.

So, what do you feel we should do? We could make the statement that it's a known limitation for now, :shipit: and potentially improve it in the future (yay for tests helping us not slide backwards). Maybe create an issue for people to weigh in on as they use it to provide feedback.

We could even scrape public code that uses SNAFU later on to see how they use it.

I'm okay just leaving it broken for now, and maybe looking into prepending super:: as a longer term solution? There might even be a more, er, standardized way of doing it. I doubt snafu would be the only proc-macro with this issue.

common for people to switch existing code to the module mode

I'm not sure, but I think "yes"

Yeah, that makes sense.

With error:: there, I definitely feel like Snafu is redundant,

I think you are right, which is why I would want to continue to allow people to remove it.

Sounds good!

So what's next?

shepmaster commented 3 years ago

So what's next?

Do you have any specific code changes you want to make before merging? Otherwise, we can transition out of WIP state.

Can you list out all the known issues/missing features/future work?

We’ll need to document this in the guide somehow. I tend to have a heavy hand in editing that to maintain consistent tone, but if you’d like to produce the first few drafts, that’d be a huge help. That doesn’t have to be this PR though.

shepmaster commented 3 years ago

I hope to spend some time on this in the next week or so. If you have a chance to address the last comment, that'd be great. Otherwise, I'll see what I can do.

SamWilsn commented 3 years ago

I'll try and take a look before next week, but I've been a bit swamped. Sorry :sweat_smile:

shepmaster commented 3 years ago

I've been a bit swamped.

you and me both; apologies appreciated but not needed!

netlify[bot] commented 3 years ago

✔️ Deploy Preview for shepmaster-snafu ready!

🔨 Explore the source changes: 0978fa1c6435312ab5eb255b32defe4045d6866e

🔍 Inspect the deploy log: https://app.netlify.com/sites/shepmaster-snafu/deploys/619019b31b080f0008e4c406

😎 Browse the preview: https://deploy-preview-295--shepmaster-snafu.netlify.app

shepmaster commented 3 years ago

Docs at https://deploy-preview-295--shepmaster-snafu.netlify.app/snafu/derive.snafu#placing-context-selectors-in-modules

shepmaster commented 2 years ago

OK, made a small push and behavior change. Now the defaults are to generate a private module and use pub(super) visibility for the selectors inside the module. This prevents some warnings ("private type FooSnafu in public interface") and avoids the module becoming publicly visible and documented (!).

shepmaster commented 2 years ago

Thanks! Published in 0.7.0-beta.2. Getting really close!

shepmaster / snafu

Implement #[snafu(module)] support #295