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

avoid a rustc bug #230

Merged
merged 2 commits into from
Aug 1, 2022
Merged

avoid a rustc bug #230

merged 2 commits into from
Aug 1, 2022

Conversation

aliemjay
Copy link
Contributor

Hi,

rust-lang/rust#98835 fixes a rustc bug that makes it incorrectly accepts the following code:

fn test<'a: 'a>() { // 'a is longer than the entire function body
    let f = |_: &'a str| {}; // expects an argument that lives as long as 'a
    f(&String::new()); // the passed reference is shorter that `'a`
}

The crater run found that innernet will break with this change because it indirectly relies on this bug. I'm not sure when this change reaches stable, if ever, but it's better to not rely on such a bug. This PR tries to fix this.

If you have an opinion on this change, please let me know here or by commenting on the PR.

You can install and test the toolchain locally by running the command: rustup-toolchain-install-master cb1f7c96119295882e08b09b4bd01051669c071b

self.cidrs
.iter()
.filter(move |c| c.parent == Some(self.contents.id))
.map(move |c| Self {
.map(move |c| CidrTree {
Copy link
Contributor Author

@aliemjay aliemjay Jul 29, 2022

Choose a reason for hiding this comment

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

The reason for this change is that Self (which is just a sugar for CidrTree<'a>) brings the the lifetime 'a to the scope of the returned opaque type. Use CidrTree instead to allow rustc to infer a shorter lifetime (not necessarily 'a).

Generally, it's recommended to use the struct name instead of Self to allow rust to infer the appropriate lifetime rather than unnecessarily forcing a specific lifetime on the created opaque type.

The error message maybe helpful: https://crater-reports.s3.amazonaws.com/pr-98835/try%23cb1f7c96119295882e08b09b4bd01051669c071b/gh/tonarino.innernet/log.txt

Copy link
Member

Choose a reason for hiding this comment

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

Oh this is an interesting one! I've often used Self in places like this, good to know the kind of issues that can cause.

Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

This looks good to me. @mcginty @strohel what do you think?

Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

This is an interesting one! It also didn't occur to me that saying Self could bring an unwanted lifetime bound in. Approving.

@aliemjay can you please rebase on main before we merge this? Or I can do that if I have force-push permissions to your branch. We've fixed clippy warnings in the mean time, so that should make the CI green.

@aliemjay
Copy link
Contributor Author

aliemjay commented Aug 1, 2022

I'm on mobile now, so I can only use github ui to merge the branch, if this is not suitable, feel free to close this and commit the change yourself.

@strohel
Copy link
Member

strohel commented Aug 1, 2022

@aliemjay oh that works, too. Merging this will rebase the merge away. Thanks!

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.

3 participants