Fix `fn main() -> impl Trait` for non-`Termination` trait by leoyvens · Pull Request #50656 · rust-lang/rust

leoyvens · 2018-05-11T14:30:49Z

This bug currently affects stable. Why I think we can go for hard error:

It will in stable for at most one cycle and there is no legitimate reason to abuse it, nor any known uses in the wild.
It only affects bin crates (which have a main), so there is little practical difference between a hard error or a deny lint, both are a one line fix.

The fix was to just unshadow a variable. Thanks @nikomatsakis for the mentoring!

@nikomatsakis

Fixes rust-lang#50595. This bug currently affects stable. Why I think we can go for hard error: - It will in stable for at most one cycle and there is no legitimate reason to abuse it, nor any known uses in the wild. - It only affects `bin` crates (which have a `main`), so there is little practical difference between a hard error or a deny lint, both are a one line fix. The fix was to just unshadow a variable. Thanks @nikomatsakis for the mentoring! r? @nikomatsakis

nikomatsakis · 2018-05-14T14:56:51Z

r=me on the technical side. cc @rust-lang/core and @rust-lang/lang to inquire re: policy, but I think we should just land this. For those who haven't been following #50595, here is the background:

We stabilize the ability to use non-() returns types in main, but found shortly after the stable release that we now permit fn main() -> impl Debug -- or indeed any trait -- so long as the "hidden type" implements Terminate. You can rationalize this, but I think pretty clearly this behavior was not intentional nor particularly desired.
This PR fixes it and converts that into a hard error (fn main() -> impl Terminate does continue to work)

We can always convert to a lint or back out if it causes trouble, but it seems like it should be ok since we are acting very quickly and this is an obscure corner case.

eddyb · 2018-05-14T15:28:30Z

@nikomatsakis But fn main() -> Result<(), impl Debug> would continue to work, right?

eddyb · 2018-05-14T15:31:22Z

src/librustc_typeck/check/mod.rs

-    let ret_ty = fcx.instantiate_anon_types_from_return_value(fn_id, &ret_ty);
-    fcx.ret_coercion = Some(RefCell::new(CoerceMany::new(ret_ty)));
+    let revealed_ret_ty = fcx.instantiate_anon_types_from_return_value(fn_id, &ret_ty);
+    fcx.ret_coercion = Some(RefCell::new(CoerceMany::new(revealed_ret_ty)));


I think the original should be called declared_ret_ty and keep this one ret_ty. Since there is exactly one use of the original ret_ty and it's not even in this diff!

Indeed the diff is confusing because the line that changes behavior isn't on the diff. What do you think of renaming both?

What I wonder is: which naming would've had a better chance of preventing the bug? I think the name revealed_ret_ty calls attention to private information being used.

I think the code that has to see the type "from the outside" being here is part of the problem - after all, this part of typeck is working on the body of the function being checked. cc @nikomatsakis

That makes sense, what do you think of moving the check to the check_item_type pass, right here. I can try it in this PR, if we're not in a rush to land because of the point release.

joshtriplett · 2018-05-14T17:28:37Z

This seems reasonable to me.

@eddyb Perhaps we should add a test for that?

alexcrichton · 2018-05-14T21:40:43Z

@nikomatsakis this definitely isn't the first time we would "break" an API just after it was released, I think if we do a point release this is good to go

nikomatsakis · 2018-05-15T17:00:21Z

@eddyb I believe that will work fine

nikomatsakis · 2018-05-15T17:50:48Z

@bors r+ -- seems like consensus is to go forward

bors · 2018-05-15T17:50:49Z

📌 Commit 587566e has been approved by nikomatsakis

nikomatsakis · 2018-05-15T17:51:04Z

@bors r- -- oh, well, were you going to rename variables @leodasvacas ?

nikomatsakis · 2018-05-15T18:43:57Z

@bors r+

bors · 2018-05-15T18:43:59Z

📌 Commit 0582d02 has been approved by nikomatsakis

@nikomatsakis

…ret, r=nikomatsakis Fix `fn main() -> impl Trait` for non-`Termination` trait Fixes rust-lang#50595. This bug currently affects stable. Why I think we can go for hard error: - It will in stable for at most one cycle and there is no legitimate reason to abuse it, nor any known uses in the wild. - It only affects `bin` crates (which have a `main`), so there is little practical difference between a hard error or a deny lint, both are a one line fix. The fix was to just unshadow a variable. Thanks @nikomatsakis for the mentoring! r? @nikomatsakis

Mark-Simulacrum · 2018-05-17T01:26:02Z

@bors p=1 (Needed for beta, stable backports)

Rollup of 17 pull requests Successful merges: - #50170 (Implement From for more types on Cow) - #50638 (Don't unconditionally set CLOEXEC twice on every fd we open on Linux) - #50656 (Fix `fn main() -> impl Trait` for non-`Termination` trait) - #50669 (rustdoc: deprecate `#![doc(passes, plugins, no_default_passes)]`) - #50726 (read2: Use inner function instead of closure) - #50728 (Fix rustdoc panic with `impl Trait` in type parameters) - #50736 (env: remove unwrap in examples in favor of try op) - #50740 (Remove LazyBTreeMap.) - #50752 (Add missing error codes in libsyntax-ext asm) - #50779 (Make mutable_noalias and arg_align_attributes be tracked) - #50787 (Fix run-make wasm tests) - #50788 (Fix an ICE when casting a nonexistent const) - #50789 (Ensure libraries built in stage0 have unique metadata) - #50793 (tidy: Add a check for empty UI test files) - #50797 (fix a typo in signed-integer::from_str_radix()) - #50808 (Stabilize num::NonZeroU*) - #50809 (GitHub: Stop treating Cargo.lock as a generated file.) Failed merges:

@alexcrichton

[beta] Process backports Merged in master and accepted: * #50758: Stabilise inclusive_range_methods * #50656: Fix `fn main() -> impl Trait` for non-`Termination` trait r? @alexcrichton

rust-highfive assigned nikomatsakis May 11, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 11, 2018

cramertj added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 11, 2018

kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label May 14, 2018

eddyb reviewed May 14, 2018

View reviewed changes

emilyalbini mentioned this pull request May 14, 2018

Rust 1.26.1 point release #50756

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 15, 2018

Rename ret_ty to declared_ret_ty

0582d02

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 15, 2018

kennytm mentioned this pull request May 16, 2018

Rollup of 17 pull requests #50807

Merged

bors merged commit 0582d02 into rust-lang:master May 17, 2018

emilyalbini mentioned this pull request May 21, 2018

[beta] Process backports #50939

Merged

alexcrichton added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels May 21, 2018

Uh oh!

Conversation

leoyvens commented May 11, 2018

Uh oh!

nikomatsakis commented May 14, 2018

Uh oh!

eddyb commented May 14, 2018

Uh oh!

eddyb May 14, 2018

Choose a reason for hiding this comment

Uh oh!

leoyvens May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb May 16, 2018

Choose a reason for hiding this comment

Uh oh!

leoyvens May 16, 2018

Choose a reason for hiding this comment

Uh oh!

joshtriplett commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented May 14, 2018

Uh oh!

nikomatsakis commented May 15, 2018

Uh oh!

nikomatsakis commented May 15, 2018

Uh oh!

bors commented May 15, 2018

Uh oh!

nikomatsakis commented May 15, 2018

Uh oh!

nikomatsakis commented May 15, 2018

Uh oh!

bors commented May 15, 2018

Uh oh!

Mark-Simulacrum commented May 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

leoyvens May 14, 2018 •

edited

Loading

joshtriplett commented May 14, 2018 •

edited

Loading