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

Fragment Specifiers for Generic Arguments #3442

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

Conversation

JarredAllen
Copy link

@JarredAllen JarredAllen commented Jun 1, 2023

I wrote up my thoughts from this rust-internals thread into an RFC.

Rendered

things instead.

Also, should `:generic_bound` include the preceding `:` in the match (e.g. `: 'a
+ SomeTrait` in the example above)? And likewise with the `=` before
Copy link
Member

Choose a reason for hiding this comment

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

don't word-wrap in the middle of a ` code span, it doesn't get rendered properly

Copy link
Author

Choose a reason for hiding this comment

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

Oops, I guess I need a better editor config. Fixed in d77ac90

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jun 1, 2023
Copy link

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

This RFC does not include specifiers for where-clause, which is highly related to generics. Is this intentional? Maybe we should mention this in text.


* `:generic`: A full set of generic parameters and their bounds (e.g. `<'a, T:
'a + SomeTrait, const N: usize>`)
* `:generic_param`: A generic parameter (e.g. `'a`, `T`, or `const N`)
Copy link

@oxalica oxalica Jun 2, 2023

Choose a reason for hiding this comment

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

const N does not work for the macro example above, $param:generic_param $( : $bound:generic_bound )?. Const generics have the syntax const $param:ident : $ty:ty $(= $default:expr)?. Note that it is a type instead of generic bounds after the colon, so syntactically it should also accept non-trait-like types like const N: (i32, i32), though it's forbidden currently.

Copy link
Author

Choose a reason for hiding this comment

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

This was a deliberate deviation from the default syntax to make it easier to parse an arbitrary set of generic parameters. If we use the fragment specifiers as I defined them in the example macro defined in the RFC (< $( $param:generic_param $( : $bound:generic_bound )? ),+ > , then this successfully matches <const N: usize> ($param being const N and $bound being usize.

If it matches as you're requesting that it match, then the same pattern will match this trait, but it will also allow in nonsense like <const N: usize: 'a + SomeTrait> which my approach avoids. I'm not sure how one could change the pattern to avoid allowing for nonsense like that.

Though also, my approach would match <const N>, which is also nonsense, so idk which one would be better to reject while matching the macro. I'm open to changing it if you have arguments for your way of splitting the definition between the two fragment specifiers being better.

To your last point, the definition that I give for :generic_bounds does explicitly allow for a type, so that syntax would be accepted.

Copy link

@Aloso Aloso Jun 5, 2023

Choose a reason for hiding this comment

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

Your approach also matches const N: 'a and 'a: usize (see my comment below)

Comment on lines 88 to 93
* `:generic_bound` matches the
[`TypeParamBounds`](https://doc.rust-lang.org/reference/trait-bounds.html)
(can be the bounds on a type parameter) or
[`LifetimeBounds`](https://doc.rust-lang.org/reference/trait-bounds.html) (can
be the bounds on a lifetime parameter) grammar items, or a type (can be the
bounds on a const parameter).
Copy link

Choose a reason for hiding this comment

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

Should it be named "generic_bound" or "generic_bounds"?
Also that TypeParamBounds does not allow empty, while the generic syntax do allow <T: /*nothing*/> (except for const generics). If we want it to match multiple bounds, it should also match no bounds.

Copy link
Author

Choose a reason for hiding this comment

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

Neat, I never knew that Rust allowed <T:> as a generic parameter list. That doesn't seem very useful to me, but I'll change it to allow that since I agree that it should be matched if Rust allows it.

@clarfonthey
Copy link
Contributor

This RFC does not include specifiers for where-clause, which is highly related to generics. Is this intentional? Maybe we should mention this in text.

I think this would be nice longer-term, but at least for now, the extremely simple case of being able to check for pieces like <T, const N> without resorting to using token-trees is pretty good on its own. That said, the inclusion of bounds in this RFC does beg the inclusion of where bounds too.

@Aloso
Copy link

Aloso commented Jun 4, 2023

I like the general idea of this RFC, but I don't like that a macro accepting $( $p:generic_param $( : $b:generic_bound )? ),* can accept illegal syntax (like a lifetime followed by a trait bound, or a const generic followed by a lifetime bound).

I also don't like that matching all generics syntax with the more granular fragments is pretty verbose:

< $(
    $( #[$m:meta] )*
    $p:generic_param
    $( : $( $b:generic_bound )? )?
    $( = $d:generic_default )?
),* $(,)? >

a where clause could be written like this, if we require trailing commas:

where $(
    $( $l:lifetime $( : $( $b1:generic_bound )? )? , )?
    $( $( for $f:generic )? $t:ty $( : $( $b2:generic_bound )? )? , )?
)*

But this isn't forward compatible with

@Aloso
Copy link

Aloso commented Jun 4, 2023

Maybe it would be best to remove :generic_param, :generic_bound and :generic_default from the RFC (unless you can give an example where they're useful), and instead add :where_clause.

@JarredAllen
Copy link
Author

I added a :where_clause, as per multiple requests. They were excluded initially because I forgot that they existed, not for any real reason.

@JarredAllen
Copy link
Author

I like the general idea of this RFC, but I don't like that a macro accepting $( $p:generic_param $( : $b:generic_bound )? ),* can accept illegal syntax (like a lifetime followed by a trait bound, or a const generic followed by a lifetime bound).

I also don't like that matching all generics syntax with the more granular fragments is pretty verbose:

Yeah, sadly the only way to prevent it from accepting illegal syntaxes like const N: 'a or 'a: usize would make it even more verbose and complicated to match the entire generics syntax, which makes your other point worse. Though, you don't need the $()? around :generic_bound anymore, I didn't know that nothing could be put there and have since updated it to allow that, so it's slightly less verbose.

I tried to strike a happy medium where relatively few illegal syntaxes are allowed (I think they've all been said on this page? Unless I'm missing some) but also it's reasonably non-verbose to parse the full generic parameter definition syntax and we're not adding too many new fragment specifiers into the language, and the split is done in such a way that it could be useful for implementing macros.

I'm open to any suggestions you have for making that trade-off better, but this was the best balance between the two goals I could think of.

When explaining fragment specifiers, this can be explained by adding the new
fragment specifiers and their descriptions:

* `:generic`: A full set of generic parameters and their bounds (e.g. `<'a, T:

Choose a reason for hiding this comment

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

maybe a nit but generic being singular confuses me. I think it would make more sense for it to be named generics or generic_set, or anything to add plurality to the concept, especially since it would be responsible for parsing the opening and closing angle brackets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants