search

tc39 / proposal-regexp-named-groups

Named capture groups for JavaScript RegExps
https://tc39.github.io/proposal-regexp-named-groups/
222 stars 21 forks source link

GetSubstitution: Non-{Object,undefined} namedCaptures argument #15

Closed schuay closed 7 years ago

schuay commented 7 years ago

GetSubstitution currently assumes namedCaptures is either an Object or undefined, and goes on to call Get(namedCaptures, groupName).

WDYT about throwing a TypeError in RegExp.prototype [ @@replace ] if namedCaptures is not an Object?

schuay commented 7 years ago

At step 14.l.i (just before the GetSubstitution call).

schuay commented 7 years ago

Another option would be to call ToObject.

hashseed commented 7 years ago

Is there a way namedCaptures is neither Object nor undefined?

schuay commented 7 years ago

AFAIK a subclassed RegExp (with a custom exec method) can return a result where Get(result, "groups") can return arbitrary values.

littledan commented 7 years ago

Another option would be to call RequireObjectCoercible here. That's what's often called in the spec when we want to do an object-like operation on something. Does that seem reasonable to you here, @schuay ?

schuay commented 7 years ago

If I understood correctly, RequireObjectCoercible still needs to be followed up with a call to ToObject before Get. What would be the advantage of doing so?

littledan commented 7 years ago

@schuay Well, GetV works with primitives--anyway, I think I was mistaken here, and ToObject is better. I'll write a patch for this.

schuay commented 7 years ago

Minor point: 769a71c requires calling ToObject(namedCaptures) for each encountered '$<'. Wouldn't it make more sense to call ToObject once in the prologue, i.e. somewhere before step 11 in https://tc39.github.io/ecma262/#sec-getsubstitution?

I'm a bit confused that GetSubstitution algorithm isn't shown in https://tc39.github.io/proposal-regexp-named-groups/#sec-getsubstitution. I guess that's because of the diff formatting?

littledan commented 7 years ago

@schuay I implemented your suggestion; it is definitely observable, and seems reasonable to me. I had only left out the GetSubstitution algorithm to avoid useless duplication of text from the main spec; now it's included and has this modification in it.