Bug 206988 – Quantifiers after lookahead assertions should be syntax errors in Unicode patterns only

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

Test case:
  /(?!.)+/u

Expected:
  SyntaxError thrown

Actual:
  RegExp instance

ECMA262:
  https://tc39.es/ecma262/#prod-annexB-Term (/u flag precludes the use of QuantifiableAssertion)

  Term[U, N] ::
    [~U] QuantifiableAssertion[?N] Quantifier

  Assertion[U, N] ::
    [~U] QuantifiableAssertion[?N]

  QuantifiableAssertion[N] ::
    (?= Disjunction[~U, ?N])
    (?! Disjunction[~U, ?N])

Test262:
  https://test262.report/browse/built-ins/RegExp/unicode_restricted_quantifiable_assertion.js
  https://test262.report/browse/language/literals/regexp/u-invalid-range-lookahead.js
  https://test262.report/browse/language/literals/regexp/u-invalid-optional-negative-lookahead.js

[Alexey Shvayka]

Created attachment 389236 [details]
Patch

[Alexey Shvayka]

Created attachment 389312 [details]
Patch

Rename bug in ChangeLog.

[Darin Adler]

Comment on attachment 389312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389312&action=review

> Source/JavaScriptCore/yarr/YarrParser.h:709
> +        auto parensType = m_parenthesesStack.takeLast();
> +        return parensType == ParenthesesType::Subpattern || !m_isUnicode;

In WebKit coding style we normally don’t abbreviate a word like parentheses even in a local variable name. I think calling this just "type" is fine.

> Source/JavaScriptCore/yarr/YarrParser.h:1124
> +    enum ParenthesesType { Subpattern, Assertion };

Might get a slightly more efficient code if this was an enum class based on bool or uint8_t.

> Source/JavaScriptCore/yarr/YarrParser.h:1133
> +    Vector<ParenthesesType, 16> m_parenthesesStack;

Thisis likely going to be a bit inefficient; we could possibly get a more efficient implementation of a bit vector.

[Darin Adler]

I didn’t set the commit queue flag yet because I think the change to enum class might be worth doing before landing. If not, add a comment some committer can add commit-queue+.

[Alexey Shvayka]

Created attachment 389448 [details]
Patch

Set reviewer, rename vars, and use std::vector with typed enum class.

[Alexey Shvayka]

(In reply to Darin Adler from comment #3)
>
> Thisis likely going to be a bit inefficient; we could possibly get a more
> efficient implementation of a bit vector.

I did some isolated profiling of std::vector<bool>: it allocates 8x less memory, but it performs ~30% slower on push_back() + pop_back() and is not compatible with `enum class`.

[Ross Kirsling]

Comment on attachment 389448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389448&action=review

> Source/JavaScriptCore/yarr/YarrParser.h:1141
> +    std::vector<ParenthesesType> m_parenthesesStack;

Regardless of the enum class type, we should still use a WTF::Vector.

[Alexey Shvayka]

Created attachment 389671 [details]
Patch

Replace std::vector with WTF::Vector.

[WebKit Commit Bot]

Comment on attachment 389671 [details]
Patch

Clearing flags on attachment: 389671

Committed r255689: <https://trac.webkit.org/changeset/255689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59157737>