Normative: add RegExp lookbehind to annex-B #1102

mysticatea · 2018-02-16T05:32:25Z

This PR adds the syntax of RegExp Lookbehind Assertions into B.1.4 Regular Expressions Patterns section.

Since #1029 didn't look to update the Annex-B section but I think it's needed (#1029 (comment)).

I apologize if it's intentional.

mathiasbynens · 2018-02-16T08:15:54Z

cc @anba @jmdyck @claudepache @IgnoredAmbience @littledan

mathiasbynens · 2018-02-16T08:16:54Z

This indeed looks like an oversight to me.

littledan · 2018-02-16T09:35:59Z

spec.html

@@ -40918,7 +40922,7 @@ <h1>Pattern Semantics</h1>
          1. Return _m_.
        </emu-alg>

-        <p>Assertion (<emu-xref href="#sec-assertion"></emu-xref>) evaluation rules for the <emu-grammar>Assertion :: `(` `?` `=` Disjunction `)`</emu-grammar> and <emu-grammar>Assertion :: `(` `?` `!` Disjunction `)`</emu-grammar> productions are also used for the |QuantifiableAssertion| productions, but with |QuantifiableAssertion| substituted for |Assertion|.</p>
+        <p>Assertion (<emu-xref href="#sec-assertion"></emu-xref>) evaluation rules for the <emu-grammar>Assertion :: `(` `?` `=` Disjunction `)`</emu-grammar>, <emu-grammar>Assertion :: `(` `?` `!` Disjunction `)`</emu-grammar>, <emu-grammar>Assertion :: `(` `?` `&lt;=` Disjunction `)`</emu-grammar>, and <emu-grammar>Assertion :: `(` `?` `&lt;!` Disjunction `)`</emu-grammar> productions are also used for the |QuantifiableAssertion| productions, but with |QuantifiableAssertion| substituted for |Assertion|.</p>


Do we actually want to enable lookbehind in QuantifiableAssertion?

We should make sure to have a test262 test one way or the other.

Just a note: I confirmed it on Chrome 65 since Annex B is the section for browser compatibility. Lookbehind assertions are quantifiable on the Chrome.

Good to know, though I think it's still within our ability to make this choice one way or another: I bet there would be no web compatibility issues with banning this particular construct.

cc @hashseed @schuay

claudepache · 2018-02-16T10:18:59Z

We have to make a conscious decision between:

(A) be consistent with other assertions of the form (?...) wrt QuantifiableAssertion; or,
(B) limit QuantifiableAssertion to what is strictly needed for legacy reason.

This PR apparently chose (A). I would prefer (B).

(Also, because a choice is needed, this PR is not ”editorial”.)

hashseed · 2018-02-16T14:03:14Z

So if I understand correctly, the question is whether to allow something like /(?<=a){2,3}b/.

I honestly don't see a reason why this should be allowed since these assertions do not consume any characters, and therefore seem rather pointless.

Btw. if we decide for (B), I will need to fix V8.

jmdyck · 2018-02-16T15:29:40Z

spec.html

          [~U] QuantifiableAssertion[?N]

        QuantifiableAssertion[N] ::
          `(` `?` `=` Disjunction[~U, ?N] `)`
          `(` `?` `!` Disjunction[~U, ?N] `)`
+          `(` `?` `&lt;=` Disjunction[?U, ?N] `)`
+          `(` `?` `&lt;!` Disjunction[?U, ?N] `)`


It's an error to use '?U' here, because QuantifiableAssertion doesn't have a 'U' parameter.

littledan · 2018-02-16T17:06:23Z

OK, seems like we're pretty unanimous in this thread so far that QuantifiableAssertion should not support positive or negative lookbehinds. @mysticatea would you be up for writing a new patch with that change, and then we can discuss it at the March 2018 TC39 meeting to confirm broader consensus?

mysticatea · 2018-02-17T01:34:52Z

I updated this PR.
I have kept Editorial: prefix since I see editorial change label.

hashseed · 2018-02-19T10:31:12Z

FWIW we have this comment in V8 implemented back in January 2016:

// With /u, lookarounds are not quantifiable.

I guess the change would be to

disallow lookbehinds for quantifiable assertions
disallow lookaheads for quantifiable assertions in unicode mode
allow lookahead for quantifiable assertions in non-unicode mode

littledan · 2018-02-19T11:02:49Z

@hashseed I think that's what the current patch does. The grammar here (EDIT: Namely, in Annex B section 1.4) only is relevant on non-Unicode RegExps. From the introduction to the section:

However, none of these extensions change the syntax of Unicode patterns recognized when parsing with the [U] parameter present on the goal symbol.

@bterlson and I were just puzzling over what those [?U] parameters were doing all over the Annex B grammar. Why should we ever be passing around a true parameter? Seems like some cases went for [~U] and others for [?U]. Anyway, a topic for another possible cleanup issue.

jmdyck · 2018-02-19T14:43:55Z

I think that's what the current patch does.

I think all three of @hashseed's bullets are accomplished in the current spec. This patch does something else.

hashseed · 2018-02-20T10:52:23Z

V8 fix in flight: https://chromium-review.googlesource.com/c/v8/v8/+/926102

littledan · 2018-02-20T11:29:32Z

@jmdyck How does the current spec support lookbehind at all when taking the Annex B grammar into account?

jmdyck · 2018-02-20T16:16:07Z

The current spec doesn't, this patch does. (Allowing lookbehinds wasn't one of @hashseed's bullets.)

littledan · 2018-02-20T21:41:59Z

@jmdyck I think we actually do want to enable lookbehinds, just not as a quantifiable assertion. Given that goal, do you think this patch seems good?

jmdyck · 2018-02-21T00:51:23Z

Yes, the patch is necessary. 21.2.1 has lookbehinds, so annex B must have them too.

- Tests that lookahead and lookbehind are not extended to QuantifiableAssertion, as in tc39/ecma262#1102 - Additional tests verifying some more combinations of cases for QuantifiableAssertion being invalid in Unicode mode. Based on the tests in https://chromium-review.googlesource.com/c/v8/v8/+/926102 These tests pass on V8 (if the throw for early errors is removed to work around a V8 issue where RegExps don't have early errors).

littledan · 2018-02-26T14:54:42Z

See tests at tc39/test262#1456

littledan · 2018-02-26T15:00:46Z

At this point, we have

Spec text, written by someone who I verified signed the IP form (thanks @mysticatea)
Test262 tests (thanks @leobalter for the review)
A full implementation in V8 (thanks @hashseed)

Do we have consensus on this normative change? I've added it to the March 2018 meeting agenda so we can confirm consensus there if necessary.

bterlson · 2018-05-14T20:46:25Z

Thanks for this PR, and sorry for the delay in getting it pulled in!

…rn evaluate semantics in annex b Bugfix: When speccing lookbehind assertions (PR tc39#1102), a `direction` parameter were added, in particular to the `evaluate` semantics of Atom productions and to the CharacterSetMatcher abstract operation. The ExtendedAtom productions found in Annex B were forgotten to be amended in the same way.

littledan reviewed Feb 16, 2018

View reviewed changes

mysticatea mentioned this pull request Feb 16, 2018

ES2018 RegExp enhancements acornjs/acorn#673

Closed

4 tasks

mysticatea changed the title ~~Editorial: add RegExp lookbehind to annex-B~~ Normative: add RegExp lookbehind to annex-B Feb 16, 2018

jmdyck reviewed Feb 16, 2018

View reviewed changes

ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. spec bug web reality editorial change labels Feb 16, 2018

mysticatea changed the title ~~Normative: add RegExp lookbehind to annex-B~~ Editorial: add RegExp lookbehind to annex-B Feb 17, 2018

mysticatea force-pushed the annex-b-regexp branch from ac7ccb8 to de64f38 Compare February 17, 2018 01:34

littledan added normative change Affects behavior required to correctly evaluate some ECMAScript source text and removed editorial change labels Feb 17, 2018

mysticatea added a commit to mysticatea/acorn that referenced this pull request Feb 17, 2018

follow tc39/ecma262#1102

2b229f0

mysticatea changed the title ~~Editorial: add RegExp lookbehind to annex-B~~ Normative: add RegExp lookbehind to annex-B Feb 17, 2018

Normative: add RegExp lookbehind assertions to annex B

c6941f3

mysticatea force-pushed the annex-b-regexp branch from de64f38 to c6941f3 Compare February 17, 2018 10:06

littledan added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Feb 20, 2018

littledan mentioned this pull request Feb 26, 2018

Additional tests for QuantifiableAssertion tc39/test262#1456

Merged

littledan added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Feb 26, 2018

littledan mentioned this pull request Feb 26, 2018

Reland "Additional tests for QuantifiableAssertion" tc39/test262#1459

Merged

ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Mar 20, 2018

ljharb added the needs-spec-draft-integration label May 10, 2018

bterlson merged commit e5b6605 into tc39:master May 14, 2018

mysticatea deleted the annex-b-regexp branch June 16, 2018 14:45

jmdyck mentioned this pull request Aug 21, 2019

bug? re lookbehind assertions in Annex B #1674

Closed

bakkot mentioned this pull request Aug 22, 2019

Normative: Make B.1 "Additional Syntax" normative #1651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: add RegExp lookbehind to annex-B #1102

Normative: add RegExp lookbehind to annex-B #1102

mysticatea commented Feb 16, 2018 •

edited

Loading

mathiasbynens commented Feb 16, 2018

mathiasbynens commented Feb 16, 2018

littledan Feb 16, 2018

littledan Feb 16, 2018

mysticatea Feb 16, 2018

littledan Feb 16, 2018 •

edited

Loading

claudepache commented Feb 16, 2018

hashseed commented Feb 16, 2018 •

edited

Loading

jmdyck Feb 16, 2018

littledan commented Feb 16, 2018

mysticatea commented Feb 17, 2018 •

edited

Loading

hashseed commented Feb 19, 2018

littledan commented Feb 19, 2018 •

edited

Loading

jmdyck commented Feb 19, 2018

hashseed commented Feb 20, 2018

littledan commented Feb 20, 2018

jmdyck commented Feb 20, 2018

littledan commented Feb 20, 2018

jmdyck commented Feb 21, 2018

littledan commented Feb 26, 2018

littledan commented Feb 26, 2018 •

edited

Loading

bterlson commented May 14, 2018

Normative: add RegExp lookbehind to annex-B #1102

Normative: add RegExp lookbehind to annex-B #1102

Conversation

mysticatea commented Feb 16, 2018 • edited Loading

mathiasbynens commented Feb 16, 2018

mathiasbynens commented Feb 16, 2018

littledan Feb 16, 2018

Choose a reason for hiding this comment

littledan Feb 16, 2018

Choose a reason for hiding this comment

mysticatea Feb 16, 2018

Choose a reason for hiding this comment

littledan Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

claudepache commented Feb 16, 2018

hashseed commented Feb 16, 2018 • edited Loading

jmdyck Feb 16, 2018

Choose a reason for hiding this comment

littledan commented Feb 16, 2018

mysticatea commented Feb 17, 2018 • edited Loading

hashseed commented Feb 19, 2018

littledan commented Feb 19, 2018 • edited Loading

jmdyck commented Feb 19, 2018

hashseed commented Feb 20, 2018

littledan commented Feb 20, 2018

jmdyck commented Feb 20, 2018

littledan commented Feb 20, 2018

jmdyck commented Feb 21, 2018

littledan commented Feb 26, 2018

littledan commented Feb 26, 2018 • edited Loading

bterlson commented May 14, 2018

mysticatea commented Feb 16, 2018 •

edited

Loading

littledan Feb 16, 2018 •

edited

Loading

hashseed commented Feb 16, 2018 •

edited

Loading

mysticatea commented Feb 17, 2018 •

edited

Loading

littledan commented Feb 19, 2018 •

edited

Loading

littledan commented Feb 26, 2018 •

edited

Loading