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

impl Trait is allowed in the return type of main #50595

Closed
leodasvacas opened this Issue May 10, 2018 · 12 comments

Comments

Projects
None yet
8 participants
@leodasvacas
Copy link
Contributor

leodasvacas commented May 10, 2018

we’re stabilizing the combination of features that allows you to put impl Trait in the return type of main as in fn main() -> impl Copy {}. Amusing but unintended so it’s better to forbid this for now.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented May 10, 2018

It's a bit late to change this. Does it cause any problem? AFAICT it just gives you a weird way to declare main, but no new capability.

@leodasvacas

This comment has been minimized.

Copy link
Contributor Author

leodasvacas commented May 10, 2018

It doesn't give any new capability, I guess impl Termination would even make sense in the return type of main. This bug could be considered a feature.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 10, 2018

@earthengine

This comment has been minimized.

Copy link

earthengine commented May 10, 2018

Why this is an issue? main returns () and () implements Copy.

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented May 10, 2018

impl Copy does not implement Termination, which is required for the return type of main.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented May 10, 2018

Yea, it seems you can use any trait:





trait Goo {}
impl Goo for () {}
fn main() -> impl Goo {}

Works on beta and nightly. So it'll probably work on this weeks stable

You can't do anything stupid with this feature. The checks see right through the impl Trait to the real type (http://play.rust-lang.org/?gist=243499b4b1e837bc41905ac814bb9a4b&version=nightly&mode=debug). I'd say adding a regression test that ensures that this corner case essentially ignores the anonymization of impl Trait is fine. We can add a deny by default lint that tells you that while this works, it's not pretty.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented May 10, 2018

This seems fine to me and unless we care enough to stop the stable release we're not going to change this. cc @rust-lang/lang though.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 10, 2018

Hmm, I feel like we should just fix this. I doubt there are a lot of people taking advantage of it. I can put up some mentoring instructions. We can do a warning period if needed.

@leodasvacas

This comment has been minimized.

Copy link
Contributor Author

leodasvacas commented May 10, 2018

Fwiw I tried fixing by doing this and got ICEs and stack oveflows which I couldn't debug so I gave up.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 10, 2018

@leodasvacas

Fwiw I tried fixed this doing this and got ICEs and stack oveflows which I couldn't debug so I gave up.

Hmm, I don't understand the stack overflows, but I think I would expect that patch to have no effect. The cause of the bug is that we are "instantiating" the impl Trait, here:

let ret_ty = fcx.instantiate_anon_types_from_return_value(fn_id, &ret_ty);

In the process, we shadow the "original" ret_ty with a new variable ret_ty. This new ret_ty will contain an inference variable used to find the "hidden" type underneath the impl Trait.

Later on, when we add the obligation that R: Terminate (where R is the return type), we use this new ret_ty variable:

let substs = fcx.tcx.mk_substs(iter::once(Kind::from(ret_ty)));

If we used the original (currently shadowed) ret_ty, I think it would permit you to use -> impl Terminate but not other impl traits.

(That said, it wouldn't be the end of the world if we just let this go, I suppose.)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 10, 2018

As far as I understand we don't want to block 1.26 stable on this, right? In that case I think we should go with a lint instead of an hard error, since both of the features will be stabilized tomorrow now.

@leodasvacas

This comment has been minimized.

Copy link
Contributor Author

leodasvacas commented May 10, 2018

@nikomatsakis I probably borked the state of my clone or something. Thanks for the explanation, makes perfect sense, will make a patch.

leodasvacas added a commit to leodasvacas/rust that referenced this issue May 11, 2018

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

kennytm added a commit to kennytm/rust that referenced this issue May 16, 2018

Rollup merge of rust-lang#50656 - leodasvacas:fix-impl-trait-in-main-…
…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

@bors bors closed this in #50656 May 17, 2018

pietroalbini added a commit to pietroalbini/rust that referenced this issue May 21, 2018

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 added a commit to Mark-Simulacrum/rust that referenced this issue May 24, 2018

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.