implement publicHoistPattern and hoistPattern
#23567
Conversation
|
Updated 6:22 PM PT - Oct 13th, 2025
❌ @dylan-conway, your commit 6c735e1 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 23567That installs a local version of the PR into your bun-23567 --bun |
WalkthroughAdds PNPM-style hoist pattern support: new PnpmMatcher module, config parsing (bunfig/.npmrc), two new install options (public_hoist_pattern, hoist_pattern), hoisting decision/tracking in isolated installs (entry.hoisted + public_hoisted/hidden_hoisted), string-escaping helpers, RegularExpression relocation, and tests/harness updates. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Comment |
1 similar comment
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/string/escapeRegExp.zig (1)
61-66: Same inconsistent behavior as inescapeRegExp.This
elsebranch has the same debug/release inconsistency as the one inescapeRegExp(lines 25-30).Apply the same fix:
- else => |c| { - if (comptime Environment.isDebug) { - unreachable; - } - try writer.writeByte(c); - }, + else => unreachable, // all special_characters are handled above
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
src/api/schema.zig(2 hunks)src/bun.js/bindings/RegularExpression.zig(1 hunks)src/bun.js/jsc.zig(1 hunks)src/bun.js/test/jest.zig(1 hunks)src/bun.zig(0 hunks)src/bunfig.zig(1 hunks)src/cli.zig(1 hunks)src/cli/Arguments.zig(1 hunks)src/collections/array_list.zig(1 hunks)src/ini.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(2 hunks)src/install/PnpmMatcher.zig(1 hunks)src/install/install.zig(1 hunks)src/install/isolated_install.zig(2 hunks)src/string/escapeRegExp.zig(1 hunks)src/string/immutable.zig(2 hunks)test/cli/install/public-hoist-pattern.test.ts(1 hunks)test/harness.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/bun.zig
🧰 Additional context used
📓 Path-based instructions (10)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/jsc.zigsrc/collections/array_list.zigsrc/string/escapeRegExp.zigsrc/cli/Arguments.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/bun.js/test/jest.zigsrc/install/PnpmMatcher.zigsrc/install/install.zigsrc/api/schema.zigsrc/cli.zigsrc/bunfig.zigsrc/install/isolated_install.zigsrc/string/immutable.zigsrc/bun.js/bindings/RegularExpression.zigsrc/ini.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/jsc.zigsrc/bun.js/test/jest.zigsrc/bun.js/bindings/RegularExpression.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/bun.js/jsc.zigsrc/collections/array_list.zigsrc/string/escapeRegExp.zigsrc/cli/Arguments.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/bun.js/test/jest.zigsrc/install/PnpmMatcher.zigsrc/install/install.zigsrc/api/schema.zigsrc/cli.zigsrc/bunfig.zigsrc/install/isolated_install.zigsrc/string/immutable.zigsrc/bun.js/bindings/RegularExpression.zigsrc/ini.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/harness.tstest/cli/install/public-hoist-pattern.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/harness.tstest/cli/install/public-hoist-pattern.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/harness.tstest/cli/install/public-hoist-pattern.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/install/public-hoist-pattern.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/cli/install/public-hoist-pattern.test.ts
test/cli/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place CLI command tests under test/cli/
Files:
test/cli/install/public-hoist-pattern.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/cli/install/public-hoist-pattern.test.ts
🧠 Learnings (10)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/jsc.zigsrc/bun.js/test/jest.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/jsc.zigsrc/bun.js/test/jest.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/jsc.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Implement JavaScript bindings in a Zig file named with a js_ prefix (e.g., js_smtp.zig, js_your_feature.zig)
Applied to files:
src/bun.js/jsc.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/test/jest.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/test/jest.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/test/jest.zig
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/js/bun/** : Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Applied to files:
src/api/schema.zig
📚 Learning: 2025-10-12T02:22:34.349Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.349Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/cli/install/public-hoist-pattern.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/cli/install/public-hoist-pattern.test.ts
🧬 Code graph analysis (1)
test/cli/install/public-hoist-pattern.test.ts (1)
test/harness.ts (5)
VerdaccioRegistry(1631-1767)runBunInstall(1215-1271)bunEnv(49-64)readdirSorted(1776-1780)bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (10)
src/collections/array_list.zig (1)
122-124: LGTM!The
writermethod implementation is correct and follows the established delegation pattern in this file, properly converting the managed allocator to a standard allocator for the unmanaged writer.src/string/immutable.zig (2)
2309-2310: LGTM!The new public exports for regex escaping functions follow the established pattern in this module and provide clear, well-named API surface.
2353-2353: LGTM!The import statement correctly follows the coding guideline to place
@importdirectives at the bottom of Zig files.src/string/escapeRegExp.zig (2)
1-1: LGTM!The
special_charactersset correctly includes standard regex metacharacters that need escaping.
74-76: LGTM!The import statements are correctly placed at the bottom of the file, following the coding guidelines.
src/install/isolated_install.zig (3)
415-416: LGTM!Memory management is correct. The
publicly_hoistedmap is properly initialized withmanager.allocatorand cleanup is ensured viadefer.
539-543: Ensurenew_entry_dep_idis valid before indexing
The code doesdependencies[new_entry_dep_id]whenentry.entry_parent_id == .root; verify thatnew_entry_dep_idcan never beinvalid_dependency_idhere or add a guard to skip/error if it is.
545-559: Verify root entry and duplicate insertion handlingNo evidence confirms that
entry_dependencies[0]always refers to the root entry or that inserting only by name prevents duplicate(entry_id, dep_id)pairs. Manually verify that:
- The root entry is initialized exactly once at index 0.
- The insert logic guards against duplicate
(entry_id, dep_id)entries, not just duplicate names.src/install/PnpmMatcher.zig (2)
107-130: LGTM!The
createMatcherfunction correctly handles:
- Trimming whitespace
- Exclusion prefix (
!)- Wildcard optimization (
*)- Regex escaping and anchoring (
^...$)The use of
.cloneUTF8ensures the regex owns its pattern string.
170-186: ```shell
#!/bin/bashInspect PnpmMatcher parsing logic to verify matcher order
rg -n "pub fn.*PnpmMatcher" -C3 src/install/PnpmMatcher.zig
rg -n "parse" -C3 src/install/PnpmMatcher.zig
rg -n ".append" -C3 src/install/PnpmMatcher.zig</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if (out.get("public-hoist-pattern")) |public_hoist_pattern_expr| { | ||
| install.public_hoist_patterns = bun.install.PnpmMatcher.fromExpr( | ||
| allocator, | ||
| public_hoist_pattern_expr, | ||
| log, | ||
| source, | ||
| ) catch |err| switch (err) { | ||
| error.OutOfMemory => |oom| return oom, | ||
| error.InvalidRegExp, error.UnexpectedExpr => patterns: { | ||
| log.reset(); | ||
| break :patterns null; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Do not clear the entire npmrc log on matcher errors
log.reset() nukes every earlier warning/error collected while parsing the same .npmrc, so any unrelated diagnostics disappear as soon as we hit an invalid hoist pattern. That makes troubleshooting other config mistakes impossible. Instead, let the log keep the matcher’s error (or just return null) without resetting it entirely.
Apply this diff to retain prior log entries:
- ) catch |err| switch (err) {
- error.OutOfMemory => |oom| return oom,
- error.InvalidRegExp, error.UnexpectedExpr => patterns: {
- log.reset();
- break :patterns null;
- },
- };
+ ) catch |err| switch (err) {
+ error.OutOfMemory => |oom| return oom,
+ error.InvalidRegExp, error.UnexpectedExpr => null,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (out.get("public-hoist-pattern")) |public_hoist_pattern_expr| { | |
| install.public_hoist_patterns = bun.install.PnpmMatcher.fromExpr( | |
| allocator, | |
| public_hoist_pattern_expr, | |
| log, | |
| source, | |
| ) catch |err| switch (err) { | |
| error.OutOfMemory => |oom| return oom, | |
| error.InvalidRegExp, error.UnexpectedExpr => patterns: { | |
| log.reset(); | |
| break :patterns null; | |
| }, | |
| }; | |
| } | |
| if (out.get("public-hoist-pattern")) |public_hoist_pattern_expr| { | |
| install.public_hoist_patterns = bun.install.PnpmMatcher.fromExpr( | |
| allocator, | |
| public_hoist_pattern_expr, | |
| log, | |
| source, | |
| ) catch |err| switch (err) { | |
| error.OutOfMemory => |oom| return oom, | |
| error.InvalidRegExp, error.UnexpectedExpr => null, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/ini.zig around lines 1072 to 1085, the current error handling for
PnpmMatcher.fromExpr calls log.reset() which clears all prior npmrc diagnostics;
remove the log.reset() call and instead simply return null (or let the matcher
error remain) when InvalidRegExp or UnexpectedExpr occur so earlier
warnings/errors are preserved; update the catch arm to omit log.reset() and
break/return null as before.
| /// https://github.com/pnpm/pnpm/blob/3abd3946237aa6ba7831552310ec371ddd3616c2/config/matcher/src/index.ts | ||
| const PnpmMatcher = @This(); | ||
|
|
||
| matchers: []const Matcher, | ||
| behavior: Behavior, | ||
|
|
||
| const Matcher = struct { | ||
| pattern: union(enum) { | ||
| match_all, | ||
| regex: *jsc.RegularExpression, | ||
| }, | ||
| is_exclude: bool, | ||
| }; | ||
|
|
||
| const Behavior = enum { | ||
| all_matchers_include, | ||
| all_matchers_exclude, | ||
| has_exclude_and_include_matchers, | ||
| }; | ||
|
|
||
| const FromExprError = OOM || error{ | ||
| InvalidRegExp, | ||
| UnexpectedExpr, | ||
| }; | ||
|
|
||
| pub fn fromExpr(allocator: std.mem.Allocator, expr: ast.Expr, log: *logger.Log, source: *const logger.Source) FromExprError!PnpmMatcher { | ||
| var buf: collections.ArrayListDefault(u8) = .init(); | ||
| defer buf.deinit(); | ||
|
|
||
| bun.jsc.initialize(false); | ||
|
|
||
| var matchers: collections.ArrayListDefault(Matcher) = .init(); | ||
|
|
||
| var has_include = false; | ||
| var has_exclude = false; | ||
|
|
||
| switch (expr.data) { | ||
| .e_string => { | ||
| const pattern = expr.data.e_string.slice(allocator); | ||
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | ||
| error.OutOfMemory => return err, | ||
| error.InvalidRegExp => { | ||
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | ||
| .loc = expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return err; | ||
| }, | ||
| }; | ||
| has_include = has_include or !matcher.is_exclude; | ||
| has_exclude = has_exclude or matcher.is_exclude; | ||
| try matchers.append(matcher); | ||
| }, | ||
| .e_array => |patterns| { | ||
| for (patterns.slice()) |pattern_expr| { | ||
| if (try pattern_expr.asStringCloned(allocator)) |pattern| { | ||
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | ||
| error.OutOfMemory => return err, | ||
| error.InvalidRegExp => { | ||
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | ||
| .loc = pattern_expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return err; | ||
| }, | ||
| }; | ||
| has_include = has_include or !matcher.is_exclude; | ||
| has_exclude = has_exclude or matcher.is_exclude; | ||
| try matchers.append(matcher); | ||
| } else { | ||
| try log.addErrorOpts("Expected a string", .{ | ||
| .loc = pattern_expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return error.UnexpectedExpr; | ||
| } | ||
| } | ||
| }, | ||
| else => { | ||
| try log.addErrorOpts("Expected a string or an array of strings", .{ | ||
| .loc = expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return error.UnexpectedExpr; | ||
| }, | ||
| } | ||
|
|
||
| const behavior: Behavior = if (!has_include) | ||
| .all_matchers_exclude | ||
| else if (!has_exclude) | ||
| .all_matchers_include | ||
| else | ||
| .has_exclude_and_include_matchers; | ||
|
|
||
| return .{ | ||
| .matchers = try matchers.toOwnedSlice(), | ||
| .behavior = behavior, | ||
| }; | ||
| } | ||
|
|
||
| const CreateMatcherError = OOM || error{InvalidRegExp}; | ||
|
|
||
| fn createMatcher(raw: []const u8, buf: *collections.ArrayListDefault(u8)) CreateMatcherError!Matcher { | ||
| buf.clearRetainingCapacity(); | ||
| var writer = buf.writer(); | ||
|
|
||
| var trimmed = strings.trim(raw, &strings.whitespace_chars); | ||
|
|
||
| var is_exclude = false; | ||
| if (strings.startsWithChar(trimmed, '!')) { | ||
| is_exclude = true; | ||
| trimmed = trimmed[1..]; | ||
| } | ||
|
|
||
| if (strings.eqlComptime(trimmed, "*")) { | ||
| return .{ .pattern = .match_all, .is_exclude = is_exclude }; | ||
| } | ||
|
|
||
| try writer.writeByte('^'); | ||
| try strings.escapeRegExpForPackageNameMatching(trimmed, writer); | ||
| try writer.writeByte('$'); | ||
|
|
||
| const regex = try jsc.RegularExpression.init(.cloneUTF8(buf.items()), .none); | ||
|
|
||
| return .{ .pattern = .{ .regex = regex }, .is_exclude = is_exclude }; | ||
| } | ||
|
|
||
| pub fn isMatch(this: *const PnpmMatcher, name: []const u8) bool { | ||
| if (this.matchers.len == 0) { | ||
| return false; | ||
| } | ||
|
|
||
| const name_str: String = .fromBytes(name); | ||
|
|
||
| switch (this.behavior) { | ||
| .all_matchers_include => { | ||
| for (this.matchers) |matcher| { | ||
| switch (matcher.pattern) { | ||
| .match_all => { | ||
| return true; | ||
| }, | ||
| .regex => |regex| { | ||
| if (regex.matches(name_str)) { | ||
| return true; | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| return false; | ||
| }, | ||
| .all_matchers_exclude => { | ||
| for (this.matchers) |matcher| { | ||
| switch (matcher.pattern) { | ||
| .match_all => { | ||
| return false; | ||
| }, | ||
| .regex => |regex| { | ||
| if (regex.matches(name_str)) { | ||
| return false; | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| return true; | ||
| }, | ||
| .has_exclude_and_include_matchers => { | ||
| var matches = false; | ||
| for (this.matchers) |matcher| { | ||
| switch (matcher.pattern) { | ||
| .match_all => { | ||
| matches = !matcher.is_exclude; | ||
| }, | ||
| .regex => |regex| { | ||
| if (regex.matches(name_str)) { | ||
| matches = !matcher.is_exclude; | ||
| } | ||
| }, | ||
| } | ||
| } | ||
| return matches; | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| const std = @import("std"); | ||
|
|
||
| const bun = @import("bun"); | ||
| const OOM = bun.OOM; | ||
| const String = bun.String; | ||
| const ast = bun.ast; | ||
| const collections = bun.collections; | ||
| const jsc = bun.jsc; | ||
| const logger = bun.logger; | ||
| const strings = bun.strings; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding inline documentation.
This module implements PNPM's public-hoist-pattern matching logic. While the reference link (line 1) is helpful, consider adding brief doc comments on the public functions (fromExpr, isMatch) to explain:
- Expected input format
- Matching semantics for each behavior mode
- Memory ownership (caller must deinit)
Example:
+/// Parse a bunfig/npmrc expression into a PnpmMatcher.
+/// Returns a matcher that must be deinitialized by the caller.
+/// Supports string or array of strings, with optional "!" prefix for exclusion.
pub fn fromExpr(allocator: std.mem.Allocator, expr: ast.Expr, log: *logger.Log, source: *const logger.Source) FromExprError!PnpmMatcher {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// https://github.com/pnpm/pnpm/blob/3abd3946237aa6ba7831552310ec371ddd3616c2/config/matcher/src/index.ts | |
| const PnpmMatcher = @This(); | |
| matchers: []const Matcher, | |
| behavior: Behavior, | |
| const Matcher = struct { | |
| pattern: union(enum) { | |
| match_all, | |
| regex: *jsc.RegularExpression, | |
| }, | |
| is_exclude: bool, | |
| }; | |
| const Behavior = enum { | |
| all_matchers_include, | |
| all_matchers_exclude, | |
| has_exclude_and_include_matchers, | |
| }; | |
| const FromExprError = OOM || error{ | |
| InvalidRegExp, | |
| UnexpectedExpr, | |
| }; | |
| pub fn fromExpr(allocator: std.mem.Allocator, expr: ast.Expr, log: *logger.Log, source: *const logger.Source) FromExprError!PnpmMatcher { | |
| var buf: collections.ArrayListDefault(u8) = .init(); | |
| defer buf.deinit(); | |
| bun.jsc.initialize(false); | |
| var matchers: collections.ArrayListDefault(Matcher) = .init(); | |
| var has_include = false; | |
| var has_exclude = false; | |
| switch (expr.data) { | |
| .e_string => { | |
| const pattern = expr.data.e_string.slice(allocator); | |
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | |
| error.OutOfMemory => return err, | |
| error.InvalidRegExp => { | |
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | |
| .loc = expr.loc, | |
| .redact_sensitive_information = true, | |
| .source = source, | |
| }); | |
| return err; | |
| }, | |
| }; | |
| has_include = has_include or !matcher.is_exclude; | |
| has_exclude = has_exclude or matcher.is_exclude; | |
| try matchers.append(matcher); | |
| }, | |
| .e_array => |patterns| { | |
| for (patterns.slice()) |pattern_expr| { | |
| if (try pattern_expr.asStringCloned(allocator)) |pattern| { | |
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | |
| error.OutOfMemory => return err, | |
| error.InvalidRegExp => { | |
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | |
| .loc = pattern_expr.loc, | |
| .redact_sensitive_information = true, | |
| .source = source, | |
| }); | |
| return err; | |
| }, | |
| }; | |
| has_include = has_include or !matcher.is_exclude; | |
| has_exclude = has_exclude or matcher.is_exclude; | |
| try matchers.append(matcher); | |
| } else { | |
| try log.addErrorOpts("Expected a string", .{ | |
| .loc = pattern_expr.loc, | |
| .redact_sensitive_information = true, | |
| .source = source, | |
| }); | |
| return error.UnexpectedExpr; | |
| } | |
| } | |
| }, | |
| else => { | |
| try log.addErrorOpts("Expected a string or an array of strings", .{ | |
| .loc = expr.loc, | |
| .redact_sensitive_information = true, | |
| .source = source, | |
| }); | |
| return error.UnexpectedExpr; | |
| }, | |
| } | |
| const behavior: Behavior = if (!has_include) | |
| .all_matchers_exclude | |
| else if (!has_exclude) | |
| .all_matchers_include | |
| else | |
| .has_exclude_and_include_matchers; | |
| return .{ | |
| .matchers = try matchers.toOwnedSlice(), | |
| .behavior = behavior, | |
| }; | |
| } | |
| const CreateMatcherError = OOM || error{InvalidRegExp}; | |
| fn createMatcher(raw: []const u8, buf: *collections.ArrayListDefault(u8)) CreateMatcherError!Matcher { | |
| buf.clearRetainingCapacity(); | |
| var writer = buf.writer(); | |
| var trimmed = strings.trim(raw, &strings.whitespace_chars); | |
| var is_exclude = false; | |
| if (strings.startsWithChar(trimmed, '!')) { | |
| is_exclude = true; | |
| trimmed = trimmed[1..]; | |
| } | |
| if (strings.eqlComptime(trimmed, "*")) { | |
| return .{ .pattern = .match_all, .is_exclude = is_exclude }; | |
| } | |
| try writer.writeByte('^'); | |
| try strings.escapeRegExpForPackageNameMatching(trimmed, writer); | |
| try writer.writeByte('$'); | |
| const regex = try jsc.RegularExpression.init(.cloneUTF8(buf.items()), .none); | |
| return .{ .pattern = .{ .regex = regex }, .is_exclude = is_exclude }; | |
| } | |
| pub fn isMatch(this: *const PnpmMatcher, name: []const u8) bool { | |
| if (this.matchers.len == 0) { | |
| return false; | |
| } | |
| const name_str: String = .fromBytes(name); | |
| switch (this.behavior) { | |
| .all_matchers_include => { | |
| for (this.matchers) |matcher| { | |
| switch (matcher.pattern) { | |
| .match_all => { | |
| return true; | |
| }, | |
| .regex => |regex| { | |
| if (regex.matches(name_str)) { | |
| return true; | |
| } | |
| }, | |
| } | |
| } | |
| return false; | |
| }, | |
| .all_matchers_exclude => { | |
| for (this.matchers) |matcher| { | |
| switch (matcher.pattern) { | |
| .match_all => { | |
| return false; | |
| }, | |
| .regex => |regex| { | |
| if (regex.matches(name_str)) { | |
| return false; | |
| } | |
| }, | |
| } | |
| } | |
| return true; | |
| }, | |
| .has_exclude_and_include_matchers => { | |
| var matches = false; | |
| for (this.matchers) |matcher| { | |
| switch (matcher.pattern) { | |
| .match_all => { | |
| matches = !matcher.is_exclude; | |
| }, | |
| .regex => |regex| { | |
| if (regex.matches(name_str)) { | |
| matches = !matcher.is_exclude; | |
| } | |
| }, | |
| } | |
| } | |
| return matches; | |
| }, | |
| } | |
| } | |
| const std = @import("std"); | |
| const bun = @import("bun"); | |
| const OOM = bun.OOM; | |
| const String = bun.String; | |
| const ast = bun.ast; | |
| const collections = bun.collections; | |
| const jsc = bun.jsc; | |
| const logger = bun.logger; | |
| const strings = bun.strings; | |
| /// Parse a bunfig/npmrc expression into a PnpmMatcher. | |
| /// Returns a matcher that must be deinitialized by the caller. | |
| /// Supports string or array of strings, with optional "!" prefix for exclusion. | |
| pub fn fromExpr(allocator: std.mem.Allocator, expr: ast.Expr, log: *logger.Log, source: *const logger.Source) FromExprError!PnpmMatcher { | |
| var buf: collections.ArrayListDefault(u8) = .init(); | |
| defer buf.deinit(); | |
| bun.jsc.initialize(false); | |
| var matchers: collections.ArrayListDefault(Matcher) = .init(); | |
| // … rest of function unchanged … | |
| } |
🤖 Prompt for AI Agents
In src/install/PnpmMatcher.zig lines 1-198, add concise Zig doc comments (///)
at the top of the file and immediately above the public functions fromExpr and
isMatch: describe expected input formats (string or array of strings for
fromExpr), the matching semantics for each Behavior variant
(all_matchers_include, all_matchers_exclude, has_exclude_and_include_matchers),
that patterns support '!' exclude prefix and '*' match-all, and clearly state
memory ownership (fromExpr allocates matcher slice and the caller must
deinit/free it or the backing allocator must outlive usage). Keep comments
short, reference the PNPM URL already present, and ensure docs mention error
cases returned by fromExpr (InvalidRegExp, UnexpectedExpr, OOM).
| pub fn fromExpr(allocator: std.mem.Allocator, expr: ast.Expr, log: *logger.Log, source: *const logger.Source) FromExprError!PnpmMatcher { | ||
| var buf: collections.ArrayListDefault(u8) = .init(); | ||
| defer buf.deinit(); | ||
|
|
||
| bun.jsc.initialize(false); | ||
|
|
||
| var matchers: collections.ArrayListDefault(Matcher) = .init(); | ||
|
|
||
| var has_include = false; | ||
| var has_exclude = false; | ||
|
|
||
| switch (expr.data) { | ||
| .e_string => { | ||
| const pattern = expr.data.e_string.slice(allocator); | ||
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | ||
| error.OutOfMemory => return err, | ||
| error.InvalidRegExp => { | ||
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | ||
| .loc = expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return err; | ||
| }, | ||
| }; | ||
| has_include = has_include or !matcher.is_exclude; | ||
| has_exclude = has_exclude or matcher.is_exclude; | ||
| try matchers.append(matcher); | ||
| }, | ||
| .e_array => |patterns| { | ||
| for (patterns.slice()) |pattern_expr| { | ||
| if (try pattern_expr.asStringCloned(allocator)) |pattern| { | ||
| const matcher = createMatcher(pattern, &buf) catch |err| switch (err) { | ||
| error.OutOfMemory => return err, | ||
| error.InvalidRegExp => { | ||
| try log.addErrorFmtOpts(allocator, "Invalid regex: {s}", .{pattern}, .{ | ||
| .loc = pattern_expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return err; | ||
| }, | ||
| }; | ||
| has_include = has_include or !matcher.is_exclude; | ||
| has_exclude = has_exclude or matcher.is_exclude; | ||
| try matchers.append(matcher); | ||
| } else { | ||
| try log.addErrorOpts("Expected a string", .{ | ||
| .loc = pattern_expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return error.UnexpectedExpr; | ||
| } | ||
| } | ||
| }, | ||
| else => { | ||
| try log.addErrorOpts("Expected a string or an array of strings", .{ | ||
| .loc = expr.loc, | ||
| .redact_sensitive_information = true, | ||
| .source = source, | ||
| }); | ||
| return error.UnexpectedExpr; | ||
| }, | ||
| } | ||
|
|
||
| const behavior: Behavior = if (!has_include) | ||
| .all_matchers_exclude | ||
| else if (!has_exclude) | ||
| .all_matchers_include | ||
| else | ||
| .has_exclude_and_include_matchers; | ||
|
|
||
| return .{ | ||
| .matchers = try matchers.toOwnedSlice(), | ||
| .behavior = behavior, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Potential memory leak: regex objects are not cleaned up.
The fromExpr function creates Matcher structs containing jsc.RegularExpression pointers via createMatcher, but there's no corresponding deinit or cleanup function for PnpmMatcher. If regex objects require explicit cleanup, this will leak memory.
Consider adding a deinit method:
+pub fn deinit(this: *PnpmMatcher, allocator: std.mem.Allocator) void {
+ for (this.matchers) |matcher| {
+ switch (matcher.pattern) {
+ .regex => |regex| regex.deinit(),
+ .match_all => {},
+ }
+ }
+ allocator.free(this.matchers);
+}And ensure callers invoke it when done with the matcher.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/install/PnpmMatcher.zig around lines 26-103, the function builds Matcher
objects that hold jsc.RegularExpression pointers but there is no cleanup,
risking leaked regex objects; add a PnpmMatcher.deinit that iterates its
matchers, calls the appropriate regex deinit/free on each Matcher (and then
frees the matchers slice/array), and wire deinit usage so callers free returned
PnpmMatcher when done; additionally, ensure fromExpr does not leak on early
returns by cleaning up any partially appended matchers (for example use a defer
cleanup that runs on error paths or release matchers before returning an error).
| pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | ||
| var remain = input; | ||
|
|
||
| while (strings.indexOfAny(remain, special_characters)) |i| { | ||
| try writer.writeAll(remain[0..i]); | ||
| switch (remain[i]) { | ||
| '|', | ||
| '\\', | ||
| '{', | ||
| '}', | ||
| '(', | ||
| ')', | ||
| '[', | ||
| ']', | ||
| '^', | ||
| '$', | ||
| '+', | ||
| '*', | ||
| '?', | ||
| '.', | ||
| => |c| try writer.writeAll(&.{ '\\', c }), | ||
| '-' => try writer.writeAll("\\x2d"), | ||
| else => |c| { | ||
| if (comptime Environment.isDebug) { | ||
| unreachable; | ||
| } | ||
| try writer.writeByte(c); | ||
| }, | ||
| } | ||
| remain = remain[i + 1 ..]; | ||
| } | ||
|
|
||
| try writer.writeAll(remain); | ||
| } | ||
|
|
||
| /// '*' becomes '.*' instead of '\\*' | ||
| pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | ||
| var remain = input; | ||
|
|
||
| while (strings.indexOfAny(remain, special_characters)) |i| { | ||
| try writer.writeAll(remain[0..i]); | ||
| switch (remain[i]) { | ||
| '|', | ||
| '\\', | ||
| '{', | ||
| '}', | ||
| '(', | ||
| ')', | ||
| '[', | ||
| ']', | ||
| '^', | ||
| '$', | ||
| '+', | ||
| '?', | ||
| '.', | ||
| => |c| try writer.writeAll(&.{ '\\', c }), | ||
| '*' => try writer.writeAll(".*"), | ||
| '-' => try writer.writeAll("\\x2d"), | ||
| else => |c| { | ||
| if (comptime Environment.isDebug) { | ||
| unreachable; | ||
| } | ||
| try writer.writeByte(c); | ||
| }, | ||
| } | ||
| remain = remain[i + 1 ..]; | ||
| } | ||
|
|
||
| try writer.writeAll(remain); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing code duplication.
The two functions are nearly identical except for how they handle '*' (line 20 vs line 59). Consider refactoring using a comptime parameter:
pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void {
return escapeRegExpImpl(input, writer, false);
}
pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void {
return escapeRegExpImpl(input, writer, true);
}
fn escapeRegExpImpl(input: []const u8, writer: anytype, comptime expand_star: bool) @TypeOf(writer).Error!void {
var remain = input;
while (strings.indexOfAny(remain, special_characters)) |i| {
try writer.writeAll(remain[0..i]);
switch (remain[i]) {
'|', '\\', '{', '}', '(', ')', '[', ']', '^', '$', '+', '?', '.',
=> |c| try writer.writeAll(&.{ '\\', c }),
'*' => try writer.writeAll(if (comptime expand_star) ".*" else &.{ '\\', '*' }),
'-' => try writer.writeAll("\\x2d"),
else => unreachable,
}
remain = remain[i + 1 ..];
}
try writer.writeAll(remain);
}🤖 Prompt for AI Agents
In src/string/escapeRegExp.zig around lines 3 to 72, both functions duplicate
the same loop and switch logic and only differ in how '*' is handled; refactor
by creating a single helper fn escapeRegExpImpl with a comptime parameter (e.g.,
expand_star: bool) that the two public functions call with true/false, move the
shared loop/switch into that impl, use a comptime branch to emit ".*" when
expand_star is true or backslash-escaped '*' otherwise, preserve the '-' and
other escapes, and keep the debug/unreachable behavior as before.
| @@ -0,0 +1,76 @@ | |||
| const special_characters = "|\\{}()[]^$+*?.-"; | |||
|
|
|||
| pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add documentation for public API.
These functions are exported as public API but lack doc comments. Document the purpose, parameters, and special character handling.
+/// Escapes regex special characters in the input string for use in a regular expression.
+/// Special characters: | \ { } ( ) [ ] ^ $ + * ? . -
+/// The '-' character is escaped as \x2d to avoid range interpretation issues.
pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | |
| /// Escapes regex special characters in the input string for use in a regular expression. | |
| /// Special characters: | \ { } ( ) [ ] ^ $ + * ? . - | |
| /// The '-' character is escaped as \x2d to avoid range interpretation issues. | |
| pub fn escapeRegExp(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { |
🤖 Prompt for AI Agents
In src/string/escapeRegExp.zig around line 3, the public function escapeRegExp
lacks a doc comment; add a Zig doc comment (///) immediately above the pub fn
that succinctly states the function purpose (escape characters so the input can
be used in a regular expression), documents parameters (input: []const u8 —
source string; writer: anytype — writer to receive escaped output), lists
behavior for special regex characters that are escaped and any characters left
unchanged, documents error behavior/returns (propagates writer errors via
@TypeOf(writer).Error), and optionally gives a short usage example; keep the
comment concise and formatted as standard Zig documentation.
| else => |c| { | ||
| if (comptime Environment.isDebug) { | ||
| unreachable; | ||
| } | ||
| try writer.writeByte(c); | ||
| }, |
There was a problem hiding this comment.
Inconsistent behavior between debug and release builds.
The else branch is marked unreachable only in debug mode but falls back to writeByte(c) in release mode. This creates different behavior across build types and suggests a logic error.
If special_characters is exhaustive (all characters that indexOfAny can find are handled in the switch), the else branch should always be unreachable in all build modes. If it's not exhaustive, the behavior should be consistent.
Apply this diff to make the behavior consistent:
- else => |c| {
- if (comptime Environment.isDebug) {
- unreachable;
- }
- try writer.writeByte(c);
- },
+ else => unreachable, // all special_characters are handled aboveIf characters beyond special_characters need handling, update both the constant and switch cases to be exhaustive.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else => |c| { | |
| if (comptime Environment.isDebug) { | |
| unreachable; | |
| } | |
| try writer.writeByte(c); | |
| }, | |
| else => unreachable, // all special_characters are handled above |
🤖 Prompt for AI Agents
In src/string/escapeRegExp.zig around lines 25-30, the else branch currently
uses comptime Environment.isDebug to call unreachable in debug builds but falls
back to writer.writeByte(c) in release builds, causing inconsistent behavior;
fix this by making the branch behavior consistent: either (A) remove the
comptime check and always call try writer.writeByte(c) so unknown characters are
handled the same in all builds, or (B) make the branch unconditionally
unreachable and ensure the special_characters constant and the switch are
exhaustive by adding any missing characters to the constant and corresponding
cases; pick one option and update the code accordingly (if choosing B, add the
missing characters and cases so no unknown char can reach the else).
| /// '*' becomes '.*' instead of '\\*' | ||
| pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Enhance documentation with proper doc comment.
Replace the inline comment with a proper doc comment that explains the wildcard expansion behavior.
-/// '*' becomes '.*' instead of '\\*'
+/// Similar to escapeRegExp but treats '*' as a package-name wildcard.
+/// The '*' character is expanded to '.*' to match any sequence of characters,
+/// enabling patterns like "@types*" to match "@types/node", "@typescript-eslint/*", etc.
pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// '*' becomes '.*' instead of '\\*' | |
| pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | |
| /// Similar to escapeRegExp but treats '*' as a package-name wildcard. | |
| /// The '*' character is expanded to '.*' to match any sequence of characters, | |
| /// enabling patterns like "@types*" to match "@types/node", "@typescript-eslint/*", etc. | |
| pub fn escapeRegExpForPackageNameMatching(input: []const u8, writer: anytype) @TypeOf(writer).Error!void { | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In src/string/escapeRegExp.zig around lines 38 to 39, replace the inline comment
"/// '*' becomes '.*' instead of '\\*'" with a proper Zig doc comment (///) that
clearly explains the function's wildcard expansion behavior: state that the
function escapes input for RegExp but treats '*' as a wildcard which is expanded
to '.*' rather than escaped, describe expected input and output behavior
concisely, and keep it adjacent to the function signature so external docs pick
it up.
dylan-conway
changed the title
publicHoistPatternpublicHoistPattern and hoistPattern
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ini.zig (1)
1072-1100: Do not wipe existing.npmrcdiagnostics on matcher parse failures.
log.reset()still erases every warning/error accumulated while parsing the same.npmrc, so any unrelated diagnostics disappear the moment a hoist-pattern is invalid. Drop the reset and just returnnull(or let the matcher error stand) so earlier messages survive.- ) catch |err| switch (err) { - error.OutOfMemory => |oom| return oom, - error.InvalidRegExp, error.UnexpectedExpr => patterns: { - log.reset(); - break :patterns null; - }, - }; + ) catch |err| switch (err) { + error.OutOfMemory => |oom| return oom, + error.InvalidRegExp, error.UnexpectedExpr => null, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
src/api/schema.zig(2 hunks)src/bunfig.zig(1 hunks)src/ini.zig(1 hunks)src/install/PackageManager/PackageManagerOptions.zig(2 hunks)src/install/isolated_install.zig(3 hunks)src/install/isolated_install/Installer.zig(2 hunks)src/install/isolated_install/Store.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/isolated_install/Store.zigsrc/install/isolated_install/Installer.zigsrc/ini.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/bunfig.zigsrc/install/isolated_install.zigsrc/api/schema.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/isolated_install/Store.zigsrc/install/isolated_install/Installer.zigsrc/ini.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/bunfig.zigsrc/install/isolated_install.zigsrc/api/schema.zig
🧠 Learnings (1)
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to src/js/bun/** : Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Applied to files:
src/api/schema.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (3)
src/install/PackageManager/PackageManagerOptions.zig (1)
74-75: LGTM! Clean addition of hoisting pattern configuration.The new
public_hoist_patternandhoist_patternfields follow the established pattern for optional configuration. The propagation in theloadfunction correctly transfers values from the config to the options struct, mirroring the approach used for other configuration fields.Also applies to: 392-398
src/install/isolated_install.zig (2)
415-419: LGTM! Proper memory management for hoisting state.The hash maps are correctly initialized with
manager.allocatorand properly deferred for cleanup, following Zig memory management best practices.
514-527: LGTM! Hoisting decision logic correctly handles deduplication.The logic appropriately:
- Defaults
hoistedto true whenhoist_patternis null (backward compatibility)- Uses
hidden_hoistedto ensure only the first matching transitive dependency per package name is marked as hoisted- This prevents duplicate hoisting of packages with the same name
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/install/isolated_install.zig (1)
557-579: Public hoist injection logic looks correct; prior safety concern resolved.
- Guard on
new_entry_dep_id != invalid_dependency_idis present; the earlier consistency issue is addressed.- Root direct deps are recorded to
public_hoisted, and transitive public hoists are gated by it to avoid replacement.Please confirm:
Store.Entry.DependenciesOrderedArraySetCtxuniqueness compares by package name (not just dep_id) so a transitive injected dep cannot replace a different direct root dep version.- For root → workspace direct deps (skipped via
break :skip_adding_dependency), is there any other guard to prevent a same‑name transitive dep from being public‑hoisted and overshadowing the workspace link? If not, consider marking such names inpublic_hoistedbefore the break.You can locate the comparator/equality for the ordered set with:
#!/bin/bash rg -n -C3 'DependenciesOrderedArraySetCtx|OrderedArraySetCtx' --type=zig rg -n -C5 'insert\\s*\\(.*DependenciesOrderedArraySetCtx' --type=zig
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/isolated_install.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/isolated_install.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/isolated_install.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (1)
src/install/isolated_install.zig (1)
534-534: LGTM: propagate hoist decision onto Entry.The new
.hoistedfield cleanly carries the decision forward.
| var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | ||
| defer public_hoisted.deinit(); | ||
|
|
||
| var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | ||
| defer hidden_hoisted.deinit(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify map intent (brief comments).
Add short comments explaining each map’s role (guards against replacement vs. determinism for hidden hoists). This improves readability in a complex area.
- var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator);
+ // public_hoisted: names owned by root (direct deps) and names already public‑hoisted.
+ // Prevents public hoist from replacing direct root dependencies.
+ var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator);
defer public_hoisted.deinit();
- var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator);
+ // hidden_hoisted: names already marked hoisted to node_modules/.bun/node_modules.
+ // Ensures determinism (first writer wins) for hidden hoists.
+ var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator);
defer hidden_hoisted.deinit();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | |
| defer public_hoisted.deinit(); | |
| var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | |
| defer hidden_hoisted.deinit(); | |
| // public_hoisted: names owned by root (direct deps) and names already public-hoisted. | |
| // Prevents public hoist from replacing direct root dependencies. | |
| var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | |
| defer public_hoisted.deinit(); | |
| // hidden_hoisted: names already marked hoisted to node_modules/.bun/node_modules. | |
| // Ensures determinism (first writer wins) for hidden hoists. | |
| var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); | |
| defer hidden_hoisted.deinit(); |
🤖 Prompt for AI Agents
In src/install/isolated_install.zig around lines 415 to 419, the two
StringArrayHashMap declarations lack inline comments explaining their intent;
add short comments immediately above or on the same line as each declaration:
for public_hoisted note it “guards against replacement of public hoisted
entries” (or similar concise phrase), and for hidden_hoisted note it “records
hidden hoisted entries to ensure deterministic ordering” (or similar concise
phrase), keeping the comments brief and focused on role to improve readability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/install/isolated_install.zig (1)
415-419: Brief comments for map intent (repeat).Add one-line comments clarifying each map’s role (guard against replacing root deps vs. determinism for hidden hoists). Improves readability in a complex area.
Apply this diff:
- var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); + // public_hoisted: names owned by root (direct deps) and already public‑hoisted; prevents replacement + var public_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); defer public_hoisted.deinit(); - var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); + // hidden_hoisted: names already hoisted into node_modules/.bun/node_modules; first‑writer wins (determinism) + var hidden_hoisted: bun.StringArrayHashMap(void) = .init(manager.allocator); defer hidden_hoisted.deinit();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/isolated_install.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/isolated_install.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/isolated_install.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (2)
src/install/isolated_install.zig (2)
539-539: Wire-up of Store.Entry.hoisted: LGTM.Field set correctly from computed flag.
514-532: Confirm root-deps hoisting semantics: verify that direct root dependencies (parent ==.root) are intentionally eligible for hoisting; if not, add a guard to skip hoisting whenparent == .root.
| if (new_entry_dep_id != invalid_dependency_id) { | ||
| if (entry.entry_parent_id == .root) { | ||
| // make sure direct dependencies are not replaced | ||
| const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf); | ||
| try public_hoisted.put(dep_name, {}); | ||
| } else { | ||
| // transitive dependencies (also direct dependencies of workspaces!) | ||
| const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf); | ||
| if (manager.options.public_hoist_pattern) |public_hoist_pattern| { | ||
| if (public_hoist_pattern.isMatch(dep_name)) { | ||
| const hoist_entry = try public_hoisted.getOrPut(dep_name); | ||
| if (!hoist_entry.found_existing) { | ||
| try entry_dependencies[0].insert( | ||
| lockfile.allocator, | ||
| .{ .entry_id = new_entry_id, .dep_id = new_entry_dep_id }, | ||
| &ctx, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use getOrPut for root insert; avoid double key copies and duplicate dep_name eval.
Saves allocations on existing keys and trims duplication.
Apply this diff:
- if (new_entry_dep_id != invalid_dependency_id) {
- if (entry.entry_parent_id == .root) {
- // make sure direct dependencies are not replaced
- const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf);
- try public_hoisted.put(dep_name, {});
- } else {
- // transitive dependencies (also direct dependencies of workspaces!)
- const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf);
- if (manager.options.public_hoist_pattern) |public_hoist_pattern| {
- if (public_hoist_pattern.isMatch(dep_name)) {
- const hoist_entry = try public_hoisted.getOrPut(dep_name);
- if (!hoist_entry.found_existing) {
- try entry_dependencies[0].insert(
- lockfile.allocator,
- .{ .entry_id = new_entry_id, .dep_id = new_entry_dep_id },
- &ctx,
- );
- }
- }
- }
- }
- }
+ if (new_entry_dep_id != invalid_dependency_id) {
+ const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf);
+ if (entry.entry_parent_id == .root) {
+ // mark direct dependencies as reserved to prevent replacement
+ _ = try public_hoisted.getOrPut(dep_name);
+ } else {
+ // transitive dependencies (also direct dependencies of workspaces!)
+ if (manager.options.public_hoist_pattern) |public_hoist_pattern| {
+ if (public_hoist_pattern.isMatch(dep_name)) {
+ const hoist_entry = try public_hoisted.getOrPut(dep_name);
+ if (!hoist_entry.found_existing) {
+ try entry_dependencies[0].insert(
+ lockfile.allocator,
+ .{ .entry_id = new_entry_id, .dep_id = new_entry_dep_id },
+ &ctx,
+ );
+ }
+ }
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (new_entry_dep_id != invalid_dependency_id) { | |
| if (entry.entry_parent_id == .root) { | |
| // make sure direct dependencies are not replaced | |
| const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf); | |
| try public_hoisted.put(dep_name, {}); | |
| } else { | |
| // transitive dependencies (also direct dependencies of workspaces!) | |
| const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf); | |
| if (manager.options.public_hoist_pattern) |public_hoist_pattern| { | |
| if (public_hoist_pattern.isMatch(dep_name)) { | |
| const hoist_entry = try public_hoisted.getOrPut(dep_name); | |
| if (!hoist_entry.found_existing) { | |
| try entry_dependencies[0].insert( | |
| lockfile.allocator, | |
| .{ .entry_id = new_entry_id, .dep_id = new_entry_dep_id }, | |
| &ctx, | |
| ); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if (new_entry_dep_id != invalid_dependency_id) { | |
| const dep_name = dependencies[new_entry_dep_id].name.slice(string_buf); | |
| if (entry.entry_parent_id == .root) { | |
| // mark direct dependencies as reserved to prevent replacement | |
| _ = try public_hoisted.getOrPut(dep_name); | |
| } else { | |
| // transitive dependencies (also direct dependencies of workspaces!) | |
| if (manager.options.public_hoist_pattern) |public_hoist_pattern| { | |
| if (public_hoist_pattern.isMatch(dep_name)) { | |
| const hoist_entry = try public_hoisted.getOrPut(dep_name); | |
| if (!hoist_entry.found_existing) { | |
| try entry_dependencies[0].insert( | |
| lockfile.allocator, | |
| .{ .entry_id = new_entry_id, .dep_id = new_entry_dep_id }, | |
| &ctx, | |
| ); | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/install/isolated_install.zig around lines 563 to 584, the code currently
computes dep_name twice and does a plain put for root entries which can cause
duplicate key copies and extra allocations; update the block so dep_name is
computed once, then for root entries use getOrPut (or equivalent) instead of put
to avoid inserting duplicate keys and extra allocations, and for non-root
entries keep the public_hoist_pattern match logic but reuse the single dep_name
value and call getOrPut when interacting with public_hoisted; ensure you only
insert into entry_dependencies when getOrPut indicates the key was newly
created.
2 participants
What does this PR do?
Adds support for
publicHoistPatterninbunfig.tomlandpublic-hoist-patternfrom.npmrc. This setting allows you to select transitive packages to hoist to the root node_modules making them available for all workspace packages.publicHoistPatternonly affects the isolated linker.Adds
hoistPattern.hoistPatternis the same aspublicHoistPattern, but applies to thenode_modules/.bun/node_modulesdirectory instead of the root node_modules. Also the default value ofhoistPatternis*(everything is hoisted tonode_modules/.bun/node_modulesby default).Fixes a determinism issue constructing the
node_modules/.bun/node_modulesdirectory.closes #23481
closes #6160
closes #23548
How did you verify your code works?
Added tests for
*