Skip to content

refactor: use static strings where possible - #30898

Closed
robjtede wants to merge 1 commit into
oven-sh:mainfrom
robjtede:less-unsafe-static-strings
Closed

robjtede wants to merge 1 commit into
oven-sh:mainfrom
robjtede:less-unsafe-static-strings

Conversation

@robjtede

@robjtede robjtede commented May 16, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Remove needless BStr usage for return types that could be static strings.

How did you verify your code works?

Trivial conversion.

@robjtede
robjtede marked this pull request as ready for review May 16, 2026 18:54

@claude claude Bot left a comment

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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@robjtede
robjtede marked this pull request as draft May 16, 2026 18:55
@coderabbitai

coderabbitai Bot commented May 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR refactors static label-returning methods across the codebase from returning &'static [u8] byte slices to returning &'static str string slices. Methods are converted to const functions, match arms updated to use string literals, and all call sites adjusted with .as_bytes() conversions where needed.

Changes

Byte-to-String Slice Return Type Refactoring

Layer / File(s) Summary
Install process and package field labels
src/install/PackageInstall.rs, src/install/lockfile/Package.rs, src/install/PackageInstaller.rs, src/install/patch_install.rs
Step::name and PackageField::name are converted to return &'static str via const functions. Error logging in PackageInstaller and patch_install updated to use values directly.
JavaScriptCore event and type labels
src/jsc/EventType.rs, src/jsc/JSType.rs, src/jsc/ConsoleObject.rs
EventType::label() and JSType::typed_array_name() return &'static str as const functions. Console output formatting updated to use returned values directly and convert typed-array labels via .as_bytes().
CLI project generation and init command labels
src/runtime/cli/create/SourceFileProjectGenerator.rs, src/runtime/cli/init_command.rs
Tag::label() and Template::name() return &'static str as const functions. Project logger and package.json/README.md generation updated to use returned values directly and call .as_bytes() where byte conversion is needed.
Spawn error handling and subprocess stdio labels
src/runtime/api/bun/spawn/stdio.rs, src/runtime/shell/subproc.rs
ToSpawnOptsError::to_str() returns &'static str as const function. Shell subprocess error paths for stdin, stdout, stderr updated to convert via .as_bytes() before constructing error payloads.
FFI ABI type labels
src/runtime/ffi/abi_type.rs
AbiRow fields c_type and c_param_type changed to &'static str. ABI_TABLE construction uses string literals. typename_label() and param_typename_label() return &'static str as const functions with .as_bytes() conversions at write sites.
Server platform tag labels
src/runtime/server/server_body.rs
Internal helpers os_tag_name and arch_tag_name return &'static str for platform identifiers. AST property generation converts via .as_bytes().
Test runner event type and pretty formatting labels
src/runtime/test_runner/pretty_format.rs
EventType::label() returns &'static str as const function. Event and typed-array pretty-printing updated to use values directly and convert typed-array names via .as_bytes().
Web core encoding and text decoder labels
src/runtime/webcore/EncodingLabel.rs, src/runtime/webcore/TextDecoder.rs
EncodingLabel::get_label() returns &'static str as const function. TextDecoder updated to convert via .as_bytes() when creating ZigString and TextCodec instances.
S3 ACL and storage class labels
src/s3_signing/acl.rs, src/s3_signing/storage_class.rs, src/runtime/webcore/S3Client.rs, src/s3_signing/credentials.rs
ACL::to_string() and StorageClass::to_string() return &'static str as const functions. S3Client ACL formatting updated to use values directly. Credentials signing updated to call .as_bytes() on converted ACL and storage class values.

Suggested reviewers

  • dylan-conway
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description addresses both template sections: it explains what the PR does (removing needless BStr usage) and how verification was performed (trivial conversion).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title 'refactor: use static strings where possible' accurately describes the main objective: converting byte-string returns to static string returns throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@robjtede
robjtede force-pushed the less-unsafe-static-strings branch from c1e3732 to 51888f7 Compare May 16, 2026 18:59
@robjtede
robjtede force-pushed the less-unsafe-static-strings branch from 51888f7 to efde4ec Compare May 16, 2026 19:01
@robjtede
robjtede marked this pull request as ready for review May 16, 2026 19:03

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/runtime/ffi/abi_type.rs`:
- Around line 278-281: The param_typename method is emitting the wrong label by
calling typename_label(); update param_typename to call param_typename_label()
instead so it writes the correct C parameter type for variants like Uint32T and
Buffer—locate the param_typename(&self, writer: &mut impl std::io::Write) ->
Result<(), bun_core::Error> implementation and replace the call to
self.typename_label() with self.param_typename_label() (keeping the
writer.write_all(...)? and Ok(()) behavior).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b8933a85-6501-42f4-a6c2-e9ca4e788d20

📥 Commits

Reviewing files that changed from the base of the PR and between e750984 and efde4ec.

📒 Files selected for processing (20)
  • src/install/PackageInstall.rs
  • src/install/PackageInstaller.rs
  • src/install/lockfile/Package.rs
  • src/install/patch_install.rs
  • src/jsc/ConsoleObject.rs
  • src/jsc/EventType.rs
  • src/jsc/JSType.rs
  • src/runtime/api/bun/spawn/stdio.rs
  • src/runtime/cli/create/SourceFileProjectGenerator.rs
  • src/runtime/cli/init_command.rs
  • src/runtime/ffi/abi_type.rs
  • src/runtime/server/server_body.rs
  • src/runtime/shell/subproc.rs
  • src/runtime/test_runner/pretty_format.rs
  • src/runtime/webcore/EncodingLabel.rs
  • src/runtime/webcore/S3Client.rs
  • src/runtime/webcore/TextDecoder.rs
  • src/s3_signing/acl.rs
  • src/s3_signing/credentials.rs
  • src/s3_signing/storage_class.rs

Comment on lines 278 to 281
pub fn param_typename(self, writer: &mut impl std::io::Write) -> Result<(), bun_core::Error> {
writer.write_all(self.typename_label())?;
writer.write_all(self.typename_label().as_bytes())?;
Ok(())
}

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.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Bug: param_typename calls wrong method — will emit incorrect type for Uint32T and Buffer.

This calls typename_label() instead of param_typename_label(). For variants where c_type differs from c_param_type (e.g., Uint32T: "uint32_t" vs "int32_t", Buffer: "void*" vs "buffer"), this produces incorrect C output.

🐛 Proposed fix
     pub fn param_typename(self, writer: &mut impl std::io::Write) -> Result<(), bun_core::Error> {
-        writer.write_all(self.typename_label().as_bytes())?;
+        writer.write_all(self.param_typename_label().as_bytes())?;
         Ok(())
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runtime/ffi/abi_type.rs` around lines 278 - 281, The param_typename
method is emitting the wrong label by calling typename_label(); update
param_typename to call param_typename_label() instead so it writes the correct C
parameter type for variants like Uint32T and Buffer—locate the
param_typename(&self, writer: &mut impl std::io::Write) -> Result<(),
bun_core::Error> implementation and replace the call to self.typename_label()
with self.param_typename_label() (keeping the writer.write_all(...)? and Ok(())
behavior).

@robjtede robjtede changed the title Less unsafe static strings refactor: use static strings where possible May 16, 2026
@robjtede robjtede closed this Jun 25, 2026
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