Improved no-useless-non-capturing-group rule (#154)

* Imporved `no-useless-non-capturing-group` rule

* Improved docs

Author: RunDevelopment

docs/rules/no-useless-non-capturing-group.md

@@ -13,7 +13,7 @@ since: "v0.4.0"
 
 ## :book: Rule Details
 
-This rule reports unnecessary Non-capturing group
+This rule reports unnecessary non-capturing group
 
 <eslint-code-block fix>
 
@@ -22,18 +22,50 @@ This rule reports unnecessary Non-capturing group
 
 /* ✓ GOOD */
 var foo = /(?:abcd)?/
-var foo = /(?:ab|cd)/
+var foo = /a(?:ab|cd)/
 
 /* ✗ BAD */
+var foo = /(?:ab|cd)/
 var foo = /(?:abcd)/
 var foo = /(?:[a-d])/
+var foo = /(?:[a-d])|e/
+var foo = /(?:a|(?:b|c)|d)/
 ```
 
 </eslint-code-block>
 
 ## :wrench: Options
 
-Nothing.
+```json5
+{
+  "regexp/no-useless-non-capturing-group": ["error", {
+    "allowTop": true
+  }]
+}
+```
+
+- `"allowTop"`:
+  Whether a top-level non-capturing group is allowed. Defaults to `false`.
+
+  Sometimes it's useful to wrap a whole pattern into a non-capturing group (e.g. when the pattern is used as a building block to construct more complex patterns). Use this option to allow top-level non-capturing groups.
+
+<eslint-code-block fix>
+
+```js
+/* eslint regexp/no-useless-non-capturing-group: ["error", { allowTop: true }] */
+
+/* ✓ GOOD */
+var foo = /(?:abcd)/
+var foo = /(?:ab|cd)/
+var foo = /(?:abcd)/
+var foo = /(?:[a-d])/
+
+/* ✗ BAD */
+var foo = /(?:[a-d])|e/
+var foo = /(?:a|(?:b|c)|d)/
+```
+
+</eslint-code-block>
 
 ## :rocket: Version
 

lib/rules/no-useless-non-capturing-group.ts

@@ -1,7 +1,27 @@
+import type { Group } from "regexpp/ast"
 import type { RegExpVisitor } from "regexpp/visitor"
 import type { RegExpContext } from "../utils"
 import { canUnwrapped, createRule, defineRegexpVisitor } from "../utils"
 
+/**
+ * Returns whether the given group is the top-level group of its pattern.
+ *
+ * A pattern with a top-level groups is of the form `/(?:...)/flags`.
+ */
+function isTopLevel(group: Group): boolean {
+    const parent = group.parent
+    if (parent.type === "Alternative" && parent.elements.length === 1) {
+        const parentParent = parent.parent
+        if (
+            parentParent.type === "Pattern" &&
+            parentParent.alternatives.length === 1
+        ) {
+            return true
+        }
+    }
+    return false
+}
+
 export default createRule("no-useless-non-capturing-group", {
     meta: {
         docs: {
@@ -11,62 +31,73 @@ export default createRule("no-useless-non-capturing-group", {
             recommended: false,
         },
         fixable: "code",
-        schema: [],
+        schema: [
+            {
+                type: "object",
+                properties: {
+                    allowTop: { type: "boolean" },
+                },
+                additionalProperties: false,
+            },
+        ],
         messages: {
             unexpected: "Unexpected quantifier Non-capturing group.",
         },
         type: "suggestion", // "problem",
     },
     create(context) {
+        const allowTop = context.options[0]?.allowTop ?? false
+
         /**
          * Create visitor
          */
         function createVisitor({
             node,
             getRegexpLocation,
-            getRegexpRange,
+            fixReplaceNode,
         }: RegExpContext): RegExpVisitor.Handlers {
             return {
                 onGroupEnter(gNode) {
-                    if (gNode.alternatives.length !== 1) {
-                        // Useful when using disjunctions.
-                        // e.g. /(?:a|b)/
-                        return
-                    }
-                    const alt = gNode.alternatives[0]
-                    if (alt.elements.length === 0) {
-                        // Ignore empty groups. You can check with another rule.
-                        // e.g. /(?:)/
-                        return
-                    }
-                    const parent = gNode.parent
-                    if (
-                        parent.type === "Quantifier" &&
-                        (alt.elements.length > 1 ||
-                            alt.elements[0].type === "Quantifier")
-                    ) {
-                        // e.g. /(?:ab)?/
+                    if (allowTop && isTopLevel(gNode)) {
                         return
                     }
-                    if (!canUnwrapped(gNode, alt.raw)) {
-                        return
+
+                    if (gNode.alternatives.length === 1) {
+                        const alt = gNode.alternatives[0]
+                        if (alt.elements.length === 0) {
+                            // Ignore empty groups. You can check with another rule.
+                            // e.g. /(?:)/
+                            return
+                        }
+                        const parent = gNode.parent
+                        if (
+                            parent.type === "Quantifier" &&
+                            (alt.elements.length > 1 ||
+                                alt.elements[0].type === "Quantifier")
+                        ) {
+                            // e.g. /(?:ab)?/
+                            return
+                        }
+                        if (!canUnwrapped(gNode, alt.raw)) {
+                            return
+                        }
+                    } else {
+                        // the group might still be useless
+                        // e.g. /a(?:b|(?:c|d)|e)/
+                        const parent = gNode.parent
+                        if (parent.type !== "Alternative") {
+                            return
+                        }
+                        if (parent.elements.length !== 1) {
+                            return
+                        }
                     }
 
                     context.report({
                         node,
                         loc: getRegexpLocation(gNode),
                         messageId: "unexpected",
-                        fix(fixer) {
-                            const groupRange = getRegexpRange(gNode)
-                            const altRange = getRegexpRange(alt)
-                            if (!groupRange || !altRange) {
-                                return null
-                            }
-                            return [
-                                fixer.removeRange([groupRange[0], altRange[0]]),
-                                fixer.removeRange([altRange[1], groupRange[1]]),
-                            ]
-                        },
+                        fix: fixReplaceNode(gNode, gNode.raw.slice(3, -1)),
                     })
                 },
             }

lib/utils/index.ts

@@ -1,7 +1,7 @@
 import type * as ESTree from "estree"
 import type { RuleListener, RuleModule, PartialRuleModule } from "../types"
 import type { RegExpVisitor } from "regexpp/visitor"
-import type { Alternative, Element, Node, Quantifier } from "regexpp/ast"
+import type { Element, Node, Quantifier } from "regexpp/ast"
 import { RegExpParser, visitRegExpAST } from "regexpp"
 import {
     CALL,
@@ -752,90 +752,111 @@ export function quantToString(quant: Readonly<Quant>): string {
 
 /* eslint-disable complexity -- X( */
 /**
- * Check the siblings to see if the regex doesn't change when unwrapped.
+ * Returns whether the concatenation of the two string might create new escape
+ * sequences or elements.
  */
-export function canUnwrapped(
+function mightCreateNewElement(
     /* eslint-enable complexity -- X( */
-    node: Element,
-    text: string,
+    before: string,
+    after: string,
 ): boolean {
-    const parent = node.parent
-    let target: Element, alternative: Alternative
-    if (parent.type === "Quantifier") {
-        alternative = parent.parent
-        target = parent
-    } else if (parent.type === "Alternative") {
-        alternative = parent
-        target = node
-    } else {
+    // control
+    // \cA
+    if (before.endsWith("\\c") && /^[a-z]/i.test(after)) {
         return true
     }
-    const index = alternative.elements.indexOf(target)
-    if (index === 0) {
+
+    // hexadecimal
+    // \xFF \uFFFF
+    if (
+        /(?:^|[^\\])(?:\\{2})*\\(?:x[\dA-Fa-f]?|u[\dA-Fa-f]{0,3})$/.test(
+            before,
+        ) &&
+        /^[\da-f]/i.test(after)
+    ) {
         return true
     }
-    if (/^\d+$/u.test(text)) {
-        let prevIndex = index - 1
-        let prev = alternative.elements[prevIndex]
-        if (prev.type === "Backreference") {
-            // e.g. /()\1[0]/ -> /()\10/
-            return false
-        }
 
-        while (
-            prev.type === "Character" &&
-            /^\d+$/u.test(prev.raw) &&
-            prevIndex > 0
-        ) {
-            prevIndex--
-            prev = alternative.elements[prevIndex]
-        }
-        if (prev.type === "Character" && prev.raw === "{") {
-            // e.g. /a{[0]}/ -> /a{0}/
-            return false
-        }
+    // unicode
+    // \u{FFFF}
+    if (
+        (/(?:^|[^\\])(?:\\{2})*\\u$/.test(before) &&
+            /^\{[\da-f]*(?:\}[\s\S]*)?$/i.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\\u\{[\da-f]*$/.test(before) &&
+            /^(?:[\da-f]+\}?|\})/i.test(after))
+    ) {
+        return true
     }
-    if (/^[0-7]+$/u.test(text)) {
-        const prev = alternative.elements[index - 1]
-        if (prev.type === "Character" && /^\\[0-7]+$/u.test(prev.raw)) {
-            // e.g. /\0[1]/ -> /\01/
-            return false
-        }
+
+    // octal
+    // \077 \123
+    if (
+        (/(?:^|[^\\])(?:\\{2})*\\0[0-7]?$/.test(before) &&
+            /^[0-7]/.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\\[1-7]$/.test(before) && /^[0-7]/.test(after))
+    ) {
+        return true
     }
-    if (/^[\da-f]+$/iu.test(text)) {
-        let prevIndex = index - 1
-        let prev = alternative.elements[prevIndex]
-        while (
-            prev.type === "Character" &&
-            /^[\da-f]+$/iu.test(prev.raw) &&
-            prevIndex > 0
-        ) {
-            prevIndex--
-            prev = alternative.elements[prevIndex]
-        }
-        if (
-            prev.type === "Character" &&
-            (prev.raw === "\\x" || prev.raw === "\\u")
-        ) {
-            // e.g. /\xF[F]/ -> /\xFF/
-            // e.g. /\uF[F]FF/ -> /\xFFFF/
-            return false
-        }
+
+    // backreference
+    // \12 \k<foo>
+    if (
+        (/(?:^|[^\\])(?:\\{2})*\\[1-9]\d*$/.test(before) &&
+            /^\d/.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\\k$/.test(before) && after.startsWith("<")) ||
+        /(?:^|[^\\])(?:\\{2})*\\k<[^<>]*$/.test(before)
+    ) {
+        return true
     }
-    if (/^[a-z]+$/iu.test(text)) {
-        if (index > 1) {
-            const prev = alternative.elements[index - 1]
-            if (prev.type === "Character" && prev.raw === "c") {
-                const prev2 = alternative.elements[index - 2]
-                if (prev2.type === "Character" && prev2.raw === "\\") {
-                    // e.g. /\c[M]/ -> /\cM/
-                    return false
-                }
-            }
-        }
+
+    // property
+    // \p{L} \P{L}
+    if (
+        (/(?:^|[^\\])(?:\\{2})*\\p$/i.test(before) &&
+            /^\{[\w=]*(?:\}[\s\S]*)?$/.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\\p\{[\w=]*$/i.test(before) &&
+            /^[\w=]+(?:\}[\s\S]*)?$|^\}/.test(after))
+    ) {
+        return true
     }
 
-    return true
+    // quantifier
+    // {1} {2,} {2,3}
+    if (
+        (/(?:^|[^\\])(?:\\{2})*\{\d*$/.test(before) && /^[\d,}]/.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\{\d+,$/.test(before) &&
+            /^(?:\d+(?:\}|$)|\})/.test(after)) ||
+        (/(?:^|[^\\])(?:\\{2})*\{\d+,\d*$/.test(before) &&
+            after.startsWith("}"))
+    ) {
+        return true
+    }
+
+    return false
+}
+
+/**
+ * Check the siblings to see if the regex doesn't change when unwrapped.
+ */
+export function canUnwrapped(node: Element, text: string): boolean {
+    let textBefore, textAfter
+
+    const parent = node.parent
+    if (parent.type === "Alternative") {
+        textBefore = parent.raw.slice(0, node.start - parent.start)
+        textAfter = parent.raw.slice(node.end - parent.start)
+    } else if (parent.type === "Quantifier") {
+        const alt = parent.parent
+        textBefore = alt.raw.slice(0, node.start - alt.start)
+        textAfter = alt.raw.slice(node.end - alt.start)
+    } else {
+        return true
+    }
+
+    return (
+        !mightCreateNewElement(textBefore, text) &&
+        !mightCreateNewElement(text, textAfter)
+    )
 }
 
 /**