[1/3] rust: macros: `parse_generics` add `decl_generics`
Commit Message
When parsing generics of a type definition, default values can be
specified. This syntax is however only available on type definitions and
not e.g. impl blocks.
This patch adds the `decl_generics` which can only be used on type
defintions, since they contain the default values for generic
parameters. This patch also changes how `impl_generics` are made up, as
these should be used with `impl<$impl_generics>`, they will omit the
default values.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/macros/helpers.rs | 91 +++++++++++++++++++++++++++--------------
rust/macros/pin_data.rs | 1 +
rust/macros/zeroable.rs | 1 +
3 files changed, 63 insertions(+), 30 deletions(-)
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
Comments
Sorry, just went through my eyes, hope you don't mind I nitpick
a bit. And maybe learn a bit in the process.
On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
> When parsing generics of a type definition, default values can be
> specified. This syntax is however only available on type definitions
> and
> not e.g. impl blocks.
Is "impl block" equivalent to a trait implementation? Maybe then just
write in better English "trait implementation? Would be IMHO better
to use commonly know terminology here.
Also for any commit, including any Rust commit. "When parsing" does
not map to anything concrete. There always should be a concrete
scenario where the parser its used. Especially since Rust is a new
thing in the kernel, these commits should really have more in-depth
information of the context.
I neither really grasped why trait implementations (if that is meant
by "impl block") not having this support connects to the code change.
Maybe just say that this patch adds the support and drop the whole
story about traits. It is sort of unnecessary context.
Finally, why this change is needed? Any commit should have existential
reason why it exists. So what will happen if "decl_generics" is not
taken to the upstream kernel? How does it make life more difficult?
You should be able to answer to this (in the commit message).
> This patch adds the `decl_generics` which can only be used on type
> defintions, since they contain the default values for generic
~~~~~~~~~~
definitions.
> parameters. This patch also changes how `impl_generics` are made up,
> as
> these should be used with `impl<$impl_generics>`, they will omit the
> default values.
What is decl_generics and what are the other _generics variables?
This lacks explanation what sort of change is implemented and why.
Nit:
s/This patch//:
1. "Add..." and
2. "Change...".
It is now useless pair of words and in the commit log "patch" is an
invalid word, given that it is a commit then, not a patch.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> rust/macros/helpers.rs | 91 +++++++++++++++++++++++++++------------
> --
> rust/macros/pin_data.rs | 1 +
> rust/macros/zeroable.rs | 1 +
> 3 files changed, 63 insertions(+), 30 deletions(-)
>
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index afb0f2e3a36a..36fecdd998d0 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
>
> -use proc_macro::{token_stream, Group, Punct, Spacing, TokenStream,
> TokenTree};
> +use proc_macro::{token_stream, Group, TokenStream, TokenTree};
>
> pub(crate) fn try_ident(it: &mut token_stream::IntoIter) ->
> Option<String> {
> if let Some(TokenTree::Ident(ident)) = it.next() {
> @@ -72,6 +72,7 @@ pub(crate) fn expect_end(it: &mut
> token_stream::IntoIter) {
>
> pub(crate) struct Generics {
> pub(crate) impl_generics: Vec<TokenTree>,
> + pub(crate) decl_generics: Vec<TokenTree>,
> pub(crate) ty_generics: Vec<TokenTree>,
> }
>
> @@ -81,6 +82,8 @@ pub(crate) struct Generics {
> pub(crate) fn parse_generics(input: TokenStream) -> (Generics,
> Vec<TokenTree>) {
> // `impl_generics`, the declared generics with their bounds.
> let mut impl_generics = vec![];
> + // The generics with bounds and default values.
> + let mut decl_generics = vec![];
> // Only the names of the generics, without any bounds.
> let mut ty_generics = vec![];
> // Tokens not related to the generics e.g. the `where` token and
> definition.
> @@ -90,10 +93,17 @@ pub(crate) fn parse_generics(input: TokenStream)
> -> (Generics, Vec<TokenTree>) {
> let mut toks = input.into_iter();
> // If we are at the beginning of a generic parameter.
> let mut at_start = true;
> - for tt in &mut toks {
> + let mut skip_until_comma = false;
> + while let Some(tt) = toks.next() {
> + if nesting == 1 && matches!(&tt, TokenTree::Punct(p) if
> p.as_char() == '>') {
> + // Found the end of the generics.
> + break;
> + } else if nesting >= 1 {
> + decl_generics.push(tt.clone());
> + }
> match tt.clone() {
> TokenTree::Punct(p) if p.as_char() == '<' => {
> - if nesting >= 1 {
> + if nesting >= 1 && !skip_until_comma {
> // This is inside of the generics and part of
> some bound.
> impl_generics.push(tt);
> }
> @@ -105,49 +115,70 @@ pub(crate) fn parse_generics(input:
> TokenStream) -> (Generics, Vec<TokenTree>) {
> break;
> } else {
> nesting -= 1;
> - if nesting >= 1 {
> + if nesting >= 1 && !skip_until_comma {
> // We are still inside of the generics and
> part of some bound.
> impl_generics.push(tt);
> }
> - if nesting == 0 {
> - break;
> - }
> }
> }
> - tt => {
> + TokenTree::Punct(p) if skip_until_comma && p.as_char()
> == ',' => {
> if nesting == 1 {
> - // Here depending on the token, it might be a
> generic variable name.
> - match &tt {
> - // Ignore const.
> - TokenTree::Ident(i) if i.to_string() ==
> "const" => {}
> - TokenTree::Ident(_) if at_start => {
> - ty_generics.push(tt.clone());
> - // We also already push the `,` token,
> this makes it easier to append
> - // generics.
> -
> ty_generics.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
> - at_start = false;
> - }
> - TokenTree::Punct(p) if p.as_char() == ',' =>
> at_start = true,
> - // Lifetimes begin with `'`.
> - TokenTree::Punct(p) if p.as_char() == '\''
> && at_start => {
> - ty_generics.push(tt.clone());
> - }
> - _ => {}
> - }
> + impl_generics.push(TokenTree::Punct(p.clone()));
> + ty_generics.push(TokenTree::Punct(p));
> + skip_until_comma = false;
> }
> - if nesting >= 1 {
> - impl_generics.push(tt);
> - } else if nesting == 0 {
> + }
> + tt if !skip_until_comma => {
> + match nesting {
> // If we haven't entered the generics yet, we
> still want to keep these tokens.
> - rest.push(tt);
> + 0 => rest.push(tt),
> + 1 => {
> + // Here depending on the token, it might be
> a generic variable name.
> + match tt {
> + TokenTree::Ident(i) if at_start &&
> i.to_string() == "const" => {
> + let Some(name) = toks.next() else {
> + // Parsing error.
> + break;
> + };
> +
> impl_generics.push(TokenTree::Ident(i));
> + impl_generics.push(name.clone());
> + ty_generics.push(name.clone());
> + decl_generics.push(name);
> + at_start = false;
> + }
> + tt @ TokenTree::Ident(_) if at_start =>
> {
> + impl_generics.push(tt.clone());
> + ty_generics.push(tt);
> + at_start = false;
> + }
> + TokenTree::Punct(p) if p.as_char() ==
> ',' => {
> +
> impl_generics.push(TokenTree::Punct(p.clone()));
> +
> ty_generics.push(TokenTree::Punct(p));
> + at_start = true;
> + }
> + // Lifetimes begin with `'`.
> + TokenTree::Punct(p) if p.as_char() ==
> '\'' && at_start => {
> +
> ty_generics.push(TokenTree::Punct(p.clone()));
> +
> impl_generics.push(TokenTree::Punct(p));
> + }
> + // Generics can have default values, we
> skip these.
> + TokenTree::Punct(p) if p.as_char() ==
> '=' => {
> + skip_until_comma = true;
> + }
> + tt => impl_generics.push(tt),
> + }
> + }
> + _ => impl_generics.push(tt),
> }
> }
> + _ => {}
> }
> }
> rest.extend(toks);
> (
> Generics {
> impl_generics,
> + decl_generics,
> ty_generics,
> },
> rest,
> diff --git a/rust/macros/pin_data.rs b/rust/macros/pin_data.rs
> index 6d58cfda9872..022e68e9720d 100644
> --- a/rust/macros/pin_data.rs
> +++ b/rust/macros/pin_data.rs
> @@ -10,6 +10,7 @@ pub(crate) fn pin_data(args: TokenStream, input:
> TokenStream) -> TokenStream {
> let (
> Generics {
> impl_generics,
> + decl_generics: _,
> ty_generics,
> },
> rest,
> diff --git a/rust/macros/zeroable.rs b/rust/macros/zeroable.rs
> index 0d605c46ab3b..cfee2cec18d5 100644
> --- a/rust/macros/zeroable.rs
> +++ b/rust/macros/zeroable.rs
> @@ -7,6 +7,7 @@ pub(crate) fn derive(input: TokenStream) ->
> TokenStream {
> let (
> Generics {
> impl_generics,
> + decl_generics: _,
> ty_generics,
> },
> mut rest,
>
> base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
BR, Jarkko
On 25.11.23 14:39, Jarkko Sakkinen wrote:
> Sorry, just went through my eyes, hope you don't mind I nitpick
> a bit. And maybe learn a bit in the process.
>
> On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
>> When parsing generics of a type definition, default values can be
>> specified. This syntax is however only available on type definitions
>> and
>> not e.g. impl blocks.
>
> Is "impl block" equivalent to a trait implementation? Maybe then just
> write in better English "trait implementation? Would be IMHO better
> to use commonly know terminology here.
"impl block" refers to the syntactic item of Implementation [1]. It
might be a trait implementation, or an inherent implementation. To me
"impl block" is known terminology.
[1]: https://doc.rust-lang.org/stable/reference/items/implementations.html
> Also for any commit, including any Rust commit. "When parsing" does
> not map to anything concrete. There always should be a concrete
> scenario where the parser its used. Especially since Rust is a new
> thing in the kernel, these commits should really have more in-depth
> information of the context.
This commit is tagged `rust: macros:`, which means that it affects the
proc macros. So when I wrote "When parsing", I meant "When parsing Rust
code in proc macros". I will change this for v2.
> I neither really grasped why trait implementations (if that is meant
> by "impl block") not having this support connects to the code change.
> Maybe just say that this patch adds the support and drop the whole
> story about traits. It is sort of unnecessary context.
Rust does not syntactically support writing
impl<const N: usize = 0> Foo<N> {
}
This is because it does not make sense. The syntax `= 0` only makes
sense on type definitions:
struct Foo<const N: usize = 0> {
}
Because then you can just write `Foo` and it will be the same type as
`Foo<0>`.
> Finally, why this change is needed? Any commit should have existential
> reason why it exists. So what will happen if "decl_generics" is not
> taken to the upstream kernel? How does it make life more difficult?
> You should be able to answer to this (in the commit message).
Does this explain it?:
In order to allow `#[pin_data]` on structs with default values for const
generic parameters, the `#[pin_data]` macro needs to parse them and have
access to the generics as they are written on the type definition.
This commit adds support for parsing them to the already present generics
parsing code in the macros crate.
>> parameters. This patch also changes how `impl_generics` are made up,
>> as
>> these should be used with `impl<$impl_generics>`, they will omit the
>> default values.
>
> What is decl_generics and what are the other _generics variables?
> This lacks explanation what sort of change is implemented and why.
The terms `impl_generics` and `ty_generics` are taken from [2]. This
patch adds a third kind which also contains any default values of const
generic parameters. I named them `decl_generics`, because they only
appear on type declarations.
[2]: https://docs.rs/syn/latest/syn/struct.Generics.html#method.split_for_impl
On Sat Nov 25, 2023 at 5:39 PM EET, Benno Lossin wrote:
> On 25.11.23 14:39, Jarkko Sakkinen wrote:
> > Sorry, just went through my eyes, hope you don't mind I nitpick
> > a bit. And maybe learn a bit in the process.
> >
> > On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
> >> When parsing generics of a type definition, default values can be
> >> specified. This syntax is however only available on type definitions
> >> and
> >> not e.g. impl blocks.
> >
> > Is "impl block" equivalent to a trait implementation? Maybe then just
> > write in better English "trait implementation? Would be IMHO better
> > to use commonly know terminology here.
>
> "impl block" refers to the syntactic item of Implementation [1]. It
> might be a trait implementation, or an inherent implementation. To me
> "impl block" is known terminology.
>
> [1]: https://doc.rust-lang.org/stable/reference/items/implementations.html
>
> > Also for any commit, including any Rust commit. "When parsing" does
> > not map to anything concrete. There always should be a concrete
> > scenario where the parser its used. Especially since Rust is a new
> > thing in the kernel, these commits should really have more in-depth
> > information of the context.
>
> This commit is tagged `rust: macros:`, which means that it affects the
> proc macros. So when I wrote "When parsing", I meant "When parsing Rust
> code in proc macros". I will change this for v2.
>
> > I neither really grasped why trait implementations (if that is meant
> > by "impl block") not having this support connects to the code change.
> > Maybe just say that this patch adds the support and drop the whole
> > story about traits. It is sort of unnecessary context.
>
> Rust does not syntactically support writing
>
> impl<const N: usize = 0> Foo<N> {
> }
>
> This is because it does not make sense. The syntax `= 0` only makes
> sense on type definitions:
>
> struct Foo<const N: usize = 0> {
> }
>
> Because then you can just write `Foo` and it will be the same type as
> `Foo<0>`.
Right.
>
> > Finally, why this change is needed? Any commit should have existential
> > reason why it exists. So what will happen if "decl_generics" is not
> > taken to the upstream kernel? How does it make life more difficult?
> > You should be able to answer to this (in the commit message).
>
> Does this explain it?:
>
> In order to allow `#[pin_data]` on structs with default values for const
> generic parameters, the `#[pin_data]` macro needs to parse them and have
> access to the generics as they are written on the type definition.
> This commit adds support for parsing them to the already present generics
> parsing code in the macros crate.
Yes.
>
> >> parameters. This patch also changes how `impl_generics` are made up,
> >> as
> >> these should be used with `impl<$impl_generics>`, they will omit the
> >> default values.
> >
> > What is decl_generics and what are the other _generics variables?
> > This lacks explanation what sort of change is implemented and why.
>
> The terms `impl_generics` and `ty_generics` are taken from [2]. This
> patch adds a third kind which also contains any default values of const
> generic parameters. I named them `decl_generics`, because they only
> appear on type declarations.
>
> [2]: https://docs.rs/syn/latest/syn/struct.Generics.html#method.split_for_impl
Thanks a lot of taking time for explaining all these concepts in a such
a detail. Appreciate it! And I apologize for my a bit intrusive
response.
I really think that "more vocal and verbose" than "legacy C" patches
would be a great policy for Rust specific patches. This would help
audience who understand kernel but are not as in Rust to give more
feedback on the patches. I mean tech is still the same whatever we
used to implement the code that enables it.
By doing that I see that all benefit and it opens doors for deeper
Rust integration in the kernel.
BR, Jarkko
On 12/2/23 18:33, Jarkko Sakkinen wrote:
> On Sat Nov 25, 2023 at 5:39 PM EET, Benno Lossin wrote:
>> On 25.11.23 14:39, Jarkko Sakkinen wrote:
>>> On Sat, 2023-11-25 at 12:50 +0000, Benno Lossin wrote:
>>>> parameters. This patch also changes how `impl_generics` are made up,
>>>> as
>>>> these should be used with `impl<$impl_generics>`, they will omit the
>>>> default values.
>>>
>>> What is decl_generics and what are the other _generics variables?
>>> This lacks explanation what sort of change is implemented and why.
>>
>> The terms `impl_generics` and `ty_generics` are taken from [2]. This
>> patch adds a third kind which also contains any default values of const
>> generic parameters. I named them `decl_generics`, because they only
>> appear on type declarations.
>>
>> [2]: https://docs.rs/syn/latest/syn/struct.Generics.html#method.split_for_impl
>
> Thanks a lot of taking time for explaining all these concepts in a such
> a detail. Appreciate it! And I apologize for my a bit intrusive
> response.
No worries!
> I really think that "more vocal and verbose" than "legacy C" patches
> would be a great policy for Rust specific patches. This would help
> audience who understand kernel but are not as in Rust to give more
> feedback on the patches. I mean tech is still the same whatever we
> used to implement the code that enables it.
I agree. One thing that we are already doing is encouraging
documentation, see [1]. We also use `-Wmissing_docs` which makes the
compiler emit a warning when you have a public item (a function,
constant, type etc.) that has no documentation.
Rust documentation is placed directly next to the source code. This
helps keep it up to date and makes it easier to find documentation.
[1]: https://docs.kernel.org/rust/coding-guidelines.html#code-documentation
Additionally we can make commit messages more elaborate. Though we
probably need help with choosing the topics and depth of the
explanations. Since when you are already familiar with something, it
is often difficult to know what is hard to understand. This also holds
for documentation.
That being said, I think that we should not document common Rust
patterns and properties. I think you understand that we do not want
to write code like this over-the-top example:
void init_foo(struct foo *foo)
{
// `foo->value` is short for `(*foo).value`
foo->value = 42;
}
I am of course open to help anyone understand the Rust code that
we upstream, by giving pointers to the relevant Rust resources (e.g.
the Rust book or std library documentation) or explaining our custom
code. We might even find something that should be documented better
and then I will be happy to do so.
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
-use proc_macro::{token_stream, Group, Punct, Spacing, TokenStream, TokenTree};
+use proc_macro::{token_stream, Group, TokenStream, TokenTree};
pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
if let Some(TokenTree::Ident(ident)) = it.next() {
@@ -72,6 +72,7 @@ pub(crate) fn expect_end(it: &mut token_stream::IntoIter) {
pub(crate) struct Generics {
pub(crate) impl_generics: Vec<TokenTree>,
+ pub(crate) decl_generics: Vec<TokenTree>,
pub(crate) ty_generics: Vec<TokenTree>,
}
@@ -81,6 +82,8 @@ pub(crate) struct Generics {
pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
// `impl_generics`, the declared generics with their bounds.
let mut impl_generics = vec![];
+ // The generics with bounds and default values.
+ let mut decl_generics = vec![];
// Only the names of the generics, without any bounds.
let mut ty_generics = vec![];
// Tokens not related to the generics e.g. the `where` token and definition.
@@ -90,10 +93,17 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
let mut toks = input.into_iter();
// If we are at the beginning of a generic parameter.
let mut at_start = true;
- for tt in &mut toks {
+ let mut skip_until_comma = false;
+ while let Some(tt) = toks.next() {
+ if nesting == 1 && matches!(&tt, TokenTree::Punct(p) if p.as_char() == '>') {
+ // Found the end of the generics.
+ break;
+ } else if nesting >= 1 {
+ decl_generics.push(tt.clone());
+ }
match tt.clone() {
TokenTree::Punct(p) if p.as_char() == '<' => {
- if nesting >= 1 {
+ if nesting >= 1 && !skip_until_comma {
// This is inside of the generics and part of some bound.
impl_generics.push(tt);
}
@@ -105,49 +115,70 @@ pub(crate) fn parse_generics(input: TokenStream) -> (Generics, Vec<TokenTree>) {
break;
} else {
nesting -= 1;
- if nesting >= 1 {
+ if nesting >= 1 && !skip_until_comma {
// We are still inside of the generics and part of some bound.
impl_generics.push(tt);
}
- if nesting == 0 {
- break;
- }
}
}
- tt => {
+ TokenTree::Punct(p) if skip_until_comma && p.as_char() == ',' => {
if nesting == 1 {
- // Here depending on the token, it might be a generic variable name.
- match &tt {
- // Ignore const.
- TokenTree::Ident(i) if i.to_string() == "const" => {}
- TokenTree::Ident(_) if at_start => {
- ty_generics.push(tt.clone());
- // We also already push the `,` token, this makes it easier to append
- // generics.
- ty_generics.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
- at_start = false;
- }
- TokenTree::Punct(p) if p.as_char() == ',' => at_start = true,
- // Lifetimes begin with `'`.
- TokenTree::Punct(p) if p.as_char() == '\'' && at_start => {
- ty_generics.push(tt.clone());
- }
- _ => {}
- }
+ impl_generics.push(TokenTree::Punct(p.clone()));
+ ty_generics.push(TokenTree::Punct(p));
+ skip_until_comma = false;
}
- if nesting >= 1 {
- impl_generics.push(tt);
- } else if nesting == 0 {
+ }
+ tt if !skip_until_comma => {
+ match nesting {
// If we haven't entered the generics yet, we still want to keep these tokens.
- rest.push(tt);
+ 0 => rest.push(tt),
+ 1 => {
+ // Here depending on the token, it might be a generic variable name.
+ match tt {
+ TokenTree::Ident(i) if at_start && i.to_string() == "const" => {
+ let Some(name) = toks.next() else {
+ // Parsing error.
+ break;
+ };
+ impl_generics.push(TokenTree::Ident(i));
+ impl_generics.push(name.clone());
+ ty_generics.push(name.clone());
+ decl_generics.push(name);
+ at_start = false;
+ }
+ tt @ TokenTree::Ident(_) if at_start => {
+ impl_generics.push(tt.clone());
+ ty_generics.push(tt);
+ at_start = false;
+ }
+ TokenTree::Punct(p) if p.as_char() == ',' => {
+ impl_generics.push(TokenTree::Punct(p.clone()));
+ ty_generics.push(TokenTree::Punct(p));
+ at_start = true;
+ }
+ // Lifetimes begin with `'`.
+ TokenTree::Punct(p) if p.as_char() == '\'' && at_start => {
+ ty_generics.push(TokenTree::Punct(p.clone()));
+ impl_generics.push(TokenTree::Punct(p));
+ }
+ // Generics can have default values, we skip these.
+ TokenTree::Punct(p) if p.as_char() == '=' => {
+ skip_until_comma = true;
+ }
+ tt => impl_generics.push(tt),
+ }
+ }
+ _ => impl_generics.push(tt),
}
}
+ _ => {}
}
}
rest.extend(toks);
(
Generics {
impl_generics,
+ decl_generics,
ty_generics,
},
rest,
@@ -10,6 +10,7 @@ pub(crate) fn pin_data(args: TokenStream, input: TokenStream) -> TokenStream {
let (
Generics {
impl_generics,
+ decl_generics: _,
ty_generics,
},
rest,
@@ -7,6 +7,7 @@ pub(crate) fn derive(input: TokenStream) -> TokenStream {
let (
Generics {
impl_generics,
+ decl_generics: _,
ty_generics,
},
mut rest,