-
Notifications
You must be signed in to change notification settings - Fork 537
Document inferred const args (feature(generic_arg_infer)
)
#1835
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
base: master
Are you sure you want to change the base?
Conversation
@ehuss and I are reviewing this on a call. This is a bit tough to review in that the content was both moved and changed in one commit. If possible, it'd be helpful to us if this could be broken out into (at least) two commits; one to move the content, then the other or others to make the changes and additions. @rustbot author |
da274a2
to
48cfb78
Compare
Sure, done |
@rustbot ready |
@ehuss @traviscross any chance y'all will be able to get around to reviewing this soon? The stabilization PR is now ready to be merged and it'd be unfortunate to block a relatively minor feature that's already been waiting years, on this PR. |
Alternatively I could file a separate smaller PR that just adds a small extra bit of wording to the existing section noting that |
If possible, I think it would be best to drop the rearrangement commit. This probably isn't the organization we'll land on, and I would prefer to keep these as separate concerns so they don't block one another. |
48cfb78
to
f356de0
Compare
feature(generic_arg_infer)
)
Ok this should be fine now |
f5f99d4
to
b63bb63
Compare
b63bb63
to
4b5878f
Compare
@traviscross @ehuss friendly poke about this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little lost, as it seems like there are two distinct things being supported here:
_
in generic args:foo::<_>()
_
in array expression repeat lengths:[123; _]
Am I understanding that correctly? If so, shouldn't there be some update to the array expression chapter? And the underscore-expr.md may need a significant update to mention that it is allowed for array repeats.
```grammar,const | ||
InferredConst -> | ||
`_` | ||
| `(` InferredConst `)` | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand this grammar since it isn't referred to anywhere.
Am I understanding correctly that this is added for GenericArgsConst? Or is it somewhere else? That is, should it be:
GenericArgsConst ->
BlockExpression
| LiteralExpression
| `-` LiteralExpression
| SimplePathSegment
| InferredConst
InferredConst ->
`_`
| `(` InferredConst `)`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep exactly 👍 It's a little bit funny because it means that InferredType and InferredConst share the same grammar but I'm not sure that matters too much for this?
r[items.generics.const.inferred.constraint] | ||
It cannot be used in item signatures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following this. If we are defining this as a new form allowed in generic args, why would there be a rule that it is not allowed in item signatures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't allow type inference in signatures in general, not just as a restriction on const arguments. For example this is an error on stable:
struct Foo<T>(T);
fn bar(param: Foo<_>) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already stated in type.inferred.constraint (inferred.md)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we state that for _
type arguments but I felt like like we should note that the restriction applies for _
const arguments too when discussing them as it might be confusing otherwise; people might get the idea that you can use _
const arguments in signatures.
|
||
r[items.generics.const.inferred.intro] | ||
The inferred const asks the compiler to infer the const argument if possible | ||
based on the surrounding information available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a semi-realistic looking example here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah sure
There is one thing being added, we support I didn't think the array-expr chapter would need updating as I've updated the definition of a const argument but it seems like the reference attempts to separately specify repeat expr counts from const arguments. This also means that the chapter does not currently talk about the fact that
I don't think we need to update underscore-expr, this feature is (confusingly) entirely orthogonal from |
If I'm understanding correctly, we define the array repeat length as a const expression with the limitations defined in const context among other places. If we are adding
I'm not quite following this. That looks like an I understand that there may be some implementation similarities, but as we define type vs expression as two different concepts, I would think that introducing
I believe these limitations are defined in https://doc.rust-lang.org/nightly/reference/const_eval.html#const-context and https://doc.rust-lang.org/nightly/reference/items/generics.html#r-items.generics.const.standalone. |
Sorry I feel like I'm repeating myself a bit here and am not sure how to explain this better. This feature is orthogonal to underscores as an expression. It's not that Maybe the core confusion here is about this distinction between a const expression and a const argument?
I think the entire way that const generics are handled in the reference right now is just not structured right for this feature to be honest ^^. It doesn't make sense to talk about const arguments as being "expressions with more restrictions"; I originally tried to refactor the reference to properly talk about const arguments as a real concept in the type system section with different sections for each kind of const argument. But that was quite a big change and this seemed smaller/easier to review. Do you think you'd prefer going back to the old approach that has more involved explanations of this topic? It sounds like the new version of this PR is actually quite confusing in practice which is unfortunate. Admittedly, I'm finding this PR to be quite stressful. I'd really like to get I'd be more than happy to put a bunch of time into documenting things in the reference and talking things over with you, but if that's where things need to go in order to get something you're happy with would it be possible to just land the stabilization PR first and continue work on documenting this afterwards?
I guess with this terminology it would be correct to say that the repeat count of array exprs is a type position ^^. Though being nitpicky it doesn't make sense to say that a
Ah yes you're right it is documented 👍 Then I guess array-expr really doesn't need updating because it is specified as being a const generic argument :D |
Tracking: rust-lang/rust#85077
Stabilization: rust-lang/rust#141610
Stabilization report: rust-lang/rust#141610 (comment)