From ff4e0d29439a8b2bd49230f4d638bc8d5ec62c2b Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Wed, 12 Jul 2023 15:45:05 -0400 Subject: [PATCH] LibJS: Avoid creating String object wrappers in iterator helpers This is a normative change in the Iterator Helpers spec. See: https://github.com/tc39/proposal-iterator-helpers/commit/3e275cf --- Userland/Libraries/LibJS/Runtime/Iterator.cpp | 12 ++++++++---- Userland/Libraries/LibJS/Runtime/Iterator.h | 7 ++++++- .../LibJS/Runtime/IteratorConstructor.cpp | 18 +++++++----------- .../LibJS/Runtime/IteratorPrototype.cpp | 4 ++-- .../Tests/builtins/Iterator/Iterator.from.js | 16 ++++++++++++++++ 5 files changed, 39 insertions(+), 18 deletions(-) diff --git a/Userland/Libraries/LibJS/Runtime/Iterator.cpp b/Userland/Libraries/LibJS/Runtime/Iterator.cpp index 9adee1e5ec..bcd0d7ec69 100644 --- a/Userland/Libraries/LibJS/Runtime/Iterator.cpp +++ b/Userland/Libraries/LibJS/Runtime/Iterator.cpp @@ -39,11 +39,15 @@ ThrowCompletionOr get_iterator_direct(VM& vm, Object& object) return iterator_record; } -ThrowCompletionOr get_iterator_flattenable(VM& vm, Value object) +// 2.1.2 GetIteratorFlattenable ( obj, stringHandling ), https://tc39.es/proposal-iterator-helpers/#sec-getiteratorflattenable +ThrowCompletionOr get_iterator_flattenable(VM& vm, Value object, StringHandling string_handling) { - // 1. If obj is not an Object, throw a TypeError exception. - if (!object.is_object()) - return vm.throw_completion(ErrorType::NotAnObject, "obj"sv); + // 1. If obj is not an Object, then + if (!object.is_object()) { + // a. If stringHandling is reject-strings or obj is not a String, throw a TypeError exception. + if (string_handling == StringHandling::RejectStrings || !object.is_string()) + return vm.throw_completion(ErrorType::NotAnObject, "obj"sv); + } // 2. Let method be ? GetMethod(obj, @@iterator). auto method = TRY(object.get_method(vm, vm.well_known_symbol_iterator())); diff --git a/Userland/Libraries/LibJS/Runtime/Iterator.h b/Userland/Libraries/LibJS/Runtime/Iterator.h index 4a0c1874f6..27ca5f3117 100644 --- a/Userland/Libraries/LibJS/Runtime/Iterator.h +++ b/Userland/Libraries/LibJS/Runtime/Iterator.h @@ -35,7 +35,12 @@ private: IteratorRecord m_iterated; // [[Iterated]] }; +enum class StringHandling { + IterateStrings, + RejectStrings, +}; + ThrowCompletionOr get_iterator_direct(VM&, Object&); -ThrowCompletionOr get_iterator_flattenable(VM&, Value); +ThrowCompletionOr get_iterator_flattenable(VM&, Value, StringHandling); } diff --git a/Userland/Libraries/LibJS/Runtime/IteratorConstructor.cpp b/Userland/Libraries/LibJS/Runtime/IteratorConstructor.cpp index 09f97baf73..038f3d47b2 100644 --- a/Userland/Libraries/LibJS/Runtime/IteratorConstructor.cpp +++ b/Userland/Libraries/LibJS/Runtime/IteratorConstructor.cpp @@ -66,27 +66,23 @@ JS_DEFINE_NATIVE_FUNCTION(IteratorConstructor::from) auto object = vm.argument(0); - // 1. If O is a String, set O to ! ToObject(O). - if (object.is_string()) - object = MUST_OR_THROW_OOM(object.to_object(vm)); + // 1. Let iteratorRecord be ? GetIteratorFlattenable(O, iterate-strings). + auto iterator_record = TRY(get_iterator_flattenable(vm, object, StringHandling::IterateStrings)); - // 2. Let iteratorRecord be ? GetIteratorFlattenable(O). - auto iterator_record = TRY(get_iterator_flattenable(vm, object)); - - // 3. Let hasInstance be ? OrdinaryHasInstance(%Iterator%, iteratorRecord.[[Iterator]]). + // 2. Let hasInstance be ? OrdinaryHasInstance(%Iterator%, iteratorRecord.[[Iterator]]). auto has_instance = TRY(ordinary_has_instance(vm, iterator_record.iterator, realm.intrinsics().iterator_constructor())); - // 4. If hasInstance is true, then + // 3. If hasInstance is true, then if (has_instance.is_boolean() && has_instance.as_bool()) { // a. Return iteratorRecord.[[Iterator]]. return iterator_record.iterator; } - // 5. Let wrapper be OrdinaryObjectCreate(%WrapForValidIteratorPrototype%, « [[Iterated]] »). - // 6. Set wrapper.[[Iterated]] to iteratorRecord. + // 4. Let wrapper be OrdinaryObjectCreate(%WrapForValidIteratorPrototype%, « [[Iterated]] »). + // 5. Set wrapper.[[Iterated]] to iteratorRecord. auto wrapper = MUST_OR_THROW_OOM(Iterator::create(realm, realm.intrinsics().wrap_for_valid_iterator_prototype(), move(iterator_record))); - // 7. Return wrapper. + // 6. Return wrapper. return wrapper; } diff --git a/Userland/Libraries/LibJS/Runtime/IteratorPrototype.cpp b/Userland/Libraries/LibJS/Runtime/IteratorPrototype.cpp index e0e2b07789..9cf0215e38 100644 --- a/Userland/Libraries/LibJS/Runtime/IteratorPrototype.cpp +++ b/Userland/Libraries/LibJS/Runtime/IteratorPrototype.cpp @@ -359,8 +359,8 @@ private: if (mapped.is_error()) return iterator.close_result(mapped.release_error()); - // vi. Let innerIterator be Completion(GetIteratorFlattenable(mapped)). - auto inner_iterator = get_iterator_flattenable(vm, mapped.release_value()); + // vi. Let innerIterator be Completion(GetIteratorFlattenable(mapped, reject-strings)). + auto inner_iterator = get_iterator_flattenable(vm, mapped.release_value(), StringHandling::RejectStrings); // vii. IfAbruptCloseIterator(innerIterator, iterated). if (inner_iterator.is_error()) diff --git a/Userland/Libraries/LibJS/Tests/builtins/Iterator/Iterator.from.js b/Userland/Libraries/LibJS/Tests/builtins/Iterator/Iterator.from.js index d471ab48fc..2f5a18e4ab 100644 --- a/Userland/Libraries/LibJS/Tests/builtins/Iterator/Iterator.from.js +++ b/Userland/Libraries/LibJS/Tests/builtins/Iterator/Iterator.from.js @@ -60,6 +60,22 @@ describe("normal behavior", () => { expect(result.done).toBeTrue(); }); + test("does not coerce strings to objects", () => { + const stringIterator = String.prototype[Symbol.iterator]; + let observedType = null; + + Object.defineProperty(String.prototype, Symbol.iterator, { + get() { + "use strict"; + observedType = typeof this; + return stringIterator; + }, + }); + + Iterator.from("ab"); + expect(observedType).toBe("string"); + }); + test("create Iterator from generator", () => { function* generator() { yield 1;