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

Visibility of inner struct depends on visibility of field #11

Open
huuff opened this issue Jan 30, 2025 · 11 comments
Open

Visibility of inner struct depends on visibility of field #11

huuff opened this issue Jan 30, 2025 · 11 comments

Comments

@huuff
Copy link

huuff commented Jan 30, 2025

I was writing this small snippet

nestify::nest! {
    #[derive(Debug, Clone)]*
    pub struct Purchase(Vec<pub struct TripPurchase {
        pub price: SupplierTripPrice,
        pub booking_references: References,
        pub ticket_numbers: References,
    }>);
}

And found that I can't access the TripPurchase struct externally, it's private. However if I make the field pub, I can:

nestify::nest! {
    #[derive(Debug, Clone)]*
    pub struct Purchase(pub Vec<pub struct TripPurchase {
        pub price: SupplierTripPrice,
        pub booking_references: References,
        pub ticket_numbers: References,
    }>);
}

Is this expected? It feels weird to me since it means the inner pub is mostly useless. Also, I'd say my first snippet is desirable for many patterns, for example if I want to hide the structure to force the use of a constructor to ensure invariants are held.

If this is not actually expected and a bug, I may be able to lend a hand to fix it.

@snowfoxsh
Copy link
Owner

i think that falls into unintended behavior category but am not entirely sure. i will have to preform some tests. thanks for being patient :).

if it does fall under unintended behavior then i would be delighted to accept your help

@snowfoxsh
Copy link
Owner

So I looked into it and currently it is expected behavior. Tuple Structs must have a field vis, The nested structure also must also have a visibility.

Consider a case where you need a vis structure like this.

pub struct Child;

pub(crate) struct Parent(pub(crate) Vec<Child>);

// aka

```rust
nestify::nest! {
  pub(crate) struct Parent(pub(crate) Vec<pub struct Child>)
}

When defining a regular tuple struct i had to make a compromise with the syntax. I could not figure out a way to create a "native" like syntax without having to share the vis across both the field and the child type def.

nestify::nest! {
  pub(crate) struct Parent(pub struct Child)
}

@huuff
Copy link
Author

huuff commented Feb 8, 2025

Thanks for taking the time for looking into this! That's a pretty interesting corner case, and you're right, I don't think there's any way to cleanly solve this other than making up different syntax.

Do you think it's worth documenting it, just in case someone else bumps into it? I can write a quick paragraph for the readme if you think so.

@huuff
Copy link
Author

huuff commented Feb 8, 2025

Alternatively... would you be comfortable with introducing a non-native-lke syntax? Something like pub(create) struct Parent(pub/ struct Child) or something similar for declaring that the visibility identifier only applies to the struct, not the field?

I don't really like that pub/ syntax honestly, just asking whether there's room for introducing some syntax for this case

@snowfoxsh
Copy link
Owner

Writing a section in the documentation would be helpful I think. It should explain the tuple struct limitation, ie) pub(crate) struct Parent(pub struct Child) where you cannot specify a separate visibility for the "Child" struct.

Perhaps an empty generic could be used.

nestify::nest! {
  pub(super) struct Parent(pub <pub (crate) struct Child>)
}

I think this is a pretty small edge case though to need a very specific visibility structure like this.

@snowfoxsh
Copy link
Owner

I don't really like that pub/ syntax honestly, just asking whether there's room for introducing some syntax for this case

I would rather not introduce too much new syntax. One of the goals of nestify is for someone who has never seen it before to be able to reasonably guess what is going on.

@snowfoxsh
Copy link
Owner

snowfoxsh commented Feb 14, 2025

I think that its time to add #12/#7. Would you like to take a look at it or would you like me? If you would like to make a contribution beyond just the documentation this would probably be a good thing to tackle.

@huuff
Copy link
Author

huuff commented Feb 17, 2025

Thanks for thinking of me! I don't have a lot of spare time, at least for the next couple of weeks, but if you don't mind me looking into it by then, I'd love to.

I'll also get a PR rolling for the documentation on this limitation.

I'm not sure about how #7 relates to this issue... maybe you're thinking of something like:

nestify::nest! {
  pub struct Parent(>pub struct Child)
}

Which is consistent (although it doesn't look very nice) with the syntax for field attributes (#>[attr]). I suppose with pub* also pub/ and pub- and >pub come along?

@snowfoxsh
Copy link
Owner

sorry about my haphazard issue management haha. this is my first real GitHub project.

@huuff
Copy link
Author

huuff commented Feb 27, 2025

Actually, I'd like to take a deeper look into this issue first, since I've been bit by it a few times!

If I understood right, the main issue is when the inner type is directly a declaration, which as you said, I don't think it's solvable without new syntax... but what about cases when the inner type not a declaration, but may contain one? For example

nestify::nest! {
  pub struct Purchase(Vec<pub struct Inner>);
}

I'd say there's no ambiguity here against

nestify::nest! {
  pub struct Purchase(pub Vec<pub struct Inner>);
}

Maybe I'm being naive, but some sort of lookahead at the visibility token could tell us whether it's followed by struct or enum (thus a declaration) or just the visibility for the inner field.

Would you be fine with me trying to put up a PR to that effect?

@snowfoxsh
Copy link
Owner

snowfoxsh commented Feb 27, 2025

Yes i believe that is correct. I the vast majority of the time there would be no ambiguity. I would happily accept a PR. Please allow the user to directly specify both visibilities if they want to as to not be a breaking change.

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

No branches or pull requests

2 participants