bump Rust nightly to nightly-2024-05-23 and set optimize_for_size

[alistair23]

Pull Request Overview

bump Rust nightly to nightly-2024-05-23

Bump to nightly-2024-05-23 of Rust. This includes:

• a few clippy fixes
• Removing mut from &mut self that don't need it
• Removing dead code hidden behind non-existant cfg

Also set a feature flag in the core library that aims to produce smaller implementations for certain algorithms. See rust-lang/rust#125011 for more details.

Testing Strategy

CI

TODO or Help Wanted

Documentation Updated

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

Formatting

• Ran make prepush.

[alistair23]

Still waiting on the flag to propagate through. Should hopefully be here in a day or two

[lschuermann]

Looks good, modulo the removed code.

At some point we ought to do a pass and remove all these feature flags in favor of reflecting this through type-level mechanisms (e.g., pluggable generics and corresponding trait implementations).

[alevy]

@alistair23 to merge this, we need to have a clean change that doesn't "sneak in" any other changes that would merit separate discussion. At this point I think that's just removing the cfg's in the lowrisc chip crate (the comment in the stm32 chip Cargo.toml is possibly inaccurate, but oh well) should be reverted, in the same manner as the STM32 cfgs were restored.

Separately, on those changes, it's important to recognize that not all code that uses this repository is in this repository----that is both a reality and desirable---so there are very well may be users of code that appears dead in the sense that no code in this repository exercises/enables it.

[ppannuto]

so there are very well may be users of code that appears dead in the sense that no code in this repository exercises/enables it.

Separate from this PR, but that's something we should probably discuss at some point. Especially once the automated, federated CI is online, having 'forever uncovered' code feels like it isn't a great system. (This is to say, if it's used externally, and we're maintaining dead-looking code to avoid folks having to carry out-of-tree patches, there should be something external checking to make sure that other changes here don't break the dead-looking stuff).

---

For the immediate term, however, I agree with leaving the status quo, such as it is.

[lschuermann]

@ppannuto I agree, but I do think that the problems are exacerbated by the fact that this is hidden behind a conditional compilation flag. We're much more confident in, and have made better experiences with, switching between different code paths at compile-time with type-system based approaches. Rust's expressive type-system and Tock's use of it allows us to catch a great many types of issues even without runtime tests, but we don't even have that with conditional compilation.

[alistair23]

Separately, on those changes, it's important to recognize that not all code that uses this repository is in this repository----that is both a reality and desirable---so there are very well may be users of code that appears dead in the sense that no code in this repository exercises/enables it.

I have re-added the aon_wkup_timer cfg.

I also updated the Tock docs to clarify that custom cfgs and feature cfgs are encouraged in Tock when there are possible users in forks. That hopefully avoids any confusion in the future when people wonder why these features/cfgs exist. As the Rust warnings can be a little confusing I also pointed to some Rust docs for hiding the warnings, which is what I have to do in this PR.

I still think this is a really bad idea to keep, espicially when there is no way for us to enable it without adding a custom build.rs (I think it can also be enabled in .cargo/config but that still requires code changes).

@ppannuto I agree, but I do think that the problems are exacerbated by the fact that this is hidden behind a conditional compilation flag. We're much more confident in, and have made better experiences with, switching between different code paths at compile-time with type-system based approaches. Rust's expressive type-system and Tock's use of it allows us to catch a great many types of issues even without runtime tests, but we don't even have that with conditional compilation.

It's not a conditional compilation flag though. It's not even a Cargo feature. It requires a special build.rs to enable it. I guess depending on how the Tock code is pulled in a out-of-tree user can enable this, but it feels really strange.

[lschuermann]

I don't want to block on this further and I don't think that this is the right place to discuss these issues, but my 2ct:

• It requires a special build.rs to enable it. I guess depending on how the Tock code is pulled in a out-of-tree user can enable this, but it feels really strange.

  I think in this case, these things really ought to be features defined in Cargo.toml. I agree that no cfgs should be used that don't have appropriate features defined in the crate config.

• I also updated the Tock docs to clarify that custom cfgs and feature cfgs are encouraged in Tock when there are possible users in forks.

  This information is worth adding to the docs, but I wouldn't go as far as to encourage it. It's the status quo, but we ought to work to get rid of these flags in lieu of type-system based approaches. That's not for this PR to implement, though.

[alevy]

Actionable: @alistair23 please just exclude the updates to docs. This can/should just be a clean upgrade nightly, and otherwise what you've done is awesome. If you just exclude the most recent doc commit, I will hit merge immediately.

Re: @lschuermann's comments (and @alistair23's position) I agree that these cfgs are a bad way of solving this problem. Feature flags would be better, and a generic would be even better. I think it absolutely makes sense to discourage both cfgs in favor of feature flags and discourage feature flags in favor of generics whenever possible, including for the stm32s.

Just in a separate discussion/PR that would solicit insights from the people involved with those chips.

[alistair23]

I don't want to cement the cfgs into the code base without updating the code style. It feels like I'm sneaking in policy changes.

I opened #4012 to split the doc change out. Let's just clarify that this is desirable behaviour from a Tock perspective before I end up being the person git blamed for solidifying all of these unused compile time configs.

[alevy]

It is not desirable!

But it is a separate discussion, that actually requires some feedback from anyone who put these in or may have insight about these chips, not something to just remove in an update of nightly.

[alistair23]

You said earlier that it is desirable, sorry if I misinterpreted that

I have CCed Wilfred who added the #[cfg(aon_wkup_timer)] originally

[alevy]

If you're worried about who gets git blamed I am happy to cherry pick those changes into a separate commit that blames me with an appropriate explanation in the commit message.

We definitely do not want to solidify a discouraged practice in the docs, and we also don't want to remove exported functionality in a commit who's title is about updating nightly in a PR that is about updating nightly.

There is really no disagreement here about what is better to have in the code, just about the appropriate place and way to change it.

[alistair23]

There is really no disagreement here about what is better to have in the code, just about the appropriate place and way to change it.

Wait, if the issue is just that you want a separate PR to remove the dead code I can just open a separate PR to remove it.

That wasn't my original interpretation of what you were asking

[alevy]

I just want a separate PR to deal with the "potentially" dead code.

One option is to just remove the cfg and code, but we'd want to do some sue dilligence so as not to lose some important value it might provide.

[alistair23]

Easy! PR opened to remove the dead code

[alevy]

Ok, thanks, but we still need to remove the doc change in this PR to merge it (remove the last commit).

I think it's likely for the new PR to involve a fair amount of detective work to (a) figure out whether that code is useful and (b) decide how to deal with it if it is (removing it is probably not the right move if it's useful, even if keeping it as cfgs is also not good).

[alistair23]

Let's just wait on either #4012 and/or #4013 to be merged then I can rebase this. It shouldn't take that long

[bradjc]

Just a note, if we merge this we need to update #1654.

[alistair23]

Ok! Dead code is removed, no policy changes are required this is ready to go

@bradjc this doesn't stop us using stable Rust (although it might result in larger binaries). Hopefully someone can just update #1654 once this goes in to clarify that we use this feature with nightly

Fixed