Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds support for specifying re-export visibility #24

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

colstrom
Copy link

Thanks for your time and effort on this crate! Your contributions to the Rust ecosystem are valuable and appreciated. ❤️

This resolves #16 and #22.

All credit for the code goes to @tlowerison, all I'm doing here is proposing that their changes be merged upstream (and bumping the version accordingly), since I'm actively using this version. 😁

If there's anything you'd like me to change, feel free to let me know!

Thanks for considering these changes.

Copy link
Owner

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to the both of you! 🙏

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
Comment on lines 446 to 453
(#[derive($($name_tt:tt)+)] = #[derive($($derives:tt)*)]; $($tt:tt)+) => {
$crate::derive_alias!(#[derive($($name_tt)+)] = #[derive($($derives)*)];);
$crate::derive_alias!($($tt)+);
};
(pub #[derive($($name_tt:tt)+)] = #[derive($($derives:tt)*)]; $($tt:tt)+) => {
$crate::derive_alias!(pub #[derive($($name_tt)+)] = #[derive($($derives)*)];);
$crate::derive_alias!($($tt)+);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid syntax duplication (e.g., having to handle doc strings here), let's merge these rules with the parent ones.

For instance, the first rule would become:

macro_rules! derive_alias {
    (
        #[derive($MacroName:ident !)] = #[derive($($derives:tt)*)];
        $($($rest:tt)+)?
    ) => (
        /* current output for `$MacroName` */

        $(
            $crate::derive_alias! { $($rest)+ }
        )?
    );
  • also, as a general rule of thumb, nested macro invocation in trailing position are better done using {} rather than ();, just in case the last one expands to as statement expression (e.g., { 42 }), so as to avoid discarding its value (e.g., the 42: i32).

Copy link
Author

@colstrom colstrom Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand what you mean here, in two specific cases:

When you say "let's merge these rules with the parent ones", what is "parent" referring to in this context? Is it positional, like "the rule above this one", or more like "the rule that calls this rule", or something else entirely?

The {} vs () comment I'm even less clear on. I'm not quite sure what doesn't click here for me, but I'll lay out what does make sense and with a bit of luck, you might see whatever it is that I'm not seeing. 🤔

I'm reasonably sure of the terms here, understanding Items to be distinct from Statements and Expressions, and in the context of macros, these map to the fragment-specifier bits in the documentation for Metavariables, right? So we're matching those at the macro entry point (matcher, but "matching in the matcher" sounds awkward) and then forwarding that on to another macro in the transcriber section? So the docs on Forwarding a matched fragment have this bit that seems relevant:

When forwarding a matched fragment to another macro-by-example, matchers in the second macro will see an opaque AST of the fragment type. The second macro can’t use literal tokens to match the fragments in the matcher, only a fragment specifier of the same type. The ident, lifetime, and tt fragment types are an exception, and can be matched by literal tokens.

And I'm somewhat fuzzy on this part, but I think that suggests that if we matched a tt, it could be kind of anything, but if we matched something more specific, like a stmt, item, or expr, it can only be forwarded to something that accepts those. So it's anything until we treat it as something, and from then on it's only that thing. And since we want to match all of these things, we're not being specific, and just matching some number of tt and forwarding them on blindly, right? Which is why we don't know if we have a stmt, item, expr, or something else.

If I've got that right, then I think I understand the context for why the (potential) problem occurs.

But the differences between {} and () still aren't clear to me at this point.

Looking up some references to figure it out led me to this chapter in The Little Book of Rust Macros, which includes this sentence that I think explains the difference?

In fact, function-like macros can be invoked with any kind of parentheses as well, but invocations with { .. } and ( ... );, notice the trailing semicolon, are special in that their expansion will always be parsed as an item.

My brain parses that as { .. } and ( ... ); always parse as an item, but [ ... ] could be something else (like a stmt or expr?).

Which seems wrong, because you're suggesting a change between two types of parentheses that I (likely incorrectly) think do the same thing?

So... yeah. I'd like to understand this, I've read the docs (not exhaustively), heck, I've even used this stuff a decent amount, and it (on the surface at least) works the way I expect it to, so I think I'm probably not too far off.

I know that's a wall of text, and I appreciate that it takes time and effort to wade through that. If you happen to notice where my blind spots are, I'd be grateful for the insight, and if I'm misunderstanding something important, I'd welcome any corrections.

Thanks for your patience!

src/lib.rs Outdated
Comment on lines 498 to 499
macro_rules! attribute_alias {
(#[apply($name:ident $(!)?)] = $( #[$($attrs:tt)*] )+;) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here (also, whitespace is very important for macro readability, let's avoid squashing things together)

Cargo.toml Show resolved Hide resolved
Co-authored-by: Daniel Henry-Mantilla <[email protected]>
@colstrom
Copy link
Author

Excellent, thank you. I'm working my way through the suggested changes. My intuition around macros isn't as robust as I'd like it to be, so feedback like this helps me better understand how others think about these things. ^_^

@danielhenrymantilla
Copy link
Owner

(forgot to circle back to this, I'll try to do so within the week; do ping me if I don't!)

@colstrom
Copy link
Author

colstrom commented Oct 1, 2024

(forgot to circle back to this, I'll try to do so within the week; do ping me if I don't!)

Sounds good. I appreciate that this -- like many (or even most?) OSS projects -- is a volunteer effort, and that maintainers have other responsibilities and personal lives and stuff. What time you can spare, when you can, is appreciated.

No stress on timeline or anything, rust/cargo makes it very easy to depend on forks and PRs and such.

The end goal is to just make sure that useful changes make their way upstream, eventually. ^_^

@colstrom
Copy link
Author

colstrom commented Oct 3, 2024

Apologies for the large diff in the last change, it's easier to read with git diff --ignore-all-space (or with "Hide Whitespace" option selected in the Diff View options if reviewing through the GitHub UI).

Hopefully this makes code review(s) more comfortable, since each level of nesting is explicit, and the indentation is normalized to 2 spaces (since the macros are quite nested), making it easier to scan and match braces visually, given that some of the suggested changes are directly relating to that stuff.

Reading through the macros after the change, it's much easier for me to spot the differences between them now, and it feels like maybe there would be a way for one to just... call the other? Initially I thought replacing pub #[apply(... with $vis:vis #[apply(... in the one matcher and then having the other (the one without pub) call it with pub(in crate), but apparently that's not allowed because vis fragments can only be followed by ident or ty, and not #. 🤔

Even if that worked, there's still the difference of the #[macro_export] in the one transcriber but not the other, and I don't (yet!) understand how to omit that in a declarative macro, assuming it's possible to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

attribute_alias! (and derive_alias!) don't do public visibility
3 participants