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 `Instance::resolve()` incorrectly returning specialized instances #67662

Merged

Conversation

@wesleywiser
Copy link
Member

wesleywiser commented Dec 27, 2019

We only want to return specializations when Reveal::All is passed, not
when Reveal::UserFacing is. Resolving this fixes several issues with
the ConstProp, SimplifyBranches, and Inline MIR optimization
passes.

Fixes #66901

Rebased on top of #67631

r? @oli-obk

@wesleywiser wesleywiser force-pushed the wesleywiser:optimizations_handle_specialization branch from 9cb8cb7 to d88a745 Dec 27, 2019
@wesleywiser wesleywiser force-pushed the wesleywiser:optimizations_handle_specialization branch 2 times, most recently from 8525d2d to 4cfd5c4 Dec 27, 2019
@anp

This comment has been minimized.

Copy link
Member

anp commented Dec 27, 2019

@oli-obk I assume that closing #67631 implies this should also fix the test failure I have in #67137, but rebasing onto @wesleywiser's branch still yields a failure for me :(.

We only want to return specializations when `Reveal::All` is passed, not
when `Reveal::UserFacing` is. Resolving this fixes several issues with
the `ConstProp`, `SimplifyBranches`, and `Inline` MIR optimization
passes.

Fixes #66901
@wesleywiser wesleywiser force-pushed the wesleywiser:optimizations_handle_specialization branch from 4cfd5c4 to 25a8b5d Dec 27, 2019
@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Dec 27, 2019

Force pushed

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Dec 27, 2019

Hi @anp do you have the error details so I can look at them?

@anp

This comment has been minimized.

Copy link
Member

anp commented Dec 27, 2019

It's the same error that highfive reports:

------------------------------------------
stderr:
------------------------------------------
warning: due to multiple output types requested, the explicitly specified output file name will be adapted for each output type

warning: struct is never constructed: `PrintName`
  --> /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:10:8
   |
10 | struct PrintName<T>(T);
   |        ^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

warning: associated const is never used: `VOID`
  --> /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:13:5
   |
13 |     const VOID: ! = panic!();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: function is never used: `no_codegen`
  --> /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:16:4
   |
16 | fn no_codegen<T>() {
   |    ^^^^^^^^^^

error: any use of this value will cause an error
  --> /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:13:21
   |
13 |     const VOID: ! = panic!();
   |     ----------------^^^^^^^^-
   |                     |
   |                     the evaluated program panicked at 'explicit panic', /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:13:21
   |
   = note: `#[deny(const_err)]` on by default
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0080]: erroneous constant used
  --> /usr/local/google/home/adamperry/c/rust/src/test/mir-opt/retain-never-const.rs:17:13
   |
17 |     let _ = PrintName::<T>::VOID;
   |             ^^^^^^^^^^^^^^^^^^^^ referenced constant has errors

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.

------------------------------------------

failures:
    [mir-opt] mir-opt/retain-never-const.rs

test result: FAILED. 73 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 27, 2019

I guess we still need my PR for backwards compatibility of @anp's PR. I'll reopen

@wesleywiser

This comment has been minimized.

Copy link
Member Author

wesleywiser commented Dec 27, 2019

I have no idea why your PR would change that test's behavior but I wonder if we shouldn't have #[warn(const_err)] on that test.

@anp

This comment has been minimized.

Copy link
Member

anp commented Dec 27, 2019

I think oli said at one point that it would be a breaking change to allow const prop to surface this error?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 27, 2019

error[E0080]: erroneous constant used

this is the problem, the use of the broken constant inside a never used function should not cause a hard error.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 27, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 27, 2019

📌 Commit 25a8b5d has been approved by oli-obk

@bors bors merged commit 25a8b5d into rust-lang:master Dec 30, 2019
4 checks passed
4 checks passed
pr Build #20191227.30 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.