Skip to content

[MIR] Handle overloaded call expressions during HIR -> HAIR translation. #30692

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

Merged
merged 3 commits into from
Jan 6, 2016

Conversation

michaelwoerister
Copy link
Member

So far, calls going through Fn::call, FnMut::call_mut, or FnOnce::call_once have not been translated properly into MIR:
The call f(a, b, c) where f: Fn(T1, T2, T3) would end up in MIR as:

call `f` with arguments  `a`, `b`, `c`

What we really want is:

call `Fn::call` with arguments  `f`, `a`, `b`, `c`

This PR transforms these kinds of overloaded calls during HIR -> HAIR translation.

What's still a bit funky is that the Fn traits expect arguments to be tupled but due to special handling type-checking and trans, we do not actually tuple arguments and everything still checks out fine. So, after this PR we end up with MIR containing calls where function signature and arguments seemingly don't match:

call Fn::call(&self, args: (T1, T2, T3)) with arguments `f`, `a`, `b`, `c`

instead of

call Fn::call(&self, args: (T1, T2, T3)) with arguments `f`, (`a`, `b`, `c`)  //  <- args tupled!

It would be nice if the call traits could go without special handling in MIR and later on.

@rust-highfive
Copy link
Contributor

r? @nikomatsakis

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

@@ -88,6 +88,19 @@ fn test8() -> isize {
Two::two()
}

#[rustc_mir(graphviz="method.dot")]
Copy link
Member

Choose a reason for hiding this comment

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

We do not want graphviz output during tests.

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 shouldn't be in there...

@nagisa
Copy link
Member

nagisa commented Jan 4, 2016

Overall LGTM.

@michaelwoerister michaelwoerister force-pushed the mir-overloaded-fn-calls branch from bc51c40 to 545e9d2 Compare January 4, 2016 14:50
@michaelwoerister
Copy link
Member Author

Fixed the graphviz thing.

#[rustc_mir]
fn test_fn_object(f: &Fn(i32, i32) -> i32, x: i32, y: i32) -> i32 {
f(x, y)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we write a test that is not for a closure object?

@nikomatsakis
Copy link
Contributor

r=me with a non-object test as well

@michaelwoerister michaelwoerister force-pushed the mir-overloaded-fn-calls branch from 545e9d2 to 1f69493 Compare January 5, 2016 17:41
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 5, 2016

📌 Commit 1f69493 has been approved by nikomatsakis

@michaelwoerister
Copy link
Member Author

@bors r-

@michaelwoerister
Copy link
Member Author

I want to get rid of an unnecessary Substs clone quickly.

@michaelwoerister michaelwoerister force-pushed the mir-overloaded-fn-calls branch from 1f69493 to e281509 Compare January 5, 2016 18:03
@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Jan 5, 2016

📌 Commit e281509 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Jan 6, 2016

⌛ Testing commit e281509 with merge 8f7e5bb...

@bors
Copy link
Collaborator

bors commented Jan 6, 2016

💔 Test failed - auto-linux-64-opt

@michaelwoerister
Copy link
Member Author

@bors retry

@bors
Copy link
Collaborator

bors commented Jan 6, 2016

⌛ Testing commit e281509 with merge 7312e0a...

bors added a commit that referenced this pull request Jan 6, 2016
…komatsakis

So far, calls going through `Fn::call`, `FnMut::call_mut`, or `FnOnce::call_once` have not been translated properly into MIR:
The call `f(a, b, c)` where `f: Fn(T1, T2, T3)` would end up in MIR as:
```
call `f` with arguments  `a`, `b`, `c`
```
What we really want is:
```
call `Fn::call` with arguments  `f`, `a`, `b`, `c`
```
This PR transforms these kinds of overloaded calls during `HIR -> HAIR` translation.

What's still a bit funky is that the `Fn` traits expect arguments to be tupled but due to special handling type-checking and trans, we do not actually tuple arguments and everything still checks out fine. So, after this PR we end up with MIR containing calls where function signature and arguments seemingly don't match:
```
call Fn::call(&self, args: (T1, T2, T3)) with arguments `f`, `a`, `b`, `c`
```
instead of
```
call Fn::call(&self, args: (T1, T2, T3)) with arguments `f`, (`a`, `b`, `c`)  //  <- args tupled!
```
It would be nice if the call traits could go without special handling in MIR and later on.
@bors bors merged commit e281509 into rust-lang:master Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants