Remove restriction on type parameters preceding consts w/ feature const-generics #74953

JulianKnodt · 2020-07-30T18:58:48Z

Removed the restriction on type parameters preceding const parameters when the feature const-generics is enabled.

Builds on #74676, which deals with unsorted generic parameters. This just lifts the check in lowering the AST to HIR that permits consts and types to be reordered with respect to each other. Lifetimes still must precede both

This change is not intended for min-const-generics, and is gated behind the #![feature(const_generics)].

One thing is that it also permits type parameters without a default to come after consts, which I expected to not work, and was hoping to get more guidance on whether that should be permitted or how to prevent it otherwise.

I did not go through the RFC process for this pull request because there was prior work to get this feature added. In the previous PR that was cited, work was done to enable this change.

r? @lcnr

rust-highfive · 2020-07-30T18:58:53Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

lcnr

Nice to see that this already works!

The way ParamKindOrd works rn is somewhat unfortunate, I think either adding a field to ParamKindOrd::Const(bool) to specify if const_generics is active or adding a new ParamKindOrd::MinConst variant is the best way to fix this for now.

For example I would expect the following to still result in an incorrect help message.

struct Foo<const N: usize, 'a, T = u32>(&'a (), T);

src/test/ui/const-generics/defaults/right-order.rs

src/test/ui/const-generics/defaults/needs-feature.rs

lcnr · 2020-07-30T19:41:41Z

cc @varkor @withoutboats @eddyb

The goal of this PR is to remove the restriction on the param ordering between types and consts to allow for type defaults.

Should we only lift that restriction when using feature(const_generics) and keep disallowing this when using feature(min_const_generics)?

As you can see from this PR, keeping that restriction doesn't simplify the implementation. So keeping that restriction would mostly allow us keep forcing that parameter order in the future.

I personally am unsure how else we might want to implement default parameters here, so I would be fine with also allowing this as part of of the MVP.

src/librustc_ast_passes/ast_validation.rs

JulianKnodt · 2020-08-05T22:38:54Z

Woops accidentally closed this while trying to revert some accidental merges

JulianKnodt · 2020-08-06T04:25:43Z

@bors try

bors · 2020-08-06T04:25:45Z

@JulianKnodt: 🔑 Insufficient privileges: not in try users

JulianKnodt · 2020-08-06T04:26:19Z

Hm it works locally with these changes

src/test/ui/const-generics/argument_order.stderr

src/librustc_ast/ast.rs

bors · 2020-08-08T04:19:46Z

☔ The latest upstream changes (presumably #74877) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr

A lot of small nits, once these are fixed r=me.

src/librustc_ast/ast.rs

src/test/ui/const-generics/defaults/complex-unord-param.rs

src/test/ui/const-generics/defaults/right-order.rs

src/test/ui/const-generics/defaults/type-after-const-requires-default.rs

src/librustc_ast/ast.rs

src/test/ui/const-generics/defaults/needs-feature.rs

src/librustc_ast/ast.rs

Added this test to ensure that reordering the parameters only works with the feature const generics enabled. Fixed nits Also added another test to verify that intermixed lifetimes are forbidden

Added more complex test and changed error message

Added minor fmt change to ast_validation

Run fmt

Updated tests and error msgs Update stderr from test Update w/ lcnr comments Change some tests around, and also updated Ord implementation for ParamKindOrd Update w/ nits from lcnr

lcnr · 2020-08-10T06:02:26Z

@bors r+

bors · 2020-08-10T06:02:27Z

📌 Commit 64f6437 has been approved by lcnr

bors · 2020-08-10T08:41:59Z

⌛ Testing commit 64f6437 with merge 04f5cefa734a20bd5edb13d43c0322720ac7174f...

bors · 2020-08-10T12:56:59Z

💥 Test timed out

lcnr · 2020-08-10T13:01:38Z

@bors retry

bors · 2020-08-10T15:19:53Z

⌛ Testing commit 64f6437 with merge 08324fe...

bors · 2020-08-10T17:12:34Z

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 08324fe to master...

varkor

What's the reason we only allow consts and types to be reordered? If we're lifting the arbitrary restriction, we may as well lift it entirely.

varkor · 2020-08-12T11:34:11Z

src/test/ui/const-generics/defaults/needs-feature.min.stderr

+  --> $DIR/needs-feature.rs:10:26
+   |
+LL | struct A<const N: usize, T=u32>(T);
+   |         -----------------^----- help: reorder the parameters: lifetimes, then consts, then types: `<T, const N: usize>`


This error message is wrong.

varkor · 2020-08-12T11:34:35Z

src/librustc_ast_passes/ast_validation.rs

+                if sess.features_untracked().const_generics {
+                    ", then consts and types"
+                } else if sess.features_untracked().min_const_generics {
+                    ", then consts, then types"


Shouldn't this be ", then types, then consts"?

@JulianKnodt do you want to open a PR for this? Otherwise I will quickly fix that

varkor · 2020-08-12T11:36:11Z

Sorry, I didn't have time to get to this before now.

lcnr · 2020-08-12T11:52:57Z

What's the reason we only allow consts and types to be reordered? If we're lifting the arbitrary restriction, we may as well lift it entirely.

mixing lt with types was forbidden before 😆 I do feel like const and type parameters are more similar than lt params.

We also can't specify late bound lt params, which makes mixing them with other parameters somewhat undesirable imo.
(https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=06b9ccfc5d0c7ba4fd610635eda2179a)

Lifting the ordering restriction on lt bounds would also require an RFC, wouldn't it?

varkor · 2020-08-12T17:32:31Z

Lifting the ordering restriction on lt bounds would also require an RFC, wouldn't it?

Really, lifting the restriction even for normal const generics ought to require some discussion, as it wasn't part of the original RFC. I think if we're restricting at all, it makes sense to have lifetimes, types, consts, type defaults, const defaults as a strict order.

@varkor

Flip order of const & type Fix swapped order of consts & types in error message introduced in rust-lang#74953 r? @varkor cc @lcnr

rust-highfive assigned lcnr Jul 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2020

lcnr requested changes Jul 30, 2020

View reviewed changes

src/test/ui/const-generics/defaults/right-order.rs Outdated Show resolved Hide resolved

src/test/ui/const-generics/defaults/needs-feature.rs Outdated Show resolved Hide resolved

tesuji reviewed Jul 31, 2020

View reviewed changes

src/librustc_ast_passes/ast_validation.rs Outdated Show resolved Hide resolved

JulianKnodt closed this Aug 5, 2020

JulianKnodt force-pushed the master branch from b8149d3 to 52b179b Compare August 5, 2020 22:00

JulianKnodt reopened this Aug 5, 2020

lcnr reviewed Aug 6, 2020

View reviewed changes

src/test/ui/const-generics/argument_order.stderr Outdated Show resolved Hide resolved

lcnr reviewed Aug 6, 2020

View reviewed changes

src/librustc_ast/ast.rs Show resolved Hide resolved

JulianKnodt force-pushed the master branch from 7d3175b to 14c1df2 Compare August 6, 2020 08:39

JulianKnodt requested a review from lcnr August 6, 2020 22:23

JulianKnodt added 2 commits August 8, 2020 04:40

Allow types to come after consts in AST validation

58b1a04

Rm restriction on ord of default types w/ consts

18481cb

JulianKnodt force-pushed the master branch from 80bb3ad to 03a9bce Compare August 8, 2020 04:43

lcnr approved these changes Aug 8, 2020

View reviewed changes

JulianKnodt force-pushed the master branch from 16e9867 to 76bd4a4 Compare August 8, 2020 20:18

lcnr reviewed Aug 9, 2020

View reviewed changes

src/librustc_ast/ast.rs Outdated Show resolved Hide resolved

src/test/ui/const-generics/defaults/needs-feature.rs Outdated Show resolved Hide resolved

src/librustc_ast/ast.rs Outdated Show resolved Hide resolved

src/librustc_ast/ast.rs Outdated Show resolved Hide resolved

JulianKnodt added 6 commits August 9, 2020 07:41

Added +1 test for only works w/ feat const gen

f858828

Added this test to ensure that reordering the parameters only works with the feature const generics enabled. Fixed nits Also added another test to verify that intermixed lifetimes are forbidden

Test lifetimes after types after consts forbidden

b8352eb

Added more complex test and changed error message

Blessed old test where error message had changed

319c4f4

Added minor fmt change to ast_validation

Add ParamKindOrd::ConstUnordered variant

1ae1a63

Switched to unordered field in ParamKindOrd

4f461f5

Run fmt

Change Ord impl for ParamKindOrd

be0d6f1

Updated tests and error msgs Update stderr from test Update w/ lcnr comments Change some tests around, and also updated Ord implementation for ParamKindOrd Update w/ nits from lcnr

JulianKnodt force-pushed the master branch from 2120b6d to f0d8b36 Compare August 9, 2020 20:54

Convert Eq impl to check Ord::Equal

64f6437

JulianKnodt force-pushed the master branch from f0d8b36 to 64f6437 Compare August 10, 2020 05:55

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 10, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 10, 2020

bors merged commit 08324fe into rust-lang:master Aug 10, 2020

varkor added the F-const_generics `#![feature(const_generics)]` label Aug 12, 2020

varkor reviewed Aug 12, 2020

View reviewed changes

JulianKnodt mentioned this pull request Aug 12, 2020

Flip order of const & type #75473

Merged

lcnr mentioned this pull request Nov 17, 2020

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

zjijz mentioned this pull request Nov 21, 2020

const generic parameter is not ordered correctly on async fn with body dtolnay/async-trait#134

Closed

Undin mentioned this pull request Jul 9, 2021

INT: const generic parameters handling by "Add impl block" intention intellij-rust/intellij-rust#7473

Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove restriction on type parameters preceding consts w/ feature const-generics #74953

Remove restriction on type parameters preceding consts w/ feature const-generics #74953

JulianKnodt commented Jul 30, 2020

rust-highfive commented Jul 30, 2020

lcnr left a comment

lcnr commented Jul 30, 2020 •

edited

Loading

JulianKnodt commented Aug 5, 2020 •

edited

Loading

JulianKnodt commented Aug 6, 2020

bors commented Aug 6, 2020

JulianKnodt commented Aug 6, 2020

bors commented Aug 8, 2020

lcnr left a comment

lcnr commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

lcnr commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

varkor left a comment

varkor Aug 12, 2020

varkor Aug 12, 2020

lcnr Aug 12, 2020

lcnr Aug 12, 2020 •

edited

Loading

varkor commented Aug 12, 2020

lcnr commented Aug 12, 2020

varkor commented Aug 12, 2020

Remove restriction on type parameters preceding consts w/ feature const-generics #74953

Remove restriction on type parameters preceding consts w/ feature const-generics #74953

Conversation

JulianKnodt commented Jul 30, 2020

rust-highfive commented Jul 30, 2020

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Jul 30, 2020 • edited Loading

JulianKnodt commented Aug 5, 2020 • edited Loading

JulianKnodt commented Aug 6, 2020

bors commented Aug 6, 2020

JulianKnodt commented Aug 6, 2020

bors commented Aug 8, 2020

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

lcnr commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

varkor left a comment

Choose a reason for hiding this comment

varkor Aug 12, 2020

Choose a reason for hiding this comment

varkor Aug 12, 2020

Choose a reason for hiding this comment

lcnr Aug 12, 2020

Choose a reason for hiding this comment

lcnr Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

varkor commented Aug 12, 2020

lcnr commented Aug 12, 2020

varkor commented Aug 12, 2020

lcnr commented Jul 30, 2020 •

edited

Loading

JulianKnodt commented Aug 5, 2020 •

edited

Loading

lcnr Aug 12, 2020 •

edited

Loading