fix(repl): duplicate property names in completions#27515
fix(repl): duplicate property names in completions#27515HK-SHAO wants to merge 1 commit intooven-sh:mainfrom
Conversation
Added a HashSet to track property names and avoid duplicates in completions.
WalkthroughSwitches REPL completion enumeration to use each object's own property list (JSObject::getOwnPropertyNames) for the target object and each prototype, preventing duplicate completion entries from traversing the prototype chain twice. No exported/public signatures were changed. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ 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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/bindings.cpp (1)
6262-6273:⚠️ Potential issue | 🟠 MajorDuplicate completions can still occur across prototype levels.
Line 6265 collects each prototype’s own names, but Line 6271 appends them without a global seen-set. If a name exists on multiple prototypes (e.g.
toString), duplicates still appear.💡 Proposed fix
@@ +#include "wtf/HashSet.h" @@ JSC::PropertyNameArrayBuilder propertyNames(vm, JSC::PropertyNameMode::Strings, JSC::PrivateSymbolMode::Exclude); JSObject::getOwnPropertyNames(object, globalObject, propertyNames, DontEnumPropertiesMode::Include); @@ unsigned completionIndex = 0; + WTF::HashSet<WTF::String> addedNames; for (const auto& propertyName : propertyNames) { WTF::String name = propertyName.string(); - if (prefix.isEmpty() || name.startsWith(prefix)) { + if ((prefix.isEmpty() || name.startsWith(prefix)) && !addedNames.contains(name)) { + addedNames.add(name); completions->putDirectIndex(globalObject, completionIndex++, JSC::jsString(vm, name)); RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(JSC::jsUndefined())); } } @@ for (const auto& propertyName : protoNames) { WTF::String name = propertyName.string(); - if (prefix.isEmpty() || name.startsWith(prefix)) { + if ((prefix.isEmpty() || name.startsWith(prefix)) && !addedNames.contains(name)) { + addedNames.add(name); completions->putDirectIndex(globalObject, completionIndex++, JSC::jsString(vm, name)); RETURN_IF_EXCEPTION(scope, JSC::JSValue::encode(completions)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/bindings.cpp` around lines 6262 - 6273, Prototype-level property names are collected per-prototype into protoNames but duplicates can still be emitted across prototype chain; add a seen set (e.g., WTF::HashSet<WTF::String> seenNames) before iterating prototypes and, inside the loop over protoNames (where propertyName/string() is used), skip any name already in seenNames and only insert new names into seenNames before calling completions->putDirectIndex(...), preserving checks for prefix, RETURN_IF_EXCEPTION and incrementing completionIndex; reference symbols: JSC::PropertyNameArrayBuilder protoNames, propertyName, completions, completionIndex, prefix, protoObj.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Around line 6262-6273: Prototype-level property names are collected
per-prototype into protoNames but duplicates can still be emitted across
prototype chain; add a seen set (e.g., WTF::HashSet<WTF::String> seenNames)
before iterating prototypes and, inside the loop over protoNames (where
propertyName/string() is used), skip any name already in seenNames and only
insert new names into seenNames before calling completions->putDirectIndex(...),
preserving checks for prefix, RETURN_IF_EXCEPTION and incrementing
completionIndex; reference symbols: JSC::PropertyNameArrayBuilder protoNames,
propertyName, completions, completionIndex, prefix, protoObj.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/bindings.cpp
What does this PR do?
Try to fix duplicate property names in REPL tab completion when properties appear multiple times in the prototype chain. #27516
Problem: When using tab completion in the Bun REPL, property names were being duplicated in the completion suggestions. For example, entering
{}.toand pressing Tab would show:This happened because the completion function traversed the entire prototype chain and added properties from each level without checking for duplicates. Since properties like
toStringexist in multiple prototype objects (e.g., bothFunction.prototypeandObject.prototype), they would appear multiple times in the completion list.Solution: Introduce a
WTF::HashSet<WTF::String>to track property names that have already been added to the completion list. Before adding any property name to the completion array, the code now checks if it's already in the set. Only unique property names are added, ensuring each property appears exactly once regardless of how many times it appears in the prototype chain.Changes:
#include "wtf/HashSet.h"to the bindings.cpp header includesBun__REPL__getCompletionsfunction insrc/bun.js/bindings/bindings.cppto use a HashSet for deduplication!addedNames.contains(name)before adding property names to the completion arrayaddedNames.add(name)call when a property is successfully added to track itHow did you verify your code works?
bun run zig:check-allandbun run buildbuild/debug/bun-debug replin my terminal