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

Typeck highlevel before bodies #24422

Merged
merged 3 commits into from
Apr 17, 2015

Conversation

pnkfelix
Copy link
Member

typeck: Do high-level structural/signature checks before function body checks.

This avoids various ICEs, e.g. premature calls to cat_expr that yield the dreaded "cat_expr Errd" ICE.

However, it also means that some early error feedback is now not provided. This may be for the best, because the error feedback were were providing in some of those cases were false positives -- it was spurious feedback and a distraction from the real problem.

So it is not 100% clear whether we actually want to put this change in or not. I think its a net win, but others might disagree.

(Kudos to @arielb1 for suggesting this modification.)

@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@pnkfelix
Copy link
Member Author

r? @nikomatsakis

}

impl<T:Get> Other for T {
fn uhoh<U:Get>(&self, foo: U, bar: <(T, U) as Get>::Value) {}
//~^ ERROR the trait `Get` is not implemented for the type `(T, U)`
//~| ERROR the trait `Get` is not implemented for the type `(T, U)`
Copy link
Member

Choose a reason for hiding this comment

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

👍 this PR just for this.

@ebfull
Copy link
Contributor

ebfull commented Apr 14, 2015

Nice!

@lilyball
Copy link
Contributor

Conceptually, I like this change. I'd rather debug the high-level structural issues first, especially as issues in function bodies may actually just be symptoms of high-level issues.

@nikomatsakis
Copy link
Contributor

r+

I've been thinking about doing exactly this, actually, so happy that you beat me to it. (The other thing I have been considering is adding some kind of rough dependency analysis so we can type-check callees before callers.)

@nikomatsakis
Copy link
Contributor

Note though that we already have the wf pass, which is supposed to be exactly this distinction of signatures vs bodies. It was introduced because forcing types to have the right annotations avoided a lot of spurious errors. Note sure if it makes sense to keep the "three-phases". I think i'm inclined to r+ it as is but gradually refactor check/mod to break it up and make the pass structure clearer.

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 17, 2015

📌 Commit d82f912 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 17, 2015

⌛ Testing commit d82f912 with merge 971440a...

@bors
Copy link
Contributor

bors commented Apr 17, 2015

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Apr 17, 2015

⌛ Testing commit d82f912 with merge 7fbedc5...

bors added a commit that referenced this pull request Apr 17, 2015
…omatsakis

typeck: Do high-level structural/signature checks before function body checks.

This avoids various ICEs, e.g. premature calls to cat_expr that yield the dreaded "cat_expr Errd" ICE.

However, it also means that some early error feedback is now not provided.  This may be for the best, because the error feedback were were providing in some of those cases were false positives -- it was spurious feedback and a distraction from the real problem.

So it is not 100% clear whether we actually want to put this change in or not.  I think its a net win, but others might disagree.

(Kudos to @arielb1 for suggesting this modification.)
@bors bors merged commit d82f912 into rust-lang:master Apr 17, 2015
@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 21, 2015
@pnkfelix
Copy link
Member Author

normally we would not (should not) accept a change like this for beta, but, since this (1.) fixes ICEs (2.) is a user-experience improvement, and (3.) is pretty low-risk, we are taking it just because 1.0 release is a special case

going from nominated to (nominated, accepted)

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 23, 2015
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants