Editorial: Extract operation 'ParsePattern'

[jmdyck]

... from common code in IsValidRegularExpressionLiteral and RegExpInitialize.

---

This is the refactoring I referred to over here.

---

I've split the PR into a series of smaller refactorings so that it's easier to follow. I expect these will be squashed together at merge, so I didn't bother prefixing their commit messages with "Editorial:". (Except for the final commit, whose commit message can serve as the message for the squash.)

IVREL = IsValidRegularExpressionLiteral, REI = RegExpInitialize

---

Note that if the parse fails, ParsePattern returns an abrupt completion. This is somewhat odd when it's being called by IsValidRegularExpressionLiteral, because the latter is static semantics. (Its only invocation is from an early error). IsValidRegularExpressionLiteral translates the abrupt completion into *false*, so the abrupt completion doesn't go any further, but you might object to it even existing during static semantics processing. If that's a sticking point, we can replace the abrupt completion with a SyntaxError object or a List of them (as in ParseScript or ParseModule).

[bakkot]

you might object to it even existing during static semantics processing

Yeah, that seems a little unfortunate. What would you think about returning ~empty~ instead?

[jmdyck]

What would you think about returning ~empty~ instead?

Well, any value that can be distinguished from a Parse Node would work, but I think it'd be better to follow the example of ParseScript and ParseModule
(a) for consistency, and
(b) because returning a List of errors is a way to communicate information to RegExpInitialize that it can use in the SyntaxError that it throws. (This is somewhat hand-wavy, since the SyntaxError isn't required to have any specific information in it, but of course it will in practice.)

[bakkot]

for consistency

I think the inconsistency of returning a completion from an operation used in static semantics bothers me more.

because returning a List of errors is a way to communicate information to RegExpInitialize that it can use in the SyntaxError that it throws

Eh, I think implementations can figure out the appropriate error to give without using having to pass around error objects.

All considered I think I would still prefer returning ~empty~, or I guess ~failure~ (as is already used when performing regex matching).

[jmdyck]

for consistency

I think the inconsistency of returning a completion from an operation used in static semantics bothers me more.

Huh? I mean, I understand that returning a completion record bothers you, but I'm not sure how that's relevant to the suggestion to return a List of errors.

All considered I think I would still prefer returning ~empty~, or I guess ~failure~ (as is already used when performing regex matching).

I'd advise against using ~failure~: it already has a defined meaning (the match failed), and that's not this.

[jmdyck]

(force-pushed to resolve a merge conflict)

[bakkot]

A List of Errors is fine by me and the other editors; let's go with that.

[jmdyck]

(force-pushed to resolve merge conflicts and change the return-type to list-of-errors)

[ljharb]

what if it's a List of something else, or an empty List?

It seems like ParsePattern returns either a Parse Node, or not - should this be "is not a Parse Node"?

[bakkot]

It returns either a Parse Node or a List of SyntaxError objects. There's no other case.

We could follow this with an Assert: _parseResult_ is a Parse Node, if you think that's valuable.

[ljharb]

Fair, it just seems too vague to me to assert that it's a List of syntax error objects (without also reading the abstract operation), because of the possibilities that implies.

[bakkot]

@ljharb Would adding Assert: _parseResult_ is a Parse Node as the following line resolve your concern?

[jmdyck]

I think I'd prefer changing the condition to "is not a Parse Node", as suggested by @ljharb. It's semantically equivalent, but easier on the reader: you don't need to look at the definition of ParsePattern to know that the only other possibility is that it's a Parse Node. (And we don't need to Assert that it is.)

[ljharb]

In that case, should we say "throw one of the items in the list as an exception"? The impl has control of what's in the list anyways, so it wouldn't be normative, but would convey the intention you find important.

[bakkot]

There's a little bit of awkwardness there in that for that to be sensible the list has to be non-empty. Which it is guaranteed to be by the implementation of ParsePattern, but you have to look there to know that.

I'd be fine with that, I guess, but would still personally weakly lean towards the If _parseResult_ is a List of *SyntaxError* objects, throw a *SyntaxError* exception form + an assert.

But any of the suggestions here are adequate. I would mostly like us to just pick one and land it.

[michaelficarra]

I think it's valuable to follow up this step with an assertion. I don't care which way around the test is.

[syg]

I feel like any reasonable non-formalist reading of "a List of x objects" excludes the vacuous empty List case, but it doesn't hurt to qualify "a non-empty List of x objects", I guess.

And I agree with @bakkot on:

I kind of prefer writing it this way because it suggests that the error thrown could be one of the errors in the list

So I'd prefer directly talking about errors than asserting it's not a parse node.

[syg]

I should add I'm happy with an additional assert, but I like the condition for throwing checking for a list of errors over checking for not being a parse node.

[syg]

I didn't review too closely to ensure correctness. I gladly trust @bakkot's existing review here that this is correct.

Editorially lgtm with rename for abstract closure.

[syg]

I feel like any reasonable non-formalist reading of "a List of x objects" excludes the vacuous empty List case, but it doesn't hurt to qualify "a non-empty List of x objects", I guess.

And I agree with @bakkot on:

I kind of prefer writing it this way because it suggests that the error thrown could be one of the errors in the list

So I'd prefer directly talking about errors than asserting it's not a parse node.

[syg]

Please do a s/abstract closure/Abstract Closure/g. We recently capitalized it to be consistent with other spec types.

[jmdyck]

force-pushed to:

• resolve a merge conflict (capitalization of "Abstract Closure"),
• insert "non-empty" before "List of SyntaxError objects", and
• add an Assert re _parseResult_ being a Parse Node.

[ljharb]

@jmdyck would you like to rebase this down to a single commit with a nicer commit message than me naively concatenating them? :-)

[jmdyck]

Thanks for the offer. I decided that there wasn't much benefit to enumerating the refactoring steps in the final commit message, but I'll do so here:

---

IsValidRegularExpressionLiteral:

• Introduce an 'Else'
• Introduce _u_
• Rename _pText_ as _patternText_ and introduce _patternText_ into the other arm of the if-else
• Factor out the last sentence of each 'Parse' step

RegExpInitialize:

• Replace _BMP_ with its complement _u_
• Swap the order of the 2 arms of the 'If'
• Rename _pText_ as _patternText_
• Reword a step slightly
• Split the 'If'
• Factor out the last sentence of each 'Parse' step

Extract operation 'ParsePattern'

IsValidRegularExpressionLiteral:

• Simplify