Make wit-parser support no_std

[wilfreddenton]

#2414

I followed a pattern similar to #2401 but this crate required more adjustments and was not as straightforward.

Currently the work depends on my fork of id-arena which updates the no_std implementation. PR for that is here: fitzgen/id-arena#20. It's a very simple fix. Additionally I updated the workspace anyhow dep to default-features=false which didn't seem to cause any issues.

This work maintains the existing API without any breaking changes but puts the functions behind std flags. New methods added:

1. SourceMap::push_str(&str, contents) - no_std version of push(&Path, contents)
2. SourceMap::source_names() - no_std version of source_files() (returns &str instead of &Path)
3. UnresolvedPackageGroup::parse_str(path: &str, contents: &str) - no_std version of parse(&Path)
4. Resolve::push_group(UnresolvedPackageGroup) - already existed but now works in no_std
5. Resolve::push_source(&str, &str) - no_std version of push_str (which uses Path)

The existing std versions delegate to these new no_std versions.

Path was not really being "used" as just .display() was being called on it so &str/String are not a bad replacement in general.

As IndexSet and IndexMap are part of the "interface" of the crate I could not swap them out to hashbrown or BTree*. Instead I just use a different hash build (S) for the different "envs" (ahash::RandomState for no_std) and switched to the default constructor instead of new.

I've added a check in the github CI:

cargo check --no-default-features -p wit-parser --target x86_64-unknown-none

If an alternate strategy seems better I am happy to try that out. Thanks!

[alexcrichton]

Thanks for this! I think this is reasonable to support, but will require a bit of care in terms of ensuring the crate doesn't suffer too much in terms of maintainability burden. Everything here looks mostly on the right track, but wanted to leave a few comments.

For Path, I agree with the changes you've made, and I'd actually say this could go 1 step further of doubling down on "the source of truth is String". That may actually already be the case (sorry only skimmed so far), but wanted to confirm that making Path things a thin wrapper around str things I think is totally reasonable.

For maps, I think we may want a bit of care there. The wasmparser crate deals with a very similar problem in this regard and would probably be a good place to draw inspiration from. Personally I like the ability to keep a HashMap without forcing usage of BTreeMap, so I'd like to preserve that if possible. I also just always forget that there's a subset of crates that can't use HashMap, and I'd prefer the answer to be "use it from some other location" vs "too bad go use something else". Along those lines I think it's reasonable to use IndexMap everywhere in public-facing structures, and then internally if std is enabled this crate uses std::collections. If std is disabled then maybe type HashMap = IndexMap or something similar? In the sense that IndexMap is at least the collection we have on-hand that's closes to a hash map and works in no_std.

Does that sound reasonable to you? I realize it's a bit vague with the map bits, so I can try to poke at those too if I'm not making any sense.

[wilfreddenton]

Thanks for the feedback @alexcrichton.

For Path, I agree with the changes you've made, and I'd actually say this could go 1 step further of doubling down on "the source of truth is String". That may actually already be the case (sorry only skimmed so far), but wanted to confirm that making Path things a thin wrapper around str things I think is totally reasonable.

Yes everything runs through the str methods and path/fs related logic has been pushed to the top edge.

The wasmparser crate deals with a very similar problem in this regard and would probably be a good place to draw inspiration from.

I looked through this and it uses the newtype pattern to abstract over hashbrown and BTree collections providing a single interface. It is very complete with explicit implementations of every method.

In my latest commit I do something similar. It's less robust but I think more appropriate for this crate. I am unifying std HashMap and the no_std IndexSet. I create a newtype wrapper HashMap over IndexMap (same for set) and rely on Deref to forward method calls through so as to not have to explicitly implement them (like in wasmparser::collections) and some iterator traits. The HashMap and IndexMap interfaces are pretty similar with a notable exception being that we need to conditionally import their respective Entry types.

Your type alias idea "type HashMap = IndexMap" almost works but the no_std IndexSet not having ::new() and only ::default() becomes an issue. I think people are very used to using new with HashMap and the only way to get that with the alias is to have an Ext trait with a new method. But then you have to remember to import that and also rust-analyzer constantly has dead code false positives with it.

Personally I like the simplicty of using BTree* types where possible to maintain no_std compatability and only reaching for something like indexmap when properties like insertion order are needed but I'm happy to go with this new strategy or something else if you want to make additional changes.

[alexcrichton]

Ah the btree/indexmap/hashmap situation in wasmparser is largely borne out of the desire to have a cargo feature to switch between hashing and BTreeMap. That affects maps in the public API which requires a bit of a convoluted situation in terms of wrappers and such. None of that I think is applicable for wit-parser which largely avoids Hash{Map,Set} in its public API (or at least that's the intention).

Another idea I've now had: reading over hashbrown nowadays there's a default-hasher feature which looks to be no-std compatible, so could that be used? Without the std feature there'd be type HashMap = hashbrown::HashMap and with std there'd be type HashMap = std::collections::HashMap. I think they're API-compatible so that should work out internally w.r.t. ::new vs ::default. That'd also remove the hack/idea of mine to feign a hash map with IndexMap, which I agree is a bit kludge.

[wilfreddenton]

Using hashbrown's HashMap simplifies the implementation. Since the workspace manifest already depends on hashbrown with the default-hasher feature enabled, adding it to this crate has minimal cost. However, there's one complication: 0adbcfd#diff-fc97c873ffa8b5cf64dd5efcc94b11744589a338eb589f3975bf0f290326b79cR2423. This solves a "cannot borrow as mutable because it is already borrowed as immutable" error between self.worlds and self.semver_track.

[alexcrichton]

Looking good to me, thanks! I've left some comments below, but as an additionally one I think it'd be reasonable to drop the update to the id-arena crate from this PR. That would mean the crate isn't actually no-std compatible nor could CI be updated, but that'll help decouple that dependency update from this PR while that's sorted out.

[wilfreddenton]

Thanks for comments.

There's now a resolve::fs sub-module that contains all the fs (std) stuff. And now in mod.rs there's just:

#[cfg(feature = "std")]
mod fs;
#[cfg(feature = "std")]
pub use fs::PackageSourceMap;

Because I made the indexmap dep in the root manifest default-features = false I had to enable std in the crates that need it.

My PR in id-arena was merged earlier today and a new release 2.3.0 published. This PR is using that now but we can do this in another PR if you want.

[alexcrichton]

Oh fine to update here, just didn't want to unduly block on that. Thanks again for this and thank you for your work on this, it's much appreciated!