internal: add tuple struct support in pin_data and pin_init!

[mqqz]

Extend pin_data and pin_init! to support tuple struct syntax.

pin_data (internal/src/pin_data.rs):

• add tuple-field handling via a refactored FieldInfo struct
• generate projections and pin-data accessors for unnamed members

pin_init! (internal/src/init.rs):

• parse initialiser keys as syn::Member (named or tuple index)
• extend parser to support both:
  • tuple-like syntax e.g. Foo(a, <- b, c)
  • and brace syntax e.g. Foo{0 : a, 1 <- b, 2: c} (No longer accepted).
  • Couldn't decide on appropriate syntax, I'm not sure if keeping both is a good idea

Testing:

• tests/tuple_struct.rs
• tests/ui/compile-fail/tuple_{duplicate,invalid,missing}_field.rs
• tests/ui/compile-fail/tuple_shorthand.rs
• tests/ui/expand/pin_{data,init}

• more that got added later

Note that currently for tuple struct internal fields identifiers like _0 (for the first index) which clippy doesn't like. I'm not sure whether it's better to change naming or add #[allow(clippy::just_underscores_and_digits)] (I don't want to go through the effort of spamming clippy and changing all expanded tests just yet). I added relevant clippy suppression so no lints come up now in user code.

Closes: #85

[BennoLossin]

Hi! Thanks for the PR!

I have several suggestions and some bigger things, but overall very happy with your changes & the style of code you used.

I really like the idea of adding FieldInfo before doing the changes to support tuple structs. I think we can take it a bit further and clean up a lot of the pin_data.rs code by moving code generation & handling pinned/unpinned fields in that type instead of the generation functions. Do you mind doing that work in the first commit as well?

Regarding syntax in the tuple form, I think it's fine to only support { 0 <- init } at the moment. I have thought about removing the <- syntax altogether (#66) & always expecting an initializer, but there are some drawbacks. So when one writes init!(Struct(a, b, c)), we just treat it as init!(Struct { 0: a, 1: b, 2: c }) and if they want to use an initializer, they need the braced version.

You can just add #[allow(clippy::just_underscores_and_digits)] to the generated code, that lint isn't useful for our code, since it's macro-generated.

We also should think about if the pin-projected version of a tuple struct should also be a tuple struct. That makes the code quite a lot more complex.

Lastly, I think we should have some more tests, some things that are missing: generics and !Unpin types with #[pin].

[mqqz]

Thanks for the review! I appreciate you taking the time to look at my work. I have looked at the comments and pushed changes that should hopefully address most of them.

I think we can take it a bit further and clean up a lot of the pin_data.rs code by moving code generation & handling pinned/unpinned fields in that type instead of the generation functions. Do you mind doing that work in the first commit as well?

I heavily refactored pin_data.rs and extracted code much of the relevant code into FieldInfo methods.

You can just add #[allow(clippy::just_underscores_and_digits)] to the generated code, that lint isn't useful for our code, since it's macro-generated.

Done. I made sure the generated code doesn't raise any lint warnings.

We also should think about if the pin-projected version of a tuple struct should also be a tuple struct. That makes the code quite a lot more complex.

I am unsure of how much value that would add apart from aesthetics/ergonomics. I would not bother it just yet as the churn might not justify it unless an apparent user need surfaces later. I would focus on having a safe + sound implementation and adding separate codegen paths for named vs tuple projected types makes it harder.

Lastly, I think we should have some more tests, some things that are missing: generics and !Unpin types with #[pin].

I added a new commit that expands the testing coverage with more comprehensive and meaningful test cases (incl. (const) generics, lifetimes and !Unpin types as requested).

Also, during testing I found that

#[pin_data]
struct Tuple<T>(T, #[pin] #[pin] i32);

compiles fine and treats it as one #[pin], (I personally think it should panic) but this isn't a pressing issue. I can address this or leave it as is, whichever option makes more sense for you.

best,
~mo

[BennoLossin]

Just looked at the first commit. I like it much better like this, thanks a lot for doing this work. I have some suggestions & small adjustments, but overall this looks good. (will do the other commits separately)

[BennoLossin]

Two things for the second commit, we don't need the TupleStruct(<- value, other) syntax and one other minor thing.

[BennoLossin]

Greatly appreciated the updated tests!

[mqqz]

rebased and removed feature gates from tests since MSRV was bumped and I have nothing better to do atm 😀

[nbdd0121]

Hi @mqqz, I have a WIP cleanup/refactoring which would simplify and remove a lot of the code that you have been refactoring here, so unfortunately I wouldn't be able to take this PR as is. If you could implement the change without significant refactoring then I would recommend that, or otherwise please wait a bit and come back once I have landed cleanups. Thanks!

[mqqz]

No worries and no rush at all either!

I'll see if I can integrate these changes when your refactor lands. Thanks

[nbdd0121]

(Also I think this doesn't work when some of the fields are #[cfg] out)