refactor: Adapt most crates to unconditionally use #![no_std]

[polarathene]

This minor refactor across crates is at the request of @robertbastian to PR the relevant changes.

I've tried to make it a bit easier to review by scoping related changes across commits.

I've additionally revised the boilerplate docs, and shuffled some extern imports around. There were some minor changes I made that I'm assuming are valid but as I'm not an actual user of any of these crates I'm deferring that to test coverage 😅 (I'll highlight these via review comments)

Any crate with boilerplate for no_std commented out has been left that way as I assume there is implicit reliance upon the std prelude and I lack time to pursue updating those crates. I have documented that discrepancy at the end of the boilerplate docs, should anyone wish to tackle that going forward 💪

I did all these changes manually (well where possible with the editor search find/replace functionality), but did consult Gemini to assist as peer review when revising my wording in the boilerplate docs.

NOTE: I've not been able to follow the contributor checklist. As I'm pressed for time and my current system is at 77/88GB of committed memory, I've pieced this PR together via the github.dev web editor (now rebranded as vscode.dev). It may not be perfect but at least outlines the scope of change.

Changelog (N/A)

[CLAassistant]

All committers have signed the CLA.

[polarathene]

The bulk of the PR is effectively:

-#![cfg_attr(not(any(test, doc)), no_std)]
+#![no_std]

+#[cfg(test)]
+extern crate std;

or:

-// #![cfg_attr(not(any(test, doc)), no_std)]
+// #![no_std]

With the removal of extern crate std from locations where it was added at the crate root instead. And additionally moving extern crate alloc to be declared earlier in a file as a convention.

Any other difference has an attached comment for context 👍

[polarathene]

I don't see a need for the target_arch = "wasm32" constraint for std, or the need to rename it.

With #![no_std] always being present now, there will be no std prelude implicitly available, which I assume this workaround was for to ensure the use of the core types perhaps? Whereas now an explicit std would be required otherwise. I didn't see any references for rust_std so I assume it is no longer used.

This rust_std rename was introduced in Aug 2021 (when the capi crate declared #![no_std] unconditonally). Later in Sep 2022 with Rust 1.64, core gained the FFI type aliases (satisfying c_char) and alloc gained CString. However by this time it seems the wasm_glue.rs file had already dropped these lines.

Reference: File history is a bit messy to track for wasm_glue.rs, but here's the relevant outline proceeding that Aug 2021 PR:

• Sep 2021: ffi/capi/src/wasm_glue.rs renamed to ffi/diplomat/src/wasm_glue.rs
• July 2022: All rust_std lines dropped
• Aug 2023: Further slimming as the target becomes feature gated and code is shifted (PR).
• Oct 2023: Renamed again in Oct 2023 (ffi/diplomat/src/wasm_glue.rs => ffi/capi/src/wasm_glue.rs). The file was then shortly after removed in a PR bumping the diplomat dep. Using diplomat-runtime for WASM logging.

[Manishearth]

Yeah, I think this makes sense

[robertbastian]

I believe what we had previously was equivalent to not(target_arch = "wasm32") – we didn't want to use std on wasm32. IIRC it's a weird target as it has a partially implemented std, so we renamed it to not accidentally use it, even if the std feature is enabled.

[polarathene]

This is similar to the WASM workaround with rust_std and other crates that had adopted that convention previously.

This file was introduced via this March 2022 PR where it extracted out diplomat/src/freertos_glue.rs to ffi/freetos/src/lib.rs and the extern crate std as rust_std line with constraint was added in a follow-up April 2022 PR. It's ambiguous what was considered "rust runtime stuff" and no context was given or discussed in the PR.

Like with the other review comment regarding WASM, this workaround should not be necessary, as std won't implicitly be available with explicit unconditional #![no_std]. It is now only in the scope of of cfg(test) where it's presumably needed. Should there ever be a need for std outside of that it can be feature gated as expected 👍

[polarathene]

I might be making a mistake here, presumably with the --all-features flag for CI testing? (the constraint to prevent target_os = "none" even with the std feature gate is presumably important for testing only?)

Given the use of std, the reliance upon alloc didn't seem correct here, so I swapped that for the std feature that is already in use. I've not looked through git blame history to understand the motive of why the alloc feature was used instead but I assume the std feature arrived later?

Since the conditions were already wrapped in an all() predicate, I split them onto separate lines, there wasn't any meaningful grouping I could infer to keep the nested all().

[Manishearth]

No, the alloc feature arrived later. Not sure what happened here.

[Manishearth]

and the target_os = none is loadbearing I think. We ship to freertos.

I don't recall the details here though. It's certainly strange that we're exposing items from std like this

[polarathene]

Likewise, the nested all() didn't seem to be of value and added friction to groking the difference, instead I just mirrored the std variant conditionals to be wrapped in not().

That prior all(not(all(not(target_os = "none")))) may still be relevant, but could be better expressed on a separate line as just target_os = "none", if it is needed for tests?

[Manishearth]

FWIW I would recommend not changing cfg gates like this in this PR, or at least not in the same commit. It is much harder to review if I have to go looking at each line to ensure I have noticed these cases.

This is currently a behavior change since it removes the target_os thing. This type of PR should not change public API behavior, even if it is unintentional.

[polarathene]

This point seems a bit outdated. Perhaps the boilerplate had relevant lints in the past, but now there is a mismatch as some of these are delegated to the workspace Cargo.toml.

I didn't drop these descriptions to defer to review feedback. In the meantime I added a note below to add context to the reader that other lints are configured at the workspace and these are purely workaround lints, with an issue link to associate them to.

[Manishearth]

Yeah we should clean this up. Probably worth doing separately.

[polarathene]

Since the change proposed by this PR does introduce some distance from the extern line in some files, I figured this was worth documenting.

I've adjusted source in crates to elevate any extern crate alloc to the top, and where extern crate std is present it made sense to have it occur after. If this extra guidance seems redundant feel free to drop it 👍

[Manishearth]

This is a good idea

[polarathene]

This is revised from the original line on the same topic. I'm not sure if you'd want a extern crate std added when the crate isn't adjusted to use or require it for anything else.

Keeping a // #![no_std] line at least makes it easy to filter a search query for until such crates are adapted to not rely on an implicit std prelude.

[Manishearth]

I'm not sure if you'd want a extern crate std added when the crate isn't adjusted to use or require it for anything else

Yes, please don't add extern crate std where not needed.

[polarathene]

Yes, please don't add extern crate std where not needed.

Just to clarify, the intention was on files that already have // #![no_std] (or the equivalent prior boilerplate definition), if you uncomment that line you would need extern crate std to be present and likely make adjustments.

The only motivation for such would be for consistency across crates, which conventionally for someone working across them might be saner. Some types that exist in both core/alloc and std however do have additional methods/support in their std variant, being aware of those differences can be useful in std only crates where those features could be leveraged.

Either way, I've not introduced changes like that to those crates functionality.

[polarathene]

This is just documentation for better awareness of why it's done this way, as it seems in the past there's been some switching between the usage and workarounds without the understanding at the time about issues experienced.

I provided more detail about why the change proposed by this PR is the better approach over in this issue comment of mine.

[polarathene]

I'm not entirely confident about this advice, but my understanding is that for tests not in a tests/ directory, you may need to ensure std is present, so that is always toggling it. While doc may only be relevant if the crate actually uses std itself?

NOTE: You have a crate (provider/blob/src/lib.rs) that uses export as the feature name instead of std. I've not mentioned that here, and I'm not sure if there's any desire/intent to adjust that feature gate for the crate 🤷‍♂️

The later bullet points in this admonition is for the current crates incompatible with running without std, while none are presently configured to use #![no_std] they could presumably add extern crate std without any annotation and be fixed up to explicitly reference std where necessary. Presently all those crates have // #![no_std] but once they've been adapted, there might be some misunderstanding at a glance about their ability to support no_std? 🤷‍♂️ (or maybe it's not a concern to worry about)

Since the line may be further apart from the boilerplate snippet and have no annotation to communicate the std-only support, someone may likewise think it's redundant without a contextual comment, but reviewers and CI would likely address such misunderstandings. Since the std prelude won't be available, I don't think they'll be any real issues that trip up any contributor besides mandatory imports from std.

[Manishearth]

While doc may only be relevant if the crate actually uses std itself?

doc is needed for intra-doc links. But also I'm not entirely sure what you're asking in this paragraph, can you be a bit more explicit about your antecedents?

NOTE: You have a crate (provider/blob/src/lib.rs) that uses export as the feature name instead of std. I've not mentioned that here, and I'm not sure if there's any desire/intent to adjust that feature gate for the crate 🤷‍♂️

Please do not change public feature sets. This is intentional; blob's export functionality is not needed by normal ICU4X users.

In general, we have ICU4X runtime code which is expected to be no_std, and "datagen", which has no such expectation. Datagen crates don't have no_std.

[polarathene]

doc is needed for intra-doc links. But also I'm not entirely sure what you're asking in this paragraph, can you be a bit more explicit about your antecedents?

Presently you have crates configured with:

#![cfg_attr(not(any(test, doc)), no_std)]

So if building docs, no_std is not used and std as an implicit prelude is used. My understanding is this was only meaningful if the crate had something related to std to document? (as in the crate actually uses std beyond tests)

As such any crate that had no std or equivalent feature to opt-in to std presumably generates docs fine with #![no_std], without needing annotation to enable std during docs generation?:

#[cfg(doc)]
extern crate std;

I could add that constraint for all uses regardless, like I had done with cfg(test) which from what I understand is necessary for cargo test to work (not verified but I've seen various resources online have a problem with it for unit tests but not integration tests), it just was not clear for cfg(doc).

---

Please do not change public feature sets. This is intentional; blob's export functionality is not needed by normal ICU4X users.

I have not done that AFAIK.

In general, we have ICU4X runtime code which is expected to be no_std, and "datagen", which has no such expectation. Datagen crates don't have no_std.

I specifically only targeted crates with the boilerplate present.

[Manishearth]

So if building docs, no_std is not used and std as an implicit prelude is used. My understanding is this was only meaningful if the crate had something related to std to document? (as in the crate actually uses std beyond tests)

Yes, this means that an intra doc link, like [std::rc::Rc], will work in the markdown docs.

I think it's fine to unconditionally enable std in doc mode.

I'd be careful here, because broken intra-doc links do not always cause CI errors. They certainly won't cause CI errors in anythng but a rustdoc build, too.

I have not done that AFAIK.

I know, I wanted to ward you off considering that as an option 😄

I specifically only targeted crates with the boilerplate present.

Good to know, thanks!

[polarathene]

Oh awesome... I'll go over the Clippy lint failures tomorrow I guess. I'm not sure if I'll be able to do a clone locally to run and resolve lints any time soon, I really don't want to risk exceeding the memory commit ceiling on my system and triggering an OOM crash elsewhere right now 😓

If anyone would like to take over this PR they're welcome to.

[Manishearth]

@polarathene I would recommend splitting this into smaller sets of changes: one with the noncontroversial cleanups (where crates that do follow the boilerplate are updated to the better boilerplate), and one with the changes that are likely to actually affect CI.

[Manishearth]

Thanks for this PR, though! Working towards more consistency here is good.

[robertbastian]

this used to be any(test, doc) because we did need this somewhere

[robertbastian]

I believe what we had previously was equivalent to not(target_arch = "wasm32") – we didn't want to use std on wasm32. IIRC it's a weird target as it has a partially implemented std, so we renamed it to not accidentally use it, even if the std feature is enabled.

[sffc]

The main thing to avoid is: a no_std crate has extern crate std; and then someone checks in code using an std-specific thing that looks benign, something like io::Write. To avoid this, we only enable std in test mode and when the feature is enabled. Relying on human or AI review is not sufficient.

[Manishearth]

To avoid this, we only enable std in test mode and when the feature is enabled.

We have a nostd CI job that is expected to fail if you accidentally enable std anywhere in the "core" ICU4X tree.