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

Fix substs issues for const errors #60508

Merged
merged 5 commits into from
May 5, 2019

Conversation

varkor
Copy link
Member

@varkor varkor commented May 3, 2019

Fixes #60503.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 3, 2019
@varkor
Copy link
Member Author

varkor commented May 3, 2019

This currently breaks

fn foo<const X: (), T>(_: &T) {
//~^ ERROR type parameters must be declared prior to const parameters
}

because it tries to substitute a const for a type parameter. I think this was a pre-existing issue, but the bug this PR fixes was hiding it. I think that type parameters are still being ordered before const parameters (even if they appear afterwards in the source code), so there's a mismatch.

@estebank
Copy link
Contributor

estebank commented May 3, 2019

@varkor the parser ignores the order when making the ast node but complains about the order so an error will be emitted. Why do we depend on source ordering here?

@varkor
Copy link
Member Author

varkor commented May 3, 2019

@estebank: I haven't fully diagnosed the issue yet, but ideally parameters would preserve their source order throughout the compiler: there shouldn't be any reason lifetimes, types and consts should come in that order specifically. The error should essentially just be a lint. However, that hasn't always been the case and I imagine there are some broken assumptions here because of it.

@varkor
Copy link
Member Author

varkor commented May 3, 2019

A quick fix would be to abort as soon as misordered const/type parameters are found in a signature. It should only regress diagnostics for const generics and I can fix the issue properly later.

@estebank
Copy link
Contributor

estebank commented May 3, 2019

@varkor I think the order is lost because in the parser we collect independent vecs for each type of argument instead of one vec with annotated items. We could indeed FatalError.raise() for this case in particular, but would like it to have a FIXME and fixed quickly, as I wouldn't want the const experience to be bad for people starting to try the feature out.

--> $DIR/const-param-in-trait.rs:3:12
|
LL | #![feature(const_generics)]
| ^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a first 😄

@varkor varkor force-pushed the const-generics-fold-ct-err branch from 7cbf469 to 735714a Compare May 4, 2019 13:44
@varkor varkor marked this pull request as ready for review May 4, 2019 13:45
@varkor
Copy link
Member Author

varkor commented May 4, 2019

I've added a temporary FatalError.raise() and a FIXME.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 4, 2019

📌 Commit 735714af24f422dcba10d1beabf35a0816f645c2 has been approved by petrochenkov

@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 May 4, 2019
@varkor varkor force-pushed the const-generics-fold-ct-err branch from 735714a to 1f47f12 Compare May 4, 2019 21:27
@varkor varkor force-pushed the const-generics-fold-ct-err branch from 1f47f12 to 3e6787c Compare May 4, 2019 21:28
@varkor
Copy link
Member Author

varkor commented May 4, 2019

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented May 4, 2019

📌 Commit 3e6787c has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented May 5, 2019

⌛ Testing commit 3e6787c with merge c55b68a...

bors added a commit that referenced this pull request May 5, 2019
@bors
Copy link
Contributor

bors commented May 5, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing c55b68a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 5, 2019
@bors bors merged commit 3e6787c into rust-lang:master May 5, 2019
@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Oct 19, 2019
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
Development

Successfully merging this pull request may close these issues.

Const generic type parameter in trait declaration ICE
5 participants