This Test262 test is failing - JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js Exception: SyntaxError: Invalid regular expression: invalid backreference for unicode pattern at JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js:33 The line in question is: assert(compareArray(["bab", "b"], "bab".match(/\k<a>(?<a>b)\w\k<a>/u))); Here the named group “a” is being referenced before if is defined. Seems like we’re doing the right thing, but we need to check.
<rdar://problem/34844068>
Created attachment 394228 [details] Patch
I will add ChangeLog entry in a minute; just making sure we are not tackling the same issue separately.
Created attachment 394256 [details] Patch Add ChangeLog and fix build.
Created attachment 394257 [details] Patch Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.
(In reply to Alexey Shvayka from comment #5) > Created attachment 394257 [details] > Patch > > Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT. I think we intentionally avoid this? I think for a release assert you may need to use NO_RETURN_DUE_TO_CRASH.
> > Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT. > > I think we intentionally avoid this? I think for a release assert you may > need to use NO_RETURN_DUE_TO_CRASH. Thank you, there are indeed no usages of [[noreturn]] in WebKit code base. API tests fail because named and numeric references should be disabled for URLFilterParser. I might have need to bring back isValidNamedForwardReference().
Created attachment 394400 [details] Patch Use NO_RETURN_DUE_TO_CRASH, fix API tests, improve error messages, and tweak naming.
Comment on attachment 394400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394400&action=review Nice work! (Looks like you need a rebase though.) > Source/JavaScriptCore/yarr/YarrErrorCode.h:55 > + InvalidNamedBackreference, Interesting how it's `Backreference` here and `BackReference` elsewhere...though this isn't your fault. :P
Created attachment 394553 [details] Patch Rebase patch, set reviewer, add second m_kIdentityEscapeSeen, check and add a comment explaining m_isUnicode check in handleIllegalReferences().
Created attachment 394555 [details] Patch Camelcase "backreference".
Thank you for taking time to review this patch, Ross. It is quite tricky.
Comment on attachment 394555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394555&action=review > Source/JavaScriptCore/yarr/YarrParser.h:919 > + if (m_kIdentityEscapeSeen && !m_captureGroupNames.isEmpty()) { > + m_errorCode = ErrorCode::InvalidNamedBackReference; > + return; > + } Do we need a test for this path, if nothing was failing without it?
(In reply to Ross Kirsling from comment #13) > Do we need a test for this path, if nothing was failing without it? We have this covered by test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js I just didn't run the tests properly.
(In reply to Alexey Shvayka from comment #14) > (In reply to Ross Kirsling from comment #13) > > Do we need a test for this path, if nothing was failing without it? > > We have this covered by > test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js > > test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js > > test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js > > I just didn't run the tests properly. Oh okay, thanks for confirming.
Committed r259026: <https://trac.webkit.org/changeset/259026> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394555 [details].