rust: remove all uses of unstable pragma "in_band_lifetimes"

[mcy]

Pull Request Overview

This commit removes the use of the unstable pragma "in_band_lifetimes" everywhere. The main culprit turned out to be the capsules crate.

Testing Strategy

This change is a noop, since it consits of either adding required lifetime annotations, or changing exlicit lifetimes 'a to placeholder liftimes '_. make allcheck should be sufficient to show this change is correct. In general, I tried to make the change that required minimal changes to the AST.

TODO or Help Wanted

N/A

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make formatall.

[gkelly]

Given that the tracking issue is making fairly slow progress (no substantive updates since 2017) and the feature's only being use as syntactic sugar it seems like this is a useful way to remove an unstable feature.

[mcy]

The placeholder lifetime '_ is the main substantive thing that came out of that RFC (which is just an unnamed in-band lifetime), so I think using it as much as possible is consistent with what the ecosystem has moved towards.

[bradjc]

This is a pretty big step backward for Rust usability. In particular, I find having to write impl<'a> sometimes is particularly confusing, and just adds clutter.

Do we know this feature is dead? What benefit does removing this give us?

[gendx]

It seems that indeed '_ can be used in most places, so removing this feature would also enforce a more consistent style (either use '_ as a placeholder, or declare a lifetime explicitly).

On the other side, this may make changes like #1628 (if we decided to implement them) more bloated.

Overall, I'm rather neutral to this change, but reducing the number of unstable features could be beneficial to the project.

[mcy]

@bradjc You need to write impl<'a> to introduce new lifetime parameters, just like with type parameters. Most Rust programmers working in stable expect to have to do this, anyway, and, in the majority of the cases I touched in my PR, '_ is sufficient. From what I know about Rust's ergonomics priorities, the full feature is unlikely to ever be stabilized; when we got '_ it seemed pretty clear that interest for this feature vanished.

This PR is part of a series I'm working on with @gkelly to make as much of Tock as possible compile under a stable Rust toolchain. In can set up a tracking issue to mirror a doc I have with all of the feature gates I'm hoping to remove.

[alevy]

Similar to #1650, I think this should go in a global tracking issue for unstable features.

[mcy]

Ask and you shall receive: #1654.

Sorry for the abrupt spray of PRs. I'm new to Tock, and I took enthusiasm on my first cleanup PR as a sign to move forward at full speed.

[pfmooney]

This is a pretty big step backward for Rust usability. In particular, I find having to write impl<'a> sometimes is particularly confusing, and just adds clutter.

I disagree. Considering the feature is enabled at the module level, rather than file, I found it extremely confusing that those lifetime declarations were necessary in some places, but not others. Without knowing the feature existed or was in use, it was difficult to reconcile that difference.

[ppannuto]

I also feel like this is a major step back in usability. It's fairly important that Tock be able to be accessible both to "Most Rust programmers working in stable" as well as embedded C programmers for whom this is their first exposure to Rust. I certainly cannot explain in one simple sentence to someone who has never seen Rust what '_ does nor when it needs to be applied. We need to keep both audiences in mind.

As I see it, currently the compiler is capable of figuring this out completely, as evidenced by the currently extant code base. Going back and manually adding a large number of <'_> or other annotations instead of letting the tool that is capable of doing it automatically do so feels like a step in strictly the wrong direction.

[ppannuto]

Considering the feature is enabled at the module level, rather than file, I found it extremely confusing that those lifetime declarations were necessary in some places, but not others. Without knowing the feature existed or was in use, it was difficult to reconcile that difference.

If the issue is that we are not using this consistently, I would argue (strongly) that we should add this to more places, rather than remove it.

[bcantrill]

For whatever it's worth, when we were new to Tock (but had done some Rust), the use of in_band_lifetimes was very confusing, in part because we wouldn't aware of the existence of the unstable feature at all! We had no idea why code that was working in one board was not working in another -- and it was only after multiple people and quite a few hours of narrowing it down that we discovered the feature. I would argue against this feature being used, as it seems unlikely to ever land in stable Rust. But if it is to be used, I would ask that every use have a comment above it explaining why the lifetime is elided...

[alevy]

Sorry for the abrupt spray of PRs

@mcy we all very much appreciate your contributions! And thanks for creating the issue!

[alevy]

If I can summarize the sides so far:

for

• Currently there is inconsistent use of in_band_lifetimes, making some code not work when copy/pasted across traits (e.g. board crates), and general inconsistent code style.

• The feature is unlikely to be stabilized and, thus, will be inconsistent with other Rust code.

against

• Adding additional impl<'a> declarations with no bounds everywhere is kinda gross.

• Replacing <'a> with <'_> in some places, where possible, is confusing and inconsistent.

[alevy]

Adding additional impl<'a> declarations with no bounds everywhere is kinda gross.

I agree it's better to let the compiler figure things out when it can, but it's this any different than extraneous impl<A>s?

In other words, it sucks that Rust is overly verbose in impl blocks, but lifetimes in particular is a relatively small delta.

[alevy]

Replacing <'a> with <'_> in some places, where possible, is confusing and inconsistent

Perhaps it would be better to avoid using <'a> in our code base, for consistency, unless there are individual cases where it really makes a big difference (e.g. @gendx's proposal to get rid if static for kernel structs)

[alevy]

The feature is unlikely to be stabilized and, thus, will be inconsistent with other Rust code.

This is really at the crux, imo.

If this is indeed unlikely to be stabilized, we probably can't reasonable go against typical use of Rust in the rest of the ecosystem in the long run and, despite whatever distaste we might have for this syntax choice, will just have to but the bullet.

Perhaps it's worth confirming the status of this feature for those of us who would prefer to keep the current syntax.

[gendx]

Replacing <'a> with <'_> in some places, where possible, is confusing and inconsistent.

I would say that <'a> could also be more confusing, e.g. when the lifetime isn't used such as here:

impl hil::time::Time for MachineTimer<'a> {

then the reader can wonder where 'a comes from. On the other hand '_ is clearly (provided the reader is familiar with this syntax) an anonymous lifetime placeholder coming from nowhere else.

---

A case where it could be confusing is when multiple lifetimes are involved, such as:

impl Iterator for Entropy8To32Iter<'_, '_>

If only the first lifetime is used, we could end up with syntax like follows:

impl<'a> Iterator for Entropy8To32Iter<'a, '_>

To be more specific, in the draft of #1628 I ended up with something like this:

impl<T: 'a + ?Sized> Deref for Borrowed<'a, '_, T> {

which would be the following without the unstable feature.

impl<'a, T: 'a + ?Sized> Deref for Borrowed<'a, '_, T> {

If that's too unclear, the two lifetimes could be named:

impl<'a, 'ker, T: 'a + ?Sized> Deref for Borrowed<'a, 'ker, T> {

[gendx]

Another thing that I noticed when working on #1628 is that in-band lifetimes are only allowed in some contexts. In the other cases, either a pre-declared lifetime or an anonymous lifetime had to be used.

I think it can add to the confusion of the unstable feature.

[alevy]

I think it can add to the confusion of the unstable feature.

Totally agreed but a reasonable remedy for this concern in particular would also be to enable the feature everywhere :)

[gendx]

I think it can add to the confusion of the unstable feature.

Totally agreed but a reasonable remedy for this concern in particular would also be to enable the feature everywhere :)

My bad, my comment wasn't clear enough.

What I meant is that even when in-band lifetimes are enabled, the Rust compiler doesn't allow one to declare in-band lifetimes in some syntactic contexts.

For example, the following code

#![feature(in_band_lifetimes)]

use core::marker::PhantomData;

pub struct A<'a> {
    foo: PhantomData<&'a ()>,
}

pub fn bar<F>(f: F) -> u32
where
    F: FnOnce(A<'a>) -> u32
{
    f(A { foo: PhantomData })
}

triggers the following error:

error[E0687]: lifetimes used in `fn` or `Fn` syntax must be explicitly declared using `<...>` binders
  --> src/lib.rs:11:17
   |
11 |     F: FnOnce(A<'a>) -> u32
   |                 ^^ in-band lifetime definition

Note: This example is a simplified version of what is found in kernel/src/sched.rs.

Instead, one can write either of the following, none of which require the in-band lifetime feature.

pub fn bar<'a, F>(f: F) -> u32
where
    F: FnOnce(A<'a>) -> u32

or

pub fn bar<F>(f: F) -> u32
where
    F: FnOnce(A<'_>) -> u32

Note that in the second case, just replacing 'a by the anonymous '_ was enough to make the compiler happy. So I'd say that when the lifetime isn't used, '_ is preferable to 'a, because the syntax is more stable and the compiler accepts it in more cases.

Overall, in-band lifetimes have a limited scope in terms of allowed syntax.

[ppannuto]

This was discussed on the core call today.

Major conclusions:

• We should start pushing towards stable Rust as policy.
• This does not currently prohibit the use of unstable features.
• However, features that we use should be on at least short-to-medium term path to stabilization.
• Priority for work on unstable feature removal by the core team will go towards the "fundamental" blockers (i.e. the new asm interface)
• However, we are happy to take in work from others towards the removal of unstable features that do not seem to be on the path to stabilization

---

What this means for this PR specifically: Once updated to current master, we should merge it.

[hudson-ayers]

Rebased on master, and also removed all in_band_lifetimes uses introduced from adding the stm32f3xx chip and from adding the apollo3 chip. cc @alistair23 this will probably cause some conflicts with your outstanding apollo3 PRs, sorry about that.

[bradjc]

Hopefully this change is pretty straightforward. I didn't actually look through all 117 files.

[ppannuto]

bors d+

As mentioned on #core in slack, this touches a huge number of files and will require frequent rebasing if it doesn't go through with some priority. Since it's really just a syntax (non-functional) change it should be reasonably safe. I'm going to go ahead an mark this last-call now as well, with plans to go ahead and merge end of day / early tomorrow.

[bors]

✌️ mcy can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

[ppannuto]

@hudson-ayers that d+ is really meant to say that you should go ahead and merge this roughly EoB today

[hudson-ayers]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)
• continuous-integration/travis-ci/push
• emulation-check