Skip to content

Use lifetime elision in impl headers for generated impls #44

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
wants to merge 2 commits into from
Closed

Use lifetime elision in impl headers for generated impls #44

wants to merge 2 commits into from

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Oct 23, 2018

Fixes #22

Said feature was finally stabilized in Rust 2015 as well and should be in the next nightly. This change makes the generated impls simpler to read (in the docs). However, this PR bumps the minimal Rust version required to use this crate. However, I think this is not a huge problem since we bump the minimum Rust version with the next release anyway.

Maybe we can still sneak this in for the 0.3 release.

Additionally, I made Travis compile the examples on beta as well. This is unrelated, but I'm bad at atomic PRs :P

But regarding Travis, I noticed something: we test all our examples and tests with Rust 2018. But it might be useful to also test them all compiled as Rust 2015? In particular in this situation where a feature was stabilized in Rust 2018 for some time already but is stabilized in Rust 2015 only later. However, I don't think the idea behind editions is that we all start testing all our crates for all editions (?). I'm not sure...

@LukasKalbertodt LukasKalbertodt changed the title WIP Use lifetime elision in impl headers for generated impls Oct 23, 2018
@LukasKalbertodt
Copy link
Member Author

Playground to reproduce the CI failure. So for beta:2018 we get this error. For nightly:2018 it works fine. For *:2015 it still complains about the missing lifetime (as the lifetime elision feature is not stabilized there yet).

Since I'm not really sure if this error is here to stay, I would just wait for the next beta release and test again then. So this is PR is blocked until we figured that out.

@LukasKalbertodt LukasKalbertodt removed the request for review from KodrAus October 23, 2018 12:43
@@ -17,43 +17,32 @@ use syn::{
/// changed.
const PROXY_TY_PARAM_NAME: &str = "__AutoImplProxyT";

/// The lifetime parameter used in the proxy type if the proxy type is `&` or
/// `&mut`. For more information see `PROXY_TY_PARAM_NAME`.
const PROXY_LT_PARAM_NAME: &str = "'__auto_impl_proxy_lifetime";
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@KodrAus
Copy link
Member

KodrAus commented Oct 25, 2018

Maybe we can still sneak this in for the 0.3 release.

Sounds good! I'm pretty comfortable pushing the minimal rustc version right up to the latest stable for our next release.

But regarding Travis, I noticed something: we test all our examples and tests with Rust 2018. But it might be useful to also test them all compiled as Rust 2015?

I think we should be ok just testing against the 2018 edition, since a 2015 edition crate should be able to depend on a 2018 edition one without knowing what edition it came from. If we want to start out cautious to start with, and maybe catch any errors for upstream Rust then we could add a dummy 2015 crate for tests that depends on auto_impl?

@KodrAus
Copy link
Member

KodrAus commented Oct 25, 2018

It looks like lifetime elision has stabilized for 1.31, so we should be a few hours away from a new beta that supports it :)

@LukasKalbertodt
Copy link
Member Author

I think we should be ok just testing against the 2018 edition, since a 2015 edition crate should be able to depend on a 2018 edition one without knowing what edition it came from.

Yeah this is true for most crates, but since we are generating code that will be compiled as (potentially) 2015 edition, we need to make sure the code we generate works on both editions. Right? For example now, the Travis-CI error has nothing to do with the missing lifetime elision feature. As far as I understand it, all our examples are compiled with edition 2018, so the lifetime elision code we generate works there (for many weeks already).

add a dummy 2015 crate for tests that depends on auto_impl?

That sounds good. Maybe I'll still add this to this PR. I think I can just create a folder in the examples folder with a Cargo.toml in it... in the back of my head I remember something about a feature like that :D

Beta can't run the test suite, but we can at least compile the
examples.
Fixes #22

Instead of generating `impl<'a, T> Foo for &'a T` we now generate
`impl<T> Foo for &T`. This works in newer versions of Rust. See
rust-lang/rust#15872 for more information.

This change makes a lot of other code useless. In particular, we don't
need to find a special lifetime parameter name anymore. Everything was
adjusted to remove this useless code.
@LukasKalbertodt
Copy link
Member Author

I just merged #43 and wanted to rebase this PR on those changes, not expecting anything bad to happen. But actually, both changes together lead to problems (and btw: huzzaa, our test suite saved us =D). Specifically:

trait Supi {}

#[auto_impl(&)]
trait Foo: Supi {}

... generates:

impl<T: Foo> Foo for &T where &T: Supi {}

And this is not allowed. sigh.

So we could:

  • Just not merge this and always have lifetime parameters in impl blocks.
  • Only generate lifetime parameters when the trait has a super trait.

And to be honest, right now, I tend to just not merge this PR and keep it the way it is. If we go with the second solution, we end up with more code than before (we can't delete any of the "find a lifetime name" code, but we need to add code for checking). And that only for the benefit of slightly more readable impl blocks for & and &mut proxy types. I'm not sure if that's worth it.

What do you think?

@KodrAus
Copy link
Member

KodrAus commented Oct 30, 2018

Yeh, I think this nice in-so-far-as it reduces complexity in our implementation, so if we have to work harder than before then I think it loses its value a bit.

@LukasKalbertodt
Copy link
Member Author

This PR is already quite old and I don't think it's worth adding this. So I will close it for now.

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.

2 participants