Skip to content

MIR inlining inlines default trait methods #40473

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

Closed
arielb1 opened this issue Mar 13, 2017 · 8 comments
Closed

MIR inlining inlines default trait methods #40473

arielb1 opened this issue Mar 13, 2017 · 8 comments
Assignees
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Mar 13, 2017

STR

$ cat mirline.rs
pub trait Foo {
    fn bar(&self) {}
}

impl Foo for () {
    fn bar(&self) { println!("Hello, World!"); }
}

pub fn main() {
    ().bar();
}
$ rustc mirline.rs -Z mir-opt-level=2
$ ./mirline

Expected Result

Code should print "Hello, World!"

Actual Result

Code does not print anything, because the empty default implementation is inlined.

cc @Aatch

@arielb1 arielb1 added I-wrong regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 13, 2017
@wesleywiser
Copy link
Member

Is this just a matter of preventing the inliner from inlining trait default methods?

@arielb1 arielb1 added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Mar 16, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Mar 16, 2017

1.17 is now beta.

@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 16, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

Marking this as medium because it requires you to opt-in to the higher "MIR opt level", which is unstable, but we should definitely fix this.

@rust-highfive rust-highfive added the P-medium Medium priority label Mar 16, 2017
@arielb1 arielb1 added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Mar 23, 2017
@brson
Copy link
Contributor

brson commented Apr 11, 2017

cc @Aatch @nikomatsakis just pinging for an update.

@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Apr 26, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Apr 27, 2017

1.17 is now stable. This is a nightly -> nightly regression affecting only -Z mir-opt-level.

@arielb1 arielb1 added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 27, 2017
@Mark-Simulacrum Mark-Simulacrum added A-specialization Area: Trait impl specialization and removed A-specialization Area: Trait impl specialization labels Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added I-nominated C-bug Category: This is a bug. and removed regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-wrong labels Jul 27, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated P-medium Medium priority labels Aug 24, 2017
@nikomatsakis
Copy link
Contributor

I have been talking to @qmx, who is going to take a look at implementing this. I'll leave some mentoring instructions shortly!

@nikomatsakis
Copy link
Contributor

So #44383 will fix this by just not inlining trait calls -- the simplest possible fix. I just opened #44389 as an enhancement issue to track a more complete fix.

bors added a commit that referenced this issue Sep 11, 2017
…sakis

MIR: should not inline trait method

Fixes #40473.

The idea here is bailing out of inlining if we're talking about a trait method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants