Convert PowerShell escaping to support linear regexp engine#2436
Convert PowerShell escaping to support linear regexp engine#2436ericcornelissen merged 17 commits intomainfrom
Conversation
Straightforward translation from RegExp to lRegExp compatible regular expression using existing techniques (either one-to-one or replacing lookbehind by capturing groups that are put back into the result when replacing).
ericcornelissen
added
the
refactor
Expand test fixtures for PowerShell escaping based on mutation testing results. This mostly identified gaps in the testing where the use of the "g" flag wasn't covered by tests due to a lack of repeated characters to escape in a string. Additionally, based on mutation testing too, update some of the regular expressions. Expressions anchored to the start or end of the string don't need to have the "g" flag and the `backslashSuffix` regex can't possibly match the start of the string because it requires at least a non-whitespace character at the start and a whitespace character after that (because of the guard in which this expression is used).
ericcornelissen
changed the title
lRegExpAs identified by differential testing (see [1]), this fixes escaping of double quoting. In particular for cases like `"\\"` in which the first `"` prevented the regular expression from matching the second `"` due to the first capturing group in `backslashQuote`. This is fixed using the marker technique introduced in [2]. To prevent regressions various unit test fixtures for this and similar cases were added. [1]: cc1076d [2]: 0f38a42
ericcornelissen
force-pushed
the
lregexp-powershell
branch
from
ec2e2f7 to
b52fac8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/fixtures/win.js (1)
2991-3024:⚠️ Potential issue | 🟡 MinorCover the whitespace + backslashes +
"intersection too.These additions pin whitespace+quotes and backslashes+quotes separately, but
src/internal/win/powershell.jsLines 39-47 and Lines 76-82 change behavior specifically when all three appear in the same argument. That leaves the marker-based branch only partially covered.Also applies to: 5297-5330
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 757ae6b8-6e04-4704-bb5a-8a2ccfd7d48b
📒 Files selected for processing (3)
config/eslint.jssrc/internal/win/powershell.jstest/fixtures/win.js
1 participant
Relates to #2122