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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.

[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]structTuple<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)

[nbdd0121]

The big refactors have landed. If you could rebase and perhaps reduce the amount of code motion (i.e. only extract needed info to FieldInfo and perhaps not create methods yet) then I could take another look.

[mqqz]

This PR is too fat and outdated to be reviewable. Plus, I think switching to using a projected tuple struct as well is a better approach (to address Gary's cfg concern).

Hence, I'm closing and replacing with a newer PR #155.