Add support for RegExp lookbehind assertions - #7109
Conversation
msaboff
added
the
JavaScriptCore
|
EWS run on previous version of this PR (hash 21c3d75) Details |
msaboff
added
merging-blocked
|
EWS run on previous version of this PR (hash e633ee1) Details |
There was a problem hiding this comment.
Let's use dataLogLnIf for new change.
There was a problem hiding this comment.
Let's use dataLogLnIf for new change.
There was a problem hiding this comment.
Because this function is called only from 4 places, I rather would like to see that these parameters are always specified instead of default parameters to improve readability.
There was a problem hiding this comment.
Why not just place this brace just after : to make this normal case clause?
There was a problem hiding this comment.
Let's make sure that this is uint8_t size with : uint8_t.
There was a problem hiding this comment.
I think this reference is wrong because we are adding a new m_terms before using this variableTerm. If Vector is expanded, this reference should point to the wrong memory.
There was a problem hiding this comment.
Refactored to avoid modifying variableTerm after the append.
There was a problem hiding this comment.
Don't we need to mark this before if (subpatternId > m_pattern.m_numSubpatterns) { check?
If we need this flag only when we actually emit backreference, so how about moving this to L710?
There was a problem hiding this comment.
Let's use !isEmpty() since it looks easier to read.
There was a problem hiding this comment.
Should we put m_pattern.m_containsBackreferences = true; here instead?
There was a problem hiding this comment.
I don't think so. This could still be a forward reference that doesn't get converted. The m_pattern.m_containsBackreferences = true around L663 takes care of the forward references that get converted.
There was a problem hiding this comment.
Took care of this set and the next two.
There was a problem hiding this comment.
That change would require refactoring const into all the match* and backtrack* functions as well as the accessors for ByteTerm.
We could make that change later.
There was a problem hiding this comment.
OK, so let's make it auto& for now.
There was a problem hiding this comment.
Use dataLogLnIf for verbose case.
There was a problem hiding this comment.
The verbose case extends to the next if in both macros.
e633ee1 to
348e940
Compare
|
EWS run on previous version of this PR (hash 348e940) Details |
There was a problem hiding this comment.
Let's use char16_t instead.
There was a problem hiding this comment.
We don't have a printStream method for char16_t, only wchar_t. I did add another 0 to the comparison value on the line above so that we can print all BMP characters.
There was a problem hiding this comment.
Changed the printInternal() method and this use to be char16_t.
There was a problem hiding this comment.
Let's define char16_t thing.
There was a problem hiding this comment.
Why do we need to have this check again while L535 is doing the same check, and reread does not change input position?
There was a problem hiding this comment.
I don't think we need it. Removed.
There was a problem hiding this comment.
We should rename checkInput to tryCheckInput since this is doing a check as the same way to tryUncheckInput.
There was a problem hiding this comment.
Also, let's change the organization of code to make Forward/Backward case clear.
if (term.matchDirection() == Forward)
input.uncheckInput(U16_LENGTH(term.atom.patternCharacter));
else {
if (!input.tryCheckInput(U16_LENGTH(term.atom.patternCharacter)))
break;
}
There was a problem hiding this comment.
I restructured the if else as suggested.
checkInput is a preexisting construct. There is a Byteterm by the same name. It always had a can fail semantic. That refactoring would confuse my historic understanding of the code. That refactoring should also be done in YarrJIT.cpp as well. When we do the YarrJIT work, that may be a good to to refactor all the code.
There was a problem hiding this comment.
I think it is very confusing that checkInput can fail, uncheckInput cannot fail, tryUncheckInput can fail.
So, if you think we should not change checkInput name, then we should rename tryUncheckInput to uncheckInput, and rename uncheckInput to uncheckInputWithoutFailure etc.
There was a problem hiding this comment.
Ditto, let's a bit reorganize this code to make Forward/Backward clear.
if (term.matchDirection() == Forward)
input.uncheckInput(1);
else {
if (!input.tryCheckInput(1))
break;
}
There was a problem hiding this comment.
For matchAssertionBOL, etc. don't we need to use tryReadBackward to check term.inputPosition is valid?
There was a problem hiding this comment.
The last major refactoring I did was to align the offsets for forward and backward matching. This allowed some of the code to not need changes like matchAssertionBOL here. I believe we don't need the check.
There was a problem hiding this comment.
Let's reorganize this code to make Forward/Backward clear.
if (term.matchDirection() == Forward)
input.uncheckInput(U16_LENGTH(term.atom.patternCharacter));
else {
if (!input.tryCheckInput(U16_LENGTH(term.atom.patternCharacter)))
break;
}
There was a problem hiding this comment.
return true; here to make this code aligned to the other code.
There was a problem hiding this comment.
And remove this one-level nest.
348e940 to
52844d0
Compare
|
EWS run on current version of this PR (hash 52844d0) Details |
https://bugs.webkit.org/show_bug.cgi?id=174931 rdar://33183185 This change implements RegExp lookbehind in the Yarr interpreter. This change introduces the notion of match direction, either forward or backward. The forward match direction is the way the current code works, matching disjunciton terms and the subject string in a right to left manner. Lookbehind assertions, as defined in the EcmaScript spec, process disjunctions terms right to left matching the correspondding subject string right to left as well. Except for the Yarr JIT, almost all of the Yarr code has been touched to account for this backward matching. An additional Byteterm has been added, HaveCheckedInput, which checks that there is at least as many characters available in the input stream, but it doesn't move the input stream position. This is basically a CheckInput, without moving the input position. For variable counted terms, we still need to check that we won't try to access characters beyond the first character of the subject string. For functions like readSurrogatePairChecked(), we check for input before calling the funcion. For new input functions with a try prefix like tryReadBackward, the function itselfs checks for available input. After these checks prove that it is safe to access an offset to the left of the current input position, the actual matching can be performed. The Yarr parser, parses regular expression in left to right order. It also computes character offest in forward order. When we Byteterm compile, we process backward matching disjunctions right to left. The parser also has special handling of forward references within a backward matching parenthetical group. All such forward references are saved for that parenthetical group and are processed at the end of the group. Every one of these forward reference are check to see if a capture to the right of the forward reference was found, if so the forward reference is converted to a back reference. As part of this work, the ByteTerm dumping code was significantly updated to allow for not only dumping of the ByteCode after it has been generated, but to dump ByteCode while it is being interpreted. This ByteTerm dumping while interpreting is enabled with the Interpreter::verbose compile time constant. Reviewed by Yusuke Suzuki. * JSTests/stress/regexp-lookbehind.js: New tests. (arrayToString): (dumpValue): (compareArray): (testRegExp): * JSTests/test262/config.yaml: * Source/JavaScriptCore/runtime/RegExp.cpp: (JSC::RegExp::compile): (JSC::RegExp::compileMatchOnly): * Source/JavaScriptCore/yarr/YarrInterpreter.cpp: (JSC::Yarr::ByteTermDumper::ByteTermDumper): (JSC::Yarr::ByteTermDumper::unicode): (JSC::Yarr::Interpreter::InputStream::readForCharacterDump): (JSC::Yarr::Interpreter::InputStream::tryReadBackward): (JSC::Yarr::Interpreter::InputStream::tryUncheckInput): (JSC::Yarr::Interpreter::InputStream::isValidNegativeInputOffset): (JSC::Yarr::Interpreter::InputStream::dump const): (JSC::Yarr::Interpreter::checkCharacter): (JSC::Yarr::Interpreter::checkSurrogatePair): (JSC::Yarr::Interpreter::checkCasedCharacter): (JSC::Yarr::Interpreter::checkCharacterClass): (JSC::Yarr::Interpreter::checkCharacterClassDontAdvanceInputForNonBMP): (JSC::Yarr::Interpreter::tryConsumeBackReference): (JSC::Yarr::Interpreter::matchAssertionWordBoundary): (JSC::Yarr::Interpreter::backtrackPatternCharacter): (JSC::Yarr::Interpreter::backtrackPatternCasedCharacter): (JSC::Yarr::Interpreter::matchCharacterClass): (JSC::Yarr::Interpreter::backtrackCharacterClass): (JSC::Yarr::Interpreter::matchBackReference): (JSC::Yarr::Interpreter::backtrackBackReference): (JSC::Yarr::Interpreter::recordParenthesesMatch): (JSC::Yarr::Interpreter::matchParenthesesOnceBegin): (JSC::Yarr::Interpreter::matchParenthesesOnceEnd): (JSC::Yarr::Interpreter::backtrackParenthesesOnceEnd): (JSC::Yarr::Interpreter::matchParentheticalAssertionBegin): (JSC::Yarr::Interpreter::backtrackParentheticalAssertionBegin): (JSC::Yarr::Interpreter::matchDisjunction): (JSC::Yarr::ByteCompiler::compile): (JSC::Yarr::ByteCompiler::haveCheckedInput): (JSC::Yarr::ByteCompiler::assertionWordBoundary): (JSC::Yarr::ByteCompiler::atomPatternCharacter): (JSC::Yarr::ByteCompiler::atomCharacterClass): (JSC::Yarr::ByteCompiler::atomBackReference): (JSC::Yarr::ByteCompiler::atomParenthesesOnceBegin): (JSC::Yarr::ByteCompiler::atomParenthesesTerminalBegin): (JSC::Yarr::ByteCompiler::atomParenthesesSubpatternBegin): (JSC::Yarr::ByteCompiler::atomParentheticalAssertionBegin): (JSC::Yarr::ByteCompiler::atomParentheticalAssertionEnd): (JSC::Yarr::ByteCompiler::atomParenthesesSubpatternEnd): (JSC::Yarr::ByteCompiler::atomParenthesesOnceEnd): (JSC::Yarr::ByteCompiler::atomParenthesesTerminalEnd): (JSC::Yarr::ByteCompiler::emitDisjunction): (JSC::Yarr::ByteCompiler::isSafeToRecurse): (JSC::Yarr::ByteTermDumper::dumpTerm): (JSC::Yarr::ByteTermDumper::dumpDisjunction): (JSC::Yarr::Interpreter::InputStream::readPair): Deleted. (JSC::Yarr::ByteCompiler::dumpDisjunction): Deleted. * Source/JavaScriptCore/yarr/YarrInterpreter.h: (JSC::Yarr::ByteTerm::ByteTerm): (JSC::Yarr::ByteTerm::HaveCheckedInput): (JSC::Yarr::ByteTerm::WordBoundary): (JSC::Yarr::ByteTerm::BackReference): (JSC::Yarr::ByteTerm::isCharacterType): (JSC::Yarr::ByteTerm::isCasedCharacterType): (JSC::Yarr::ByteTerm::isCharacterClass): (JSC::Yarr::ByteTerm::matchDirection): * Source/JavaScriptCore/yarr/YarrJIT.cpp: (JSC::Yarr::dumpCompileFailure): * Source/JavaScriptCore/yarr/YarrJIT.h: * Source/JavaScriptCore/yarr/YarrParser.h: (JSC::Yarr::Parser::parseParenthesesBegin): * Source/JavaScriptCore/yarr/YarrPattern.cpp: (JSC::Yarr::YarrPatternConstructor::resetForReparsing): (JSC::Yarr::YarrPatternConstructor::assertionBOL): (JSC::Yarr::YarrPatternConstructor::atomPatternCharacter): (JSC::Yarr::YarrPatternConstructor::atomBuiltInCharacterClass): (JSC::Yarr::YarrPatternConstructor::atomParenthesesSubpatternBegin): (JSC::Yarr::YarrPatternConstructor::atomParentheticalAssertionBegin): (JSC::Yarr::YarrPatternConstructor::atomParenthesesEnd): (JSC::Yarr::YarrPatternConstructor::atomBackReference): (JSC::Yarr::YarrPatternConstructor::copyDisjunction): (JSC::Yarr::YarrPatternConstructor::quantifyAtom): (JSC::Yarr::YarrPatternConstructor::disjunction): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::SavedContext::SavedContext): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::SavedContext::restore): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::ParenthesisContext): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::push): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::pop): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::setInvert): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::invert const): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::setMatchDirection): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::matchDirection const): (JSC::Yarr::YarrPatternConstructor::ParenthesisContext::reset): (JSC::Yarr::YarrPatternConstructor::pushParenthesisContext): (JSC::Yarr::YarrPatternConstructor::popParenthesisContext): (JSC::Yarr::YarrPatternConstructor::setParenthesisInvert): (JSC::Yarr::YarrPatternConstructor::parenthesisInvert const): (JSC::Yarr::YarrPatternConstructor::setParenthesisMatchDirection): (JSC::Yarr::YarrPatternConstructor::parenthesisMatchDirection const): (JSC::Yarr::YarrPattern::YarrPattern): (JSC::Yarr::dumpCharacterClass): (JSC::Yarr::PatternTerm::dump): * Source/JavaScriptCore/yarr/YarrPattern.h: (JSC::Yarr::PatternTerm::PatternTerm): (JSC::Yarr::PatternTerm::convertToBackreference): (JSC::Yarr::PatternTerm::setMatchDirection): (JSC::Yarr::PatternTerm::matchDirection const): (JSC::Yarr::PatternAlternative::PatternAlternative): (JSC::Yarr::PatternAlternative::matchDirection const): (JSC::Yarr::PatternDisjunction::addNewAlternative): (JSC::Yarr::YarrPattern::resetForReparsing): * Source/JavaScriptCore/yarr/YarrSyntaxChecker.cpp: (JSC::Yarr::SyntaxChecker::atomParentheticalAssertionBegin): * Source/WTF/wtf/PrintStream.cpp: (WTF::printInternal): * Source/WTF/wtf/PrintStream.h: * Source/WebCore/contentextensions/URLFilterParser.cpp: (WebCore::ContentExtensions::PatternParser::atomParentheticalAssertionBegin): Canonical link: https://commits.webkit.org/257823@main
52844d0 to
46e6b3f
Compare
|
Committed 257823@main (46e6b3f): https://commits.webkit.org/257823@main Reviewed commits have been landed. Closing PR #7109 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
|
When will this be available in the defealt builds of WebKit on MacOS devices? Also, Is this part of Safari Technology preview 161? (sorry if this PR isnt right place to ask this, would appreciate a redirect) |
|
@MoeBazziGIT It is part of Safari Technology preview 161 but originally it wasn't mentioned in the release notes, now it is, top of the Javascript section:
|
6 participants
46e6b3f
52844d0
🧪 ios-wk2🧪 api-ios🧪 mac-wk1