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

Move generic arg/param validation to create_substs_for_generic_args to resolve various const generics issues #68434

Merged
merged 12 commits into from
Feb 27, 2020

Conversation

varkor
Copy link
Member

@varkor varkor commented Jan 21, 2020

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

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Jan 21, 2020
@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

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

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@petrochenkov petrochenkov removed their assignment Jan 24, 2020
arg.descr(),
kind,
)
.emit();
Copy link
Contributor

Choose a reason for hiding this comment

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

No reordering suggestion for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

varkor commented Jan 25, 2020

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

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Jan 25, 2020
Comment on lines 332 to 337
explicit_lifetimes = Err(GenericArgCountMismatch(Some(ErrorReported)));
let mut err = tcx.sess.struct_span_err(span, msg);
err.span_note(span_late, note);
err.emit();
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that'd be cute. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we tracking this anywhere?

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 think so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #69426.

@yodaldevoid
Copy link
Contributor

Is this PR waiting on anything specific?

@varkor
Copy link
Member Author

varkor commented Feb 2, 2020

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

@bors
Copy link
Contributor

bors commented Feb 6, 2020

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

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
src/librustc_typeck/astconv.rs Outdated Show resolved Hide resolved
@@ -4,7 +4,6 @@ struct Bar<'a>(&'a ());

fn main() {
Foo::<'static, 'static, ()>(&0); //~ ERROR wrong number of lifetime arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this error go away?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

(Some(generic_args), infer_args)
} else {
// The last component of this tuple is unimportant.
(None, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -74,29 +74,30 @@ LL | struct Dl<'a, 'b, 'c, T, U, V, M: ThreeWithLifetime<T, 'a, A=(), B=(), U, '
| | this associated type binding should be moved after the generic parameters
| this associated type binding should be moved after the generic parameters

error: lifetime arguments must be declared prior to type arguments
--> $DIR/suggest-move-types.rs:34:46
error[E0747]: type provided when a lifetime was expected
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 332 to 337
explicit_lifetimes = Err(GenericArgCountMismatch(Some(ErrorReported)));
let mut err = tcx.sess.struct_span_err(span, msg);
err.span_note(span_late, note);
err.emit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that'd be cute. :)

src/librustc_error_codes/error_codes/E0747.md Show resolved Hide resolved
@@ -74,29 +74,30 @@ LL | struct Dl<'a, 'b, 'c, T, U, V, M: ThreeWithLifetime<T, 'a, A=(), B=(), U, '
| | this associated type binding should be moved after the generic parameters
| this associated type binding should be moved after the generic parameters

error: lifetime arguments must be declared prior to type arguments
--> $DIR/suggest-move-types.rs:34:46
error[E0747]: type provided when a lifetime was expected
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@varkor
Copy link
Member Author

varkor commented Feb 22, 2020

@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.

@varkor 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
@rust-highfive

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

@bors r+

thanks, @varkor

@bors
Copy link
Contributor

bors commented Feb 25, 2020

📌 Commit bead79e has been approved by nikomatsakis

@bors 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
Copy link
Contributor

bors commented Feb 27, 2020

⌛ Testing commit bead79e with merge 6d69cab...

@bors
Copy link
Contributor

bors commented Feb 27, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2020
@bors bors merged commit 6d69cab into rust-lang:master Feb 27, 2020
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #68434!

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
10 participants