Implement #[snafu(module)] support #295

SamWilsn · 2021-07-06T05:23:43Z

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 Change the default context selector suffix to Snafu #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 · 2021-07-06T13:13:43Z

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 · 2021-07-06T15:19:17Z

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 · 2021-07-07T01:31:27Z

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 · 2021-07-07T01:53:55Z

like shadowing ContextSelector

Hmm, where do I do that?

snafu/snafu-derive/src/lib.rs

Line 1251 in 42bcc33

use crate::shared::ContextSelector;

...

Thanks!

shepmaster

First pass through. It seems to be looking good; a few questions / comments.

snafu-derive/Cargo.toml

snafu-derive/src/lib.rs

snafu-derive/src/parse.rs

shepmaster · 2021-07-07T12:57:16Z

tests/module.rs

+    pub struct Dummy0;
+
+    #[derive(Debug)]
+    pub struct Dummy1;


What value do the multiple dummy structs all at the same level bring? I'd have expected to see them scattered across different modules (and then imported in interesting ways in the error enum) to exercise how the importing works.

Since the variants all have the same name, I wanted to make sure the generated selectors had the correct type in them. Probably overkill though, so I can remove them all if you like.

Since the variants all have the same name

This is actually the biggest thing to me that I hadn't even quite realized before today! I've not run into that specific problem, but I've heard of some other people who had.

Oh god, it's half of my motivation for this PR. I'll have a constructor that can fail in A, B, C ways, then the rest of the methods can fail in A, D, E, F ways, and the A context struct will conflict.

tests/module.rs

shepmaster

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).

I confess to being a bit surprised that there aren't any test failures here. Perhaps the use super::* is enough for our purposes? For example, this is consistent and works:

mod alpha {
    pub struct A;
}

mod gen {
    use super::*;

    fn x(_: alpha::A) {}
}

Do you have any test cases up your sleeve where the current implementation might fail?

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"? 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?

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?

I expect that there is some value in it, but maybe it doesn't need to be immediately implemented.

Some ideal would be if we could compute the "minimum" of all of the visibilities of the selectors, but that seems like an overly clever and brittle idea...

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

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. This means that people would be able to easily switch to using a module with a use the_name::*.

The "good" news is that once someone is using the #[snafu] attribute, adding a bit more to disable the suffix shouldn't be too painful. 🤞

I'm not against changing it in a future version, though, based on feedback.

shepmaster · 2021-07-19T23:37:26Z

tests/module.rs

+    fn can_set_module_visibility_restricted() {
+        let _ = self::child::restricted_error::VariantSnafu { v: Dummy3 }.build();
+    }


The visibility tests would benefit from negative test case(s) in the compatibility-tests/compile-fail/ directory. Let me know if you need help with that.

shepmaster · 2021-07-19T23:39:11Z

tests/module.rs

+    pub struct Dummy0;
+
+    #[derive(Debug)]
+    pub struct Dummy1;


Since the variants all have the same name

This is actually the biggest thing to me that I hadn't even quite realized before today! I've not run into that specific problem, but I've heard of some other people who had.

shepmaster · 2021-07-20T00:18:51Z

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

SamWilsn · 2021-07-20T00:49:14Z

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 · 2021-08-09T00:28:32Z

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

shepmaster · 2021-08-21T00:21:36Z

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, 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 · 2021-08-21T17:49:48Z

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, 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 · 2021-08-21T20:34:20Z

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 · 2021-09-15T23:39:38Z

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 · 2021-09-16T00:42:01Z

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

shepmaster · 2021-09-16T01:12:46Z

I've been a bit swamped.

you and me both; apologies appreciated but not needed!

netlify · 2021-09-29T01:24:34Z

✔️ Deploy Preview for shepmaster-snafu ready!

🔨 Explore the source changes: 0978fa1

🔍 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 · 2021-10-23T14:33:10Z

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

shepmaster · 2021-11-13T19:16:18Z

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 (!).

The new attribute `module` makes the derive macro put all the context selectors for structs and enums in a module. Closes shepmaster#188

shepmaster · 2021-11-15T00:41:34Z

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

shepmaster reviewed Jul 7, 2021

View reviewed changes

SamWilsn force-pushed the derive-with-module branch from 6e0411e to c099bff Compare July 7, 2021 20:47

SamWilsn requested a review from shepmaster July 13, 2021 22:09

shepmaster reviewed Jul 20, 2021

View reviewed changes

shepmaster force-pushed the derive-with-module branch from 15cc66b to 97b87b4 Compare September 15, 2021 23:37

shepmaster changed the title ~~WiP: Implement #[snafu(module)] support (#188)~~ Implement #[snafu(module)] support (#188) Sep 15, 2021

shepmaster changed the title ~~Implement #[snafu(module)] support (#188)~~ Implement #[snafu(module)] support Sep 15, 2021

shepmaster linked an issue Sep 15, 2021 that may be closed by this pull request

Option to put generated context selectors into a submodule #188

Closed

shepmaster force-pushed the derive-with-module branch from 97b87b4 to 54d1881 Compare September 29, 2021 01:24

shepmaster force-pushed the derive-with-module branch from 54d1881 to 222447b Compare October 23, 2021 14:02

shepmaster force-pushed the derive-with-module branch from 222447b to 24d12bf Compare November 13, 2021 19:13

shepmaster marked this pull request as ready for review November 13, 2021 19:16

Implement #[snafu(module)] support

0978fa1

The new attribute `module` makes the derive macro put all the context selectors for structs and enums in a module. Closes shepmaster#188

shepmaster force-pushed the derive-with-module branch from 18255ad to 0978fa1 Compare November 13, 2021 20:01

shepmaster approved these changes Nov 15, 2021

View reviewed changes

shepmaster merged commit e846c56 into shepmaster:main Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #[snafu(module)] support #295

Implement #[snafu(module)] support #295

SamWilsn commented Jul 6, 2021

shepmaster commented Jul 6, 2021

SamWilsn commented Jul 6, 2021

shepmaster commented Jul 7, 2021

SamWilsn commented Jul 7, 2021

shepmaster left a comment

shepmaster Jul 7, 2021

SamWilsn Jul 7, 2021

shepmaster Jul 19, 2021

SamWilsn Jul 20, 2021

shepmaster left a comment

shepmaster Jul 19, 2021

shepmaster Jul 19, 2021

shepmaster commented Jul 20, 2021

SamWilsn commented Jul 20, 2021

SamWilsn commented Aug 9, 2021

shepmaster commented Aug 21, 2021 •

edited

SamWilsn commented Aug 21, 2021

shepmaster commented Aug 21, 2021

shepmaster commented Sep 15, 2021

SamWilsn commented Sep 16, 2021

shepmaster commented Sep 16, 2021

netlify bot commented Sep 29, 2021 •

edited

shepmaster commented Oct 23, 2021

shepmaster commented Nov 13, 2021

shepmaster commented Nov 15, 2021

Implement #[snafu(module)] support #295

Implement #[snafu(module)] support #295

Conversation

SamWilsn commented Jul 6, 2021

shepmaster commented Jul 6, 2021

SamWilsn commented Jul 6, 2021

shepmaster commented Jul 7, 2021

SamWilsn commented Jul 7, 2021

shepmaster left a comment

Choose a reason for hiding this comment

shepmaster Jul 7, 2021

Choose a reason for hiding this comment

SamWilsn Jul 7, 2021

Choose a reason for hiding this comment

shepmaster Jul 19, 2021

Choose a reason for hiding this comment

SamWilsn Jul 20, 2021

Choose a reason for hiding this comment

shepmaster left a comment

Choose a reason for hiding this comment

shepmaster Jul 19, 2021

Choose a reason for hiding this comment

shepmaster Jul 19, 2021

Choose a reason for hiding this comment

shepmaster commented Jul 20, 2021

SamWilsn commented Jul 20, 2021

SamWilsn commented Aug 9, 2021

shepmaster commented Aug 21, 2021 • edited

SamWilsn commented Aug 21, 2021

shepmaster commented Aug 21, 2021

shepmaster commented Sep 15, 2021

SamWilsn commented Sep 16, 2021

shepmaster commented Sep 16, 2021

netlify bot commented Sep 29, 2021 • edited

shepmaster commented Oct 23, 2021

shepmaster commented Nov 13, 2021

shepmaster commented Nov 15, 2021

shepmaster commented Aug 21, 2021 •

edited

netlify bot commented Sep 29, 2021 •

edited