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 ICE when calling non-functions within closures #46780

Merged
merged 5 commits into from
Dec 22, 2017

Conversation

varkor
Copy link
Member

@varkor varkor commented Dec 17, 2017

The visitor for walking function bodies did not previously properly
handle error-cases for function calls. These are now ignored,
preventing the panic. This fixes #46771.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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.

fn main() {
struct Foo;
(1 .. 2).find(|_| Foo(0) == 0); //~ ERROR expected function, found `main::Foo`
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing newline

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, that's what I get for late-night commits. Fixed.

@arielb1
Copy link
Contributor

arielb1 commented Dec 18, 2017

Your code does not compile libcore, and I suspect it allows such aberrant code as

fn foo<F: FnMut()>(f: &F) {
    (*f)();
}

The visitor for walking function bodies did not previously properly
handle error-cases for function calls. These are now ignored,
preventing the panic.
@varkor
Copy link
Member Author

varkor commented Dec 18, 2017

Not quite sure what happened, but that previous commit was entirely wrong. Sorry, it should be fixed now.

ty::ImmBorrow,
ClosureInvocation);
let type_dependent_defs = self.mc.tables.type_dependent_defs();
if type_dependent_defs.contains_key(call.hir_id) {
Copy link
Contributor

@arielb1 arielb1 Dec 18, 2017

Choose a reason for hiding this comment

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

Could you use this instead? It's more standard.

if let Some(def) = type_dependent_defs.get(call.hir_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually prefer

let def_id = match type_dependent_defs.get(call.hir_id) {
    Some(def) => def.def_id(),
    None => tcx.sess.delay_span_bug(call.span, "no type-dependant def for overloaded call?)
};

as that would make sure we get an ICE if there's ever a mismatch.

Copy link
Member Author

@varkor varkor Dec 18, 2017

Choose a reason for hiding this comment

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

Isn't an ICE exactly what we're trying to avoid in this situation? In the issue this fix addresses, this function is called for a struct (with no type-dependent call definition) — an error has already been logged, we just want to make sure we don't attempt anything else with it.
EDIT: delay_span_bug will only ICE if no errors have been reported when this occurs.

@arielb1
Copy link
Contributor

arielb1 commented Dec 18, 2017

r=me with comment addressed

@arielb1
Copy link
Contributor

arielb1 commented Dec 18, 2017

[00:03:23] tidy error: /checkout/src/librustc/middle/expr_use_visitor.rs:582: line longer than 100 chars

@arielb1
Copy link
Contributor

arielb1 commented Dec 18, 2017

I'll r+ when travis is green

@arielb1
Copy link
Contributor

arielb1 commented Dec 18, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Dec 18, 2017

📌 Commit 5741dcd has been approved by arielb1

@kennytm kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 19, 2017
kennytm added a commit to kennytm/rust that referenced this pull request Dec 21, 2017
Fix ICE when calling non-functions within closures

The visitor for walking function bodies did not previously properly
handle error-cases for function calls. These are now ignored,
preventing the panic. This fixes rust-lang#46771.
bors added a commit that referenced this pull request Dec 21, 2017
Rollup of 14 pull requests

- Successful merges: #46636, #46780, #46784, #46809, #46814, #46820, #46839, #46847, #46858, #46878, #46884, #46890, #46898, #46918
- Failed merges:
@bors bors merged commit 5741dcd into rust-lang:master Dec 22, 2017
@varkor varkor deleted the contrib-5 branch December 22, 2017 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[ICE] Calling syntax on struct
7 participants