Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spec text for RegExp Modifiers #3221

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rbuckton
Copy link
Contributor

This adds the specification text for the Stage 3 RegExp Modifiers proposal.

Test262 tests can be found at tc39/test262#3960.

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. has test262 tests proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Nov 16, 2023
@ljharb ljharb marked this pull request as draft November 16, 2023 00:28
<emu-grammar>Atom :: `(?` RegularExpressionFlags `:` Disjunction `)`</emu-grammar>
<ul>
<li>
It is a Syntax Error if the source text matched by |RegularExpressionFlags| contains any code point other than `i`, `m`, or `s`, or if it contains the same code point more than once.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It is a Syntax Error if the source text matched by |RegularExpressionFlags| contains any code point other than `i`, `m`, or `s`, or if it contains the same code point more than once.
It is a Syntax Error if the source text matched by |RegularExpressionFlags| contains any code point other than `i`, `m`, or `s`, or contains the same code point more than once.

(for consistency with the others)

Comment on lines +37095 to +37100
1. If _add_ contains *"i"*, set _ignoreCase_ to *true*.
1. If _add_ contains *"m"*, set _multiline_ to *true*.
1. If _add_ contains *"s"*, set _dotAll_ to *true*.
1. If _remove_ contains *"i"*, set _ignoreCase_ to *false*.
1. If _remove_ contains *"m"*, set _multiline_ to *false*.
1. If _remove_ contains *"s"*, set _dotAll_ to *false*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If _add_ contains *"i"*, set _ignoreCase_ to *true*.
1. If _add_ contains *"m"*, set _multiline_ to *true*.
1. If _add_ contains *"s"*, set _dotAll_ to *true*.
1. If _remove_ contains *"i"*, set _ignoreCase_ to *false*.
1. If _remove_ contains *"m"*, set _multiline_ to *false*.
1. If _remove_ contains *"s"*, set _dotAll_ to *false*.
1. If _add_ contains *"i"*, set _ignoreCase_ to *true*.
1. Else if _remove_ contains *"i"*, set _ignoreCase_ to *false*.
1. If _add_ contains *"m"*, set _multiline_ to *true*.
1. Else if _remove_ contains *"m"*, set _multiline_ to *false*.
1. If _add_ contains *"s"*, set _dotAll_ to *true*.
1. Else if _remove_ contains *"s"*, set _dotAll_ to *false*.

Use else-if to further emphasise that add/remove are disjoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this doesn't indicate disjointedness any more than the independent Ifs do. Also, this suggestion prioritizes modifiers in add, while the existing text prioritizes modifiers in remove. Existing implementations in other languages that don't error on (?i-i:), such as C#/.NET, prioritize removal (i.e., set and then unset). While the consensus was to be more restrictive and issue an error, I'd still prefer we still maintain remove priority in the specification text for the sake of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would still prefer the "Else if", I would prefer to rewrite it as follows:

Suggested change
1. If _add_ contains *"i"*, set _ignoreCase_ to *true*.
1. If _add_ contains *"m"*, set _multiline_ to *true*.
1. If _add_ contains *"s"*, set _dotAll_ to *true*.
1. If _remove_ contains *"i"*, set _ignoreCase_ to *false*.
1. If _remove_ contains *"m"*, set _multiline_ to *false*.
1. If _remove_ contains *"s"*, set _dotAll_ to *false*.
1. If _remove_ contains *"i"*, set _ignoreCase_ to *false*.
1. Else if _add_ contains *"i"*, set _ignoreCase_ to *true*.
1. If _remove_ contains *"m"*, set _multiline_ to *false*.
1. Else if _add_ contains *"m"*, set _multiline_ to *true*.
1. If _remove_ contains *"s"*, set _dotAll_ to *false*.
1. Else if _add_ contains *"s"*, set _dotAll_ to *true*.

Such that remove continues to take precedence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine too.

<dl class="header">
</dl>
<emu-alg>
1. Let _ignoreCase_ be _rer_.[[IgnoreCase]].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let _ignoreCase_ be _rer_.[[IgnoreCase]].
1. Assert: _add_ and _remove_ have no elements in common.
1. Let _ignoreCase_ be _rer_.[[IgnoreCase]].

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we use the RegularExpressionFlags production and restrict it with an early error instead of introducing a new, more restricted production?

@rbuckton
Copy link
Contributor Author

Is there a reason we use the RegularExpressionFlags production and restrict it with an early error instead of introducing a new, more restricted production?

Is there a reason not to? Modifiers are a strict subset of RegularExpressionFlags, and RegularExpressionFlags itself is not restricted in the grammar, only via semantics. If I were to create a RegularExpressionModifiers production, it would have the same definition as RegularExpressionFlags.

@michaelficarra
Copy link
Member

I believe RegularExpressionFlags is not restricted in the grammar because RegularExpressionLiteral needs to not change when we add new flags. This is due to the overlap with division. We don't have that same constraint with the modifiers grammar though, so it should be able to be done with grammar restrictions and not early errors.

@rbuckton
Copy link
Contributor Author

I believe RegularExpressionFlags is not restricted in the grammar because RegularExpressionLiteral needs to not change when we add new flags. This is due to the overlap with division. We don't have that same constraint with the modifiers grammar though, so it should be able to be done with grammar restrictions and not early errors.

How would you propose it be written so as not to require early errors? Modifiers can appear in any order, cannot be duplicated within one or both of the modifier locations, and I do plan to extend the set of allowed modifiers over time with things like x-mode, so it needs to be fairly flexible. Even if we limit it to a subset of characters like i, m, and s for now, we either need an early error for duplicates, or a complex grammar like:

RegularExpressionModifiers:
  RegularExpressionModifierChars[+IgnoreCase, +Multiline, +DotAll]

RegularExpressionModifierChars[IgnoreCase, Multiline, DotAll]:
  [empty]
  [+IgnoreCase] `i` RegularExpressionModifierChars[~IgnoreCase, ?Multiline, ?DotAll]?
  [+Multiline] `m` RegularExpressionModifierChars[?IgnoreCase, ~Multiline, ?DotAll]?
  [+DotAll] `s` RegularExpressionModifierChars[?IgnoreCase, ?Multiline, ~DotAll]?

The more modifiers we add, the more production parameters we need, and the more complex the production becomes. Plus, this doesn't avoid the need for an early error to forbid (?i-i:).

@michaelficarra
Copy link
Member

@rbuckton I'm not saying we can't use any early errors at all on the production, just that we don't need to use early errors to enforce the allowed flag characters when an alternative grammar could do the job. I'm happy to continue using early errors to check for duplicates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants