Move generic arg/param validation to `create_substs_for_generic_args` to resolve various const generics issues by varkor · Pull Request #68434 · rust-lang/rust

varkor · 2020-01-21T23:09:39Z

This changes some diagnostics, but I think they're around as helpful as the previous ones, and occur infrequently regardless.

Fixes #68257.
Fixes #68398.

r? @eddyb

rust-highfive · 2020-01-21T23:09:43Z

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

eddyb

LGTM, but I'd like a second opinion from @rust-lang/compiler (and maybe we need a check-mode crater run? I doubt it though)

src/librustc_typeck/astconv.rs

src/librustc/ty/structural_impls.rs

src/librustc_typeck/astconv.rs

estebank · 2020-01-24T22:19:09Z

src/librustc_typeck/astconv.rs

No reordering suggestion for this case?

I'd rather leave this PR for the bug-fixes and follow up with the diagnostics improvements. That's a good follow up, though.

varkor · 2020-01-25T22:25:56Z

r? @nikomatsakis for a second opinion (#68434 (review))

eddyb · 2020-01-25T23:02:54Z

src/librustc_typeck/astconv.rs

@Centril @estebank Can we make it so you have to emit an error to get an ErrorReported token, and maybe make it unconstructible except by the diagnostic reporting machinery?

Oh, that'd be cute. :)

Are we tracking this anywhere?

Don't think so.

Created #69426.

yodaldevoid · 2020-02-02T04:17:14Z

Is this PR waiting on anything specific?

varkor · 2020-02-02T10:23:14Z

@yodaldevoid: a second reviewer's opinion on the PR.

bors · 2020-02-06T19:09:06Z

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

nikomatsakis

I didn't follow 100% of what's going on here, but it overall seems pretty reasonable -- except that I think the new errors are kind of confusingly worded. Am I missing something?

src/librustc_typeck/astconv.rs

nikomatsakis · 2020-02-14T17:41:53Z

src/test/ui/generic/generic-arg-mismatch-recover.rs

why does this error go away?

Previously if we encountered an incorrect generic argument, we would just try to skip it and continue going: https://github.com/rust-lang/rust/pull/68434/files#diff-1b4d50b82a87f1031cd5f99b64123de7L578. Now we just stop checking the remaining arguments, because there's a high likelihood the remaining arguments are invalid and we'd just spit out lots of errors that aren't particularly helpful.

nikomatsakis · 2020-02-14T17:43:11Z

src/librustc_typeck/astconv.rs

(Annoying, it'd be nice to change this to a custom enum -- but obviously pre-existing for this PR.)

nikomatsakis · 2020-02-14T17:51:48Z

src/test/ui/suggestions/suggest-move-types.stderr

I find this error quite a bit more confusing than the old one. It suggests to me that the fix is to change T into a lifetime -- but that won't help.

I'm not quite sure what the "mental model" is that we are trying to convey to users.

I'm convinced we should be suggesting the correct order in all of these cases.

The reason I changed this is because there isn't really a restriction on the order of generic arguments: the restriction is on generic parameters, but generic arguments must match the order of generic parameters. The fact that lifetime arguments must precede type arguments is a side-effect of the parameter order. I'll try tweaking the wording to improve this. As @estebank says, ideally we'd suggest the correct order, but I'd like to get this merged sooner rather than later, as I don't know how much time I'll have for more elaborate changes now.

OK, I see. I still find the message more confusing than the old one, but perhaps I now better understand what this PR is aiming for.

nikomatsakis · 2020-02-14T18:02:14Z

src/librustc_typeck/astconv.rs

Oh, that'd be cute. :)

src/librustc_error_codes/error_codes/E0747.md

src/test/ui/const-generics/const-param-after-const-literal-arg.rs

src/test/ui/const-generics/const-param-before-other-params.rs

estebank · 2020-02-14T19:15:50Z

src/test/ui/suggestions/suggest-move-types.stderr

I'm convinced we should be suggesting the correct order in all of these cases.

…l error

varkor · 2020-02-22T02:38:23Z

@nikomatsakis: I've addressed the code comments, and also added a note to the error messages making them as descriptive as the previous ones. I'll open an issue for suggesting the correct order of all provided arguments: I'd rather split it out into a separate PR than conflate it with this one.

nikomatsakis · 2020-02-25T19:20:11Z

@bors r+

thanks, @varkor

bors · 2020-02-25T19:20:12Z

📌 Commit bead79e has been approved by nikomatsakis

bors · 2020-02-27T18:38:26Z

⌛ Testing commit bead79e with merge 6d69cab...

bors · 2020-02-27T21:56:22Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 6d69cab to master...

rust-highfive · 2020-02-27T21:57:29Z

📣 Toolstate changed by #68434!

Tested on commit 6d69cab.
Direct link to PR: #68434

🎉 book on windows: test-fail → test-pass (cc @carols10cents @steveklabnik).

@carols10cents

Tested on commit rust-lang/rust@6d69cab. Direct link to PR: <rust-lang/rust#68434> 🎉 book on windows: test-fail → test-pass (cc @carols10cents @steveklabnik).

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

rust-highfive assigned eddyb Jan 21, 2020

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

varkor force-pushed the astconv-mismatch-error branch from 9b4b01e to d2a750c Compare January 21, 2020 23:11

eddyb approved these changes Jan 21, 2020

View reviewed changes

Centril reviewed Jan 21, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

varkor mentioned this pull request Jan 21, 2020

Improve generic arg–param mismatch errors #68437

Closed

petrochenkov self-assigned this Jan 22, 2020

eddyb reviewed Jan 22, 2020

View reviewed changes

src/librustc/ty/structural_impls.rs Outdated Show resolved Hide resolved

eddyb reviewed Jan 22, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

varkor force-pushed the astconv-mismatch-error branch from 1174b94 to afad00d Compare January 23, 2020 01:16

eddyb reviewed Jan 24, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

eddyb reviewed Jan 24, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

eddyb reviewed Jan 24, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

eddyb reviewed Jan 24, 2020

View reviewed changes

src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved

petrochenkov removed their assignment Jan 24, 2020

estebank reviewed Jan 24, 2020

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned eddyb Jan 25, 2020

eddyb reviewed Jan 25, 2020

View reviewed changes

varkor mentioned this pull request Feb 9, 2020

ICE when type parameter and const generic are in wrong order #68868

Closed

nikomatsakis requested changes Feb 14, 2020

View reviewed changes

estebank reviewed Feb 14, 2020

View reviewed changes

varkor added 4 commits February 22, 2020 00:28

Refactor create_substs_for_generic_args a little

c9b7b1f

Make return value of check_generic_arg_count semantically clearer

dff64eb

Use Result instead of bool throughout

104131c

Be explicit about whether GenericArgCountMismatch arose from a fata…

33143fd

…l error

varkor force-pushed the astconv-mismatch-error branch from 332b61f to b0b88e7 Compare February 22, 2020 02:35

varkor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2020

This comment has been minimized.

Sign in to view

varkor added 3 commits February 22, 2020 11:34

Add invalid argument spans to GenericArgCountMismatch

76fb26b

Expand the documentation for E0747

e372ad4

Add note regarding argument ordering

bead79e

varkor force-pushed the astconv-mismatch-error branch from b0b88e7 to bead79e Compare February 22, 2020 11:34

varkor mentioned this pull request Feb 24, 2020

Make emitting an error necessary to acquire a ErrorReported token #69426

Open

nikomatsakis approved these changes Feb 25, 2020

View reviewed changes

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 Feb 25, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2020

bors merged commit 6d69cab into rust-lang:master Feb 27, 2020

This was referenced Feb 27, 2020

Create E0748 error code for unterminated raw strings #69260

Merged

Accept Enum::<Param>::Variant {} over Enum::Variant::<Param> {} #69363

Closed

JohnTitor mentioned this pull request Feb 28, 2020

ICE w/ default generic type parameter and const parameter #67858

Closed

varkor mentioned this pull request Jul 22, 2020

Tracking issue for const generics (RFC 2000) #44580

Closed

16 tasks

dwrensha mentioned this pull request Oct 14, 2021

thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0' #89868

Closed

Uh oh!

Conversation

varkor commented Jan 21, 2020

Uh oh!

rust-highfive commented Jan 21, 2020

Uh oh!

eddyb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varkor commented Jan 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yodaldevoid commented Feb 2, 2020

Uh oh!

varkor commented Feb 2, 2020

Uh oh!

bors commented Feb 6, 2020

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varkor commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

nikomatsakis commented Feb 25, 2020

Uh oh!

bors commented Feb 25, 2020

Uh oh!

bors commented Feb 27, 2020

varkor commented Feb 22, 2020 •

edited

Loading