RegExpBuiltinExec should create "groups" property unconditionally

https://bugs.webkit.org/show_bug.cgi?id=204067

Patch by Alexey Shvayka <[email protected]> on 2019-11-12
Reviewed by Ross Kirsling.

JSTests:

* test262/expectations.yaml: Mark 4 test cases as passing.

Source/JavaScriptCore:

After RegExp named capture groups were initially implemented in JSC, the spec was changed
to unconditionally create "groups" property.
(tc39/proposal-regexp-named-groups#34)

This patch implements the change (that was shipped by V8), reducing number of structures
we use for RegExpMatchesArray, and also sets [[Prototype]] of "groups" object to `null`.
(step 24 of https://tc39.es/ecma262/#sec-regexpbuiltinexec)

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::init):
(JSC::JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut):
(JSC::JSGlobalObject::visitChildren):
* runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::regExpMatchesArrayStructure const):
(JSC::JSGlobalObject::regExpMatchesArrayWithGroupsStructure const): Deleted.
* runtime/RegExpMatchesArray.cpp:
(JSC::createStructureImpl):
(JSC::createRegExpMatchesArrayWithGroupsStructure): Deleted.
(JSC::createRegExpMatchesArrayWithGroupsSlowPutStructure): Deleted.
* runtime/RegExpMatchesArray.h:
(JSC::createRegExpMatchesArray):
* runtime/StringPrototype.cpp:
(JSC::replaceUsingRegExpSearch):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@252374 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog

@@ -1,3 +1,12 @@
+2019-11-12  Alexey Shvayka  <[email protected]>
+
+        RegExpBuiltinExec should create "groups" property unconditionally
+        https://bugs.webkit.org/show_bug.cgi?id=204067
+
+        Reviewed by Ross Kirsling.
+
+        * test262/expectations.yaml: Mark 4 test cases as passing.
+
 2019-11-11  Ross Kirsling  <[email protected]>
 
         UTC offset for Samoa is miscalculated when !HAVE(TIMEGM)

JSTests/test262/expectations.yaml

@@ -1183,12 +1183,6 @@ test/built-ins/RegExp/named-groups/groups-object-subclass-sans.js:
 test/built-ins/RegExp/named-groups/groups-object-subclass.js:
   default: 'Test262Error: Expected SameValue(«b», «$<a>») to be true'
   strict mode: 'Test262Error: Expected SameValue(«b», «$<a>») to be true'
-test/built-ins/RegExp/named-groups/groups-object-undefined.js:
-  default: 'Test262Error: Expected true but got false'
-  strict mode: 'Test262Error: Expected true but got false'
-test/built-ins/RegExp/named-groups/groups-object.js:
-  default: 'Test262Error: Expected SameValue(«null», «[object Object]») to be true'
-  strict mode: 'Test262Error: Expected SameValue(«null», «[object Object]») to be true'
 test/built-ins/RegExp/named-groups/string-replace-nocaptures.js:
   default: 'Test262Error: Expected SameValue(«$<snd>$<fst>cd», «$<$<fst>cd») to be true'
   strict mode: 'Test262Error: Expected SameValue(«$<snd>$<fst>cd», «$<$<fst>cd») to be true'

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,38 @@
+2019-11-12  Alexey Shvayka  <[email protected]>
+
+        RegExpBuiltinExec should create "groups" property unconditionally
+        https://bugs.webkit.org/show_bug.cgi?id=204067
+
+        Reviewed by Ross Kirsling.
+
+        After RegExp named capture groups were initially implemented in JSC, the spec was changed
+        to unconditionally create "groups" property.
+        (https://github.com/tc39/proposal-regexp-named-groups/issues/34)
+
+        This patch implements the change (that was shipped by V8), reducing number of structures
+        we use for RegExpMatchesArray, and also sets [[Prototype]] of "groups" object to `null`.
+        (step 24 of https://tc39.es/ecma262/#sec-regexpbuiltinexec)
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGStrengthReductionPhase.cpp:
+        (JSC::DFG::StrengthReductionPhase::handleNode):
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::init):
+        (JSC::JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut):
+        (JSC::JSGlobalObject::visitChildren):
+        * runtime/JSGlobalObject.h:
+        (JSC::JSGlobalObject::regExpMatchesArrayStructure const):
+        (JSC::JSGlobalObject::regExpMatchesArrayWithGroupsStructure const): Deleted.
+        * runtime/RegExpMatchesArray.cpp:
+        (JSC::createStructureImpl):
+        (JSC::createRegExpMatchesArrayWithGroupsStructure): Deleted.
+        (JSC::createRegExpMatchesArrayWithGroupsSlowPutStructure): Deleted.
+        * runtime/RegExpMatchesArray.h:
+        (JSC::createRegExpMatchesArray):
+        * runtime/StringPrototype.cpp:
+        (JSC::replaceUsingRegExpSearch):
+
 2019-11-11  Yusuke Suzuki  <[email protected]>
 
         Unreviewed, fix incorrect assertion

Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

@@ -2356,7 +2356,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                     m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
                     RegisteredStructureSet structureSet;
                     structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayStructure()));
-                    structureSet.add(m_graph.registerStructure(globalObject->regExpMatchesArrayWithGroupsStructure()));
                     setForNode(node, structureSet);
                     forNode(node).merge(SpecOther);
                     break;

Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp

@@ -606,12 +606,7 @@ class StrengthReductionPhase : public Phase {
 
                 m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
 
-                Structure* structure;
-                if ((m_node->op() == RegExpExec || m_node->op() == RegExpExecNonGlobalOrSticky) && regExp->hasNamedCaptures())
-                    structure = globalObject->regExpMatchesArrayWithGroupsStructure();
-                else
-                    structure = globalObject->regExpMatchesArrayStructure();
-
+                Structure* structure = globalObject->regExpMatchesArrayStructure();
                 if (structure->indexingType() != ArrayWithContiguous) {
                     // This is further protection against a race with haveABadTime.
                     if (verbose)

Source/JavaScriptCore/runtime/JSGlobalObject.cpp

@@ -708,7 +708,6 @@ void JSGlobalObject::init(VM& vm)
     m_regExpPrototype.set(vm, this, RegExpPrototype::create(vm, this, RegExpPrototype::createStructure(vm, this, m_objectPrototype.get())));
     m_regExpStructure.set(vm, this, RegExpObject::createStructure(vm, this, m_regExpPrototype.get()));
     m_regExpMatchesArrayStructure.set(vm, this, createRegExpMatchesArrayStructure(vm, this));
-    m_regExpMatchesArrayWithGroupsStructure.set(vm, this, createRegExpMatchesArrayWithGroupsStructure(vm, this));
 
     m_moduleRecordStructure.initLater(
         [] (const Initializer<Structure>& init) {
@@ -1557,8 +1556,6 @@ void JSGlobalObject::fireWatchpointAndMakeAllArrayStructuresSlowPut(VM& vm)
     Structure* slowPutStructure;
     slowPutStructure = createRegExpMatchesArraySlowPutStructure(vm, this);
     m_regExpMatchesArrayStructure.set(vm, this, slowPutStructure);
-    slowPutStructure = createRegExpMatchesArrayWithGroupsSlowPutStructure(vm, this);
-    m_regExpMatchesArrayWithGroupsStructure.set(vm, this, slowPutStructure);
     slowPutStructure = ClonedArguments::createSlowPutStructure(vm, this, m_objectPrototype.get());
     m_clonedArgumentsStructure.set(vm, this, slowPutStructure);
 
@@ -1818,7 +1815,6 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
     visitor.append(thisObject->m_asyncGeneratorStructure);
     thisObject->m_iteratorResultObjectStructure.visit(visitor);
     visitor.append(thisObject->m_regExpMatchesArrayStructure);
-    visitor.append(thisObject->m_regExpMatchesArrayWithGroupsStructure);
     thisObject->m_moduleRecordStructure.visit(visitor);
     thisObject->m_moduleNamespaceObjectStructure.visit(visitor);
     thisObject->m_proxyObjectStructure.visit(visitor);

Source/JavaScriptCore/runtime/JSGlobalObject.h

@@ -376,7 +376,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
     WriteBarrier<Structure> m_asyncGeneratorStructure;
     LazyProperty<JSGlobalObject, Structure> m_iteratorResultObjectStructure;
     WriteBarrier<Structure> m_regExpMatchesArrayStructure;
-    WriteBarrier<Structure> m_regExpMatchesArrayWithGroupsStructure;
     LazyProperty<JSGlobalObject, Structure> m_moduleRecordStructure;
     LazyProperty<JSGlobalObject, Structure> m_moduleNamespaceObjectStructure;
     LazyProperty<JSGlobalObject, Structure> m_proxyObjectStructure;
@@ -754,7 +753,6 @@ class JSGlobalObject : public JSSegmentedVariableObject {
     Structure* stringObjectStructure() const { return m_stringObjectStructure.get(); }
     Structure* iteratorResultObjectStructure() const { return m_iteratorResultObjectStructure.get(this); }
     Structure* regExpMatchesArrayStructure() const { return m_regExpMatchesArrayStructure.get(); }
-    Structure* regExpMatchesArrayWithGroupsStructure() const { return m_regExpMatchesArrayWithGroupsStructure.get(); }
     Structure* moduleRecordStructure() const { return m_moduleRecordStructure.get(this); }
     Structure* moduleNamespaceObjectStructure() const { return m_moduleNamespaceObjectStructure.get(this); }
     Structure* proxyObjectStructure() const { return m_proxyObjectStructure.get(this); }

Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp

@@ -70,24 +70,16 @@ JSArray* createEmptyRegExpMatchesArray(JSGlobalObject* globalObject, JSString* i
     return array;
 }
 
-enum class ShouldCreateGroups { No, Yes };
-
-static Structure* createStructureImpl(VM& vm, JSGlobalObject* globalObject, IndexingType indexingType, ShouldCreateGroups shouldCreateGroups = ShouldCreateGroups::No)
+static Structure* createStructureImpl(VM& vm, JSGlobalObject* globalObject, IndexingType indexingType)
 {
     Structure* structure = globalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType);
     PropertyOffset offset;
     structure = Structure::addPropertyTransition(vm, structure, vm.propertyNames->index, 0, offset);
     ASSERT(offset == RegExpMatchesArrayIndexPropertyOffset);
     structure = Structure::addPropertyTransition(vm, structure, vm.propertyNames->input, 0, offset);
     ASSERT(offset == RegExpMatchesArrayInputPropertyOffset);
-    switch (shouldCreateGroups) {
-    case ShouldCreateGroups::Yes:
-        structure = Structure::addPropertyTransition(vm, structure, vm.propertyNames->groups, 0, offset);
-        ASSERT(offset == RegExpMatchesArrayGroupsPropertyOffset);
-        break;
-    case ShouldCreateGroups::No:
-        break;
-    }
+    structure = Structure::addPropertyTransition(vm, structure, vm.propertyNames->groups, 0, offset);
+    ASSERT(offset == RegExpMatchesArrayGroupsPropertyOffset);
     return structure;
 }
 
@@ -101,14 +93,4 @@ Structure* createRegExpMatchesArraySlowPutStructure(VM& vm, JSGlobalObject* glob
     return createStructureImpl(vm, globalObject, ArrayWithSlowPutArrayStorage);
 }
 
-Structure* createRegExpMatchesArrayWithGroupsStructure(VM& vm, JSGlobalObject* globalObject)
-{
-    return createStructureImpl(vm, globalObject, ArrayWithContiguous, ShouldCreateGroups::Yes);
-}
-
-Structure* createRegExpMatchesArrayWithGroupsSlowPutStructure(VM& vm, JSGlobalObject* globalObject)
-{
-    return createStructureImpl(vm, globalObject, ArrayWithSlowPutArrayStorage, ShouldCreateGroups::Yes);
-}
-
 } // namespace JSC

Source/JavaScriptCore/runtime/RegExpMatchesArray.h

@@ -24,6 +24,7 @@
 #include "JSArray.h"
 #include "JSCInlines.h"
 #include "JSGlobalObject.h"
+#include "ObjectConstructor.h"
 #include "RegExpInlines.h"
 #include "RegExpObject.h"
 
@@ -82,18 +83,13 @@ ALWAYS_INLINE JSArray* createRegExpMatchesArray(
 
     unsigned numSubpatterns = regExp->numSubpatterns();
     bool hasNamedCaptures = regExp->hasNamedCaptures();
-    JSObject* groups = nullptr;
+    JSObject* groups = hasNamedCaptures ? constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure()) : nullptr;
     Structure* matchStructure = globalObject->regExpMatchesArrayStructure();
-    if (hasNamedCaptures) {
-        groups = JSFinalObject::create(vm, JSFinalObject::createStructure(vm, globalObject, globalObject->objectPrototype(), 0));
-        matchStructure = globalObject->regExpMatchesArrayWithGroupsStructure();
-    }
 
     auto setProperties = [&] () {
         array->putDirect(vm, RegExpMatchesArrayIndexPropertyOffset, jsNumber(result.start));
         array->putDirect(vm, RegExpMatchesArrayInputPropertyOffset, input);
-        if (hasNamedCaptures)
-            array->putDirect(vm, RegExpMatchesArrayGroupsPropertyOffset, groups);
+        array->putDirect(vm, RegExpMatchesArrayGroupsPropertyOffset, hasNamedCaptures ? groups : jsUndefined());
 
         ASSERT(!array->butterfly()->indexingHeader()->preCapacity(matchStructure));
         auto capacity = matchStructure->outOfLineCapacity();
@@ -155,7 +151,7 @@ ALWAYS_INLINE JSArray* createRegExpMatchesArray(
 
     // We initialize the groups object late as it could allocate, which with the current API could cause
     // allocations.
-    if (groups) {
+    if (hasNamedCaptures) {
         for (unsigned i = 1; i <= numSubpatterns; ++i) {
             String groupName = regExp->getCaptureGroupName(i);
             if (!groupName.isEmpty())
@@ -179,7 +175,5 @@ inline JSArray* createRegExpMatchesArray(JSGlobalObject* globalObject, JSString*
 JSArray* createEmptyRegExpMatchesArray(JSGlobalObject*, JSString*, RegExp*);
 Structure* createRegExpMatchesArrayStructure(VM&, JSGlobalObject*);
 Structure* createRegExpMatchesArraySlowPutStructure(VM&, JSGlobalObject*);
-Structure* createRegExpMatchesArrayWithGroupsStructure(VM&, JSGlobalObject*);
-Structure* createRegExpMatchesArrayWithGroupsSlowPutStructure(VM&, JSGlobalObject*);
 
 } // namespace JSC

Source/JavaScriptCore/runtime/StringPrototype.cpp

@@ -39,6 +39,7 @@
 #include "JSGlobalObjectFunctions.h"
 #include "JSStringIterator.h"
 #include "Lookup.h"
+#include "ObjectConstructor.h"
 #include "ObjectPrototype.h"
 #include "ParseInt.h"
 #include "PropertyNameArray.h"
@@ -571,11 +572,7 @@ static ALWAYS_INLINE JSString* replaceUsingRegExpSearch(
                 OUT_OF_MEMORY(globalObject, scope);
 
             cachedCall.clearArguments();
-
-            JSObject* groups = nullptr;
-
-            if (hasNamedCaptures)
-                groups = JSFinalObject::create(vm, JSFinalObject::createStructure(vm, globalObject, globalObject->objectPrototype(), 0));
+            JSObject* groups = hasNamedCaptures ? constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure()) : nullptr;
 
             for (unsigned i = 0; i < regExp->numSubpatterns() + 1; ++i) {
                 int matchStart = ovector[i * 2];
@@ -636,10 +633,7 @@ static ALWAYS_INLINE JSString* replaceUsingRegExpSearch(
                     OUT_OF_MEMORY(globalObject, scope);
 
                 MarkedArgumentBuffer args;
-                JSObject* groups = nullptr;
-
-                if (hasNamedCaptures)
-                    groups = JSFinalObject::create(vm, JSFinalObject::createStructure(vm, globalObject, globalObject->objectPrototype(), 0));
+                JSObject* groups = hasNamedCaptures ? constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure()) : nullptr;
 
                 for (unsigned i = 0; i < regExp->numSubpatterns() + 1; ++i) {
                     int matchStart = ovector[i * 2];