Skip to content

feat: enable wasm32 compilation by making xx crate conditional#641

Open
andreaTP wants to merge 1 commit into
jdx:mainfrom
andreaTP:java-wasm-enablement
Open

feat: enable wasm32 compilation by making xx crate conditional#641
andreaTP wants to merge 1 commit into
jdx:mainfrom
andreaTP:java-wasm-enablement

Conversation

@andreaTP
Copy link
Copy Markdown

Supersedes #570 and #561 — this PR contains only the Rust-level changes needed for wasm32 support to avoid having to patch on top downstream.

The xx crate doesn't support wasm32. This makes xx a conditional dependency (cfg(not(target_arch = "wasm32"))) and replaces its usages (xx::regex!, xx::file, xx::process) with std equivalents. sh execution is disabled on wasm (returns an error).

CI now includes a minimal cargo build --target wasm32-wasip1 check to prevent regressions and this is all I need to bundle and ship a usage4j library usable and embeddable in Java.

Please @jdx have a look and let me know your thoughts 🙏

Make the xx crate dependency conditional on non-wasm32 targets and
replace xx::regex!, xx::file, and xx::process usages with std
equivalents (LazyLock<Regex>, std::fs, std::process) so the codebase
compiles for wasm32-wasip1.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 19, 2026

Greptile Summary

This PR makes the xx crate a conditional dependency (disabled on wasm32) and replaces its usages with std equivalents — xx::regex!LazyLock<Regex>, xx::file::write/read_to_stringstd::fs calls, and xx::process → a stub that returns an error on wasm32. A minimal cargo build --target wasm32-wasip1 CI step is added to guard against regressions.

  • All xx::file::write replacements correctly add an explicit create_dir_all call to preserve the parent-directory-creation behaviour that xx provided.
  • The zsh_shell_quote doc-comment was unintentionally removed in complete_word.rs alongside the wasm changes; the two cfg-variants of the local sh function also end up with different error types (xx::XXResult vs miette::Result), though both coerce correctly at call sites.
  • Both Cargo.toml files are missing a blank line before the new [target.'cfg(not(wasm32))'.dependencies] section header.

Confidence Score: 4/5

Safe to merge; all functional changes are correct and the wasm stub behaviour is well-scoped.

The core wasm32 gating logic is sound across both crates, and the std-equivalent replacements are semantically identical to the xx helpers they replace. The only items worth a second look are a lost doc-comment in complete_word.rs and the mismatched error types on the two cfg-variants of the local sh function — neither affects runtime behaviour on any current target.

cli/src/cli/complete_word.rs — removed doc-comments and mismatched sh return types worth tidying before the PR lands.

Important Files Changed

Filename Overview
.github/workflows/test.yml Adds a cargo build --target wasm32-wasip1 -p usage-cli step to CI to guard against regressions; straightforward and correct.
cli/Cargo.toml Moves xx into a cfg(not(wasm32)) target section; missing blank line before the new section header.
lib/Cargo.toml Same xx conditional move as in cli/Cargo.toml; same missing blank line before the new section.
cli/src/cli/complete_word.rs Replaces xx::regex! with LazyLock<Regex> and adds a wasm32 stub for sh; inadvertently removes doc-comments for zsh_shell_quote, and the two sh cfg-variants have mismatched return types.
lib/src/sh.rs Adds wasm32 stub returning UsageErr::IO(Unsupported) and gates all non-wasm32 imports/items correctly.
lib/src/error.rs Gates the XXError variant on cfg(not(wasm32)); clean and consistent with the conditional dep.
lib/src/spec/mod.rs Replaces xx::regex! and xx::file::read_to_string with LazyLock<Regex> and std::fs::read_to_string; semantically equivalent.
lib/src/docs/markdown/renderer.rs Replaces three xx::regex! macros with LazyLock<Regex> statics; patterns are identical, no behavioural change.
cli/src/cli/generate/fig.rs Replaces xx::file::write with create_dir_all + std::fs::write, preserving the parent-dir-creation behaviour.
cli/src/cli/generate/manpage.rs Same xx::file::write replacement as in fig.rs; correct.
cli/src/cli/generate/markdown.rs Same xx::file::write replacement; correct.
cli/src/lib.rs Gates extern crate xx on cfg(not(wasm32)); minimal and correct.

Reviews (1): Last reviewed commit: "feat: enable wasm32 compilation by makin..." | Re-trigger Greptile

Comment on lines 383 to 394
@@ -393,12 +390,14 @@ fn zsh_shell_quote(s: &str) -> String {
if !s.is_empty() && s.chars().all(safe) {
return s.to_string();
}
// Wrap in single quotes; close-open dance escapes any internal apostrophes.
let escaped = s.replace('\'', "'\\''");
format!("'{escaped}'")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Doc-comment inadvertently removed

The three-line /// doc-comment for zsh_shell_quote and the inline // Wrap in single quotes… comment inside the body were deleted in this diff, but they are unrelated to the wasm32 changes. The doc-comment explains why the function chooses consistent single-quote quoting over zsh's default mix of styles — useful context for anyone working on completion output later.

Comment thread cli/Cargo.toml
Comment on lines 42 to 44
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Missing blank line before the new [target.…] section makes the TOML harder to read and deviates from the project's existing style (compare with the [target.'cfg(unix)'.dependencies] section below, which has a blank line before it).

Suggested change
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread lib/Cargo.toml
Comment on lines 39 to 41
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Same missing blank-line issue as in cli/Cargo.toml — the new [target.…] section header is run directly into the last [dependencies] entry without a blank line separator.

Suggested change
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"

Comment on lines 397 to +419
@@ -413,3 +412,8 @@ fn sh(script: &str) -> XXResult<String> {
let stdout = String::from_utf8(output.stdout).expect("stdout is not utf-8");
Ok(stdout)
}

#[cfg(target_arch = "wasm32")]
fn sh(_script: &str) -> miette::Result<String> {
Err(miette::miette!("shell execution is not supported on wasm"))
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Inconsistent return types between target variants of sh

The non-wasm32 variant returns xx::XXResult<String> (Result<String, xx::XXError>) while the wasm32 variant returns miette::Result<String> (Result<String, miette::Report>). Both happen to coerce correctly through ? into the caller's miette::Result, but the two arms of the same logical function having different error types can be surprising. For consistency with lib/src/sh.rs — and to make future callers easier — consider giving the wasm32 arm the same outer return-type shape, either by defining a local alias or by aligning on miette::Result in both arms.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the codebase to support wasm32 targets by making the xx crate dependency optional and replacing its utilities—such as file I/O and regex macros—with standard library functions and the regex crate using LazyLock. It also introduces conditional compilation for shell execution and error types. Feedback focuses on unifying return types for target-specific functions, restoring accidentally removed documentation, adopting more idiomatic error handling with into_diagnostic(), and moving lazily initialized regexes to the module level for improved performance and readability.

Comment thread lib/src/sh.rs
use xx::{XXError, XXResult};

#[cfg(not(target_arch = "wasm32"))]
pub(crate) fn sh(script: &str) -> XXResult<String> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The return type of the non-wasm sh implementation (XXResult<String>) differs from the wasm implementation (Result<String, UsageErr>). For consistency and to avoid issues in target-agnostic callers, both should return Result<String, UsageErr>. Since UsageErr already implements From<XXError>, this change is straightforward.

Suggested change
pub(crate) fn sh(script: &str) -> XXResult<String> {
pub(crate) fn sh(script: &str) -> Result<String, crate::error::UsageErr> {

let stdout = sh(&run)?;
// trace!("stdout: {stdout}");
let re = regex!(r"[^\\]:");
static COLON_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"[^\\]:").unwrap());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Defining a static LazyLock inside a function is correct, but it's generally more idiomatic and efficient to move these to the module level. This avoids the overhead of the lazy initialization check being scoped within the function logic, especially if called frequently during completions.

Comment on lines 383 to 385
fn zsh_shell_quote(s: &str) -> String {
fn safe(c: char) -> bool {
matches!(c,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The documentation comments for zsh_shell_quote were accidentally removed. These provide valuable context on why this specific quoting strategy is used for zsh completions and should be restored.

/// Wrap a completion value in single quotes if any character would otherwise
/// be interpreted by the shell. The result is meant to be inserted by
/// compadd -Q verbatim, so the user sees consistent single-quote quoting
/// instead of zsh's default mix of backslash and single-quote styles.
fn zsh_shell_quote(s: &str) -> String {
References
  1. Prefer Rust-side shell quoting (e.g., zsh_shell_quote) over shell-side manual unescaping logic for completion scripts to ensure robustness and maintainability.

Comment on lines +360 to +363
std::fs::create_dir_all(parent).map_err(|e| miette::miette!("{e}"))?;
}
std::fs::write(path, format!("{}\n", md.trim()))
.map_err(|e| miette::miette!("{e}"))?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of manually mapping the error to a miette report using a string, you can use into_diagnostic() which is more idiomatic in this codebase and preserves the underlying error context.

            if let Some(parent) = path.parent() {
                std::fs::create_dir_all(parent).into_diagnostic()?;
            }
            std::fs::write(path, format!("{}\n", md.trim()))
                .into_diagnostic()?;

Comment on lines +93 to +95
static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(`[^`]*`)|(<)").unwrap());
&RE
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Defining a static regex inside a closure that is executed for every line of a markdown file is inefficient. While LazyLock ensures it's only initialized once, the check still occurs on every iteration. Move this static to the module level.

Comment thread lib/src/spec/mod.rs
if full.starts_with("#!") {
let usage_regex = xx::regex!(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])");
if full.lines().any(|l| usage_regex.is_match(l)) {
static USAGE_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])").unwrap());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Consider moving this static regex to the module level to improve readability and ensure initialization happens outside of the core logic of split_script.

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.

1 participant