Repository: ota-meshi/eslint-plugin-regexp (ts)

Issue:
Add suggestions to `regexp/no-dupe-disjunctions`
**Description**
It might be useful to have suggestions (but not fixes) for `regexp/no-dupe-disjunctions`.

What do think @ota-meshi?

Pull request description:
Add suggestions for `regexp/no-dupe-disjunctions`
This fixes #405.

This is a branch of #404. I'll rebase this after #404 is merged.

---

Regarding the changes: It's all relatively simple except for the range replacement. 

I wanted suggestions for ranges (e.g. `a+|[a-z]` -> `a+|[b-z]`), but this wasn't easy. The main problems was: How do we print the new range? We have to print a new character (in the example above, we needed to print `b` to make `b-z`), but do we do that?

I decided to print them as hexadecimal escapes, except when the character is in [`allowedCharacterRanges`](https://ota-meshi.github.io/eslint-plugin-regexp/settings/#available-settings). This is a somewhat unintended use case for this setting, so I hope this won't case problems down the road.

Patch:
diff --git a/lib/rules/no-dupe-characters-character-class.ts b/lib/rules/no-dupe-characters-character-class.ts
index e7724e0..9bf527a 100644
--- a/lib/rules/no-dupe-characters-character-class.ts
+++ b/lib/rules/no-dupe-characters-character-class.ts
@@ -14,13 +14,12 @@ import {
     createRule,
     defineRegexpVisitor,
     toCharSetSource,
-    mightCreateNewElement,
+    fixRemoveCharacterClassElement,
 } from "../utils"
 import type { CharRange, CharSet } from "refa"
 import { JS } from "refa"
 import type { ReadonlyFlags } from "regexp-ast-analysis"
 import { toCharSet } from "regexp-ast-analysis"
-import type { Rule } from "eslint"
 import { mentionChar } from "../utils/mention"
 
 interface Grouping {
@@ -106,61 +105,6 @@ function inRange({ min, max }: CharRange, char: number): boolean {
     return min <= char && char <= max
 }
 
-/**
- * Removes the given character class element from its character class.
- */
-function fixRemove(
-    context: RegExpContext,
-    element: CharacterClassElement,
-): Rule.ReportDescriptor["fix"] {
-    const parent = element.parent
-    if (parent.type !== "CharacterClass") {
-        throw new Error("Only call this function for character class elements.")
-    }
-
-    return context.fixReplaceNode(element, () => {
-        const textBefore = parent.raw.slice(0, element.start - parent.start)
-        const textAfter = parent.raw.slice(element.end - parent.start)
-
-        if (mightCreateNewElement(textBefore, textAfter)) {
-            return null
-        }
-
-        const elementBefore: CharacterClassElement | undefined =
-            parent.elements[parent.elements.indexOf(element) - 1]
-        const elementAfter: CharacterClassElement | undefined =
-            parent.elements[parent.elements.indexOf(element) + 1]
-
-        if (
-            elementBefore &&
-            elementAfter &&
-            elementBefore.type === "Character" &&
-            elementBefore.raw === "-" &&
-            elementAfter.type === "Character"
-        ) {
-            // e.g. [\s0-\s9] -> [\s0-9] is incorrect
-            return null
-        }
-
-        // add a backslash if ...
-        if (
-            // ... the text character is a dash
-            // e.g. [a\w-b] -> [a\-b], [\w-b] -> [-b], [\s\w-b] -> [\s-b]
-            (textAfter.startsWith("-") &&
-                elementBefore &&
-                elementBefore.type === "Character") ||
-            // ... the next character is a caret and the caret will then be the
-            // first character in the character class
-            // e.g. [a^b] -> [\^b], [ba^] -> [b^]
-            (textAfter.startsWith("^") && !parent.negate && !elementBefore)
-        ) {
-            return "\\"
-        }
-
-        return ""
-    })
-}
-
 export default createRule("no-dupe-characters-character-class", {
     meta: {
         type: "suggestion",
@@ -202,7 +146,10 @@ export default createRule("no-dupe-characters-character-class", {
                     data: {
                         duplicate: mentionChar(duplicate),
                     },
-                    fix: fixRemove(regexpContext, duplicate),
+                    fix: fixRemoveCharacterClassElement(
+                        regexpContext,
+                        duplicate,
+                    ),
                 })
             } else {
                 context.report({
@@ -213,7 +160,10 @@ export default createRule("no-dupe-characters-character-class", {
                         duplicate: mentionChar(duplicate),
                         element: mentionChar(element),
                     },
-                    fix: fixRemove(regexpContext, duplicate),
+                    fix: fixRemoveCharacterClassElement(
+                        regexpContext,
+                        duplicate,
+                    ),
                 })
             }
         }
@@ -259,7 +209,10 @@ export default createRule("no-dupe-characters-character-class", {
                     subsetElement: mentionChar(subsetElement),
                     element: mentionChar(element),
                 },
-                fix: fixRemove(regexpContext, subsetElement),
+                fix: fixRemoveCharacterClassElement(
+                    regexpContext,
+                    subsetElement,
+                ),
             })
         }
 
@@ -283,7 +236,10 @@ export default createRule("no-dupe-characters-character-class", {
                         .map((e) => e.raw)
                         .join("")}' (${elements.map(mentionChar).join(", ")})`,
                 },
-                fix: fixRemove(regexpContext, subsetElement),
+                fix: fixRemoveCharacterClassElement(
+                    regexpContext,
+                    subsetElement,
+                ),
             })
         }
 
diff --git a/lib/rules/no-dupe-disjunctions.ts b/lib/rules/no-dupe-disjunctions.ts
index 02dc312..ca530c3 100644
--- a/lib/rules/no-dupe-disjunctions.ts
+++ b/lib/rules/no-dupe-disjunctions.ts
@@ -9,7 +9,12 @@ import type {
     Quantifier,
 } from "regexpp/ast"
 import type { RegExpContext } from "../utils"
-import { createRule, defineRegexpVisitor } from "../utils"
+import {
+    createRule,
+    defineRegexpVisitor,
+    fixRemoveCharacterClassElement,
+    fixRemoveAlternative,
+} from "../utils"
 import { isCoveredNode, isEqualNodes } from "../utils/regexp-ast"
 import type { Expression, FiniteAutomaton, NoParent, ReadonlyNFA } from "refa"
 import {
@@ -35,6 +40,8 @@ import { canReorder } from "../utils/reorder-alternatives"
 import { mention, mentionChar } from "../utils/mention"
 import type { NestedAlternative } from "../utils/partial-parser"
 import { PartialParser } from "../utils/partial-parser"
+import type { Rule } from "eslint"
+import { getAllowedCharRanges, inRange } from "../utils/char-ranges"
 
 type ParentNode = Group | CapturingGroup | Pattern | LookaroundAssertion
 
@@ -837,6 +844,34 @@ function mentionNested(nested: NestedAlternative): string {
     return mentionChar(nested)
 }
 
+/**
+ * Returns a fix that removes the given alternative.
+ */
+function fixRemoveNestedAlternative(
+    context: RegExpContext,
+    alternative: NestedAlternative,
+) {
+    switch (alternative.type) {
+        case "Alternative":
+            return fixRemoveAlternative(context, alternative)
+
+        case "Character":
+        case "CharacterClassRange":
+        case "CharacterSet": {
+            if (alternative.parent.type !== "CharacterClass") {
+                // This isn't supposed to happen. We can't just remove the only
+                // alternative of its parent
+                return () => null
+            }
+
+            return fixRemoveCharacterClassElement(context, alternative)
+        }
+
+        default:
+            throw assertNever(alternative)
+    }
+}
+
 const enum ReportOption {
     all = "all",
     trivial = "trivial",
@@ -873,6 +908,7 @@ export default createRule("no-dupe-disjunctions", {
             category: "Possible Errors",
             recommended: true,
         },
+        hasSuggestions: true,
         schema: [
             {
                 type: "object",
@@ -905,6 +941,9 @@ export default createRule("no-dupe-disjunctions", {
                 "Unexpected superset. This alternative is a superset of {{others}}. It might be possible to remove the other alternative(s).{{cap}}{{exp}}",
             overlap:
                 "Unexpected overlap. This alternative overlaps with {{others}}. The overlap is {{expr}}.{{cap}}{{exp}}",
+
+            remove: "Remove the {{alternative}} {{type}}.",
+            replaceRange: "Replace {{range}} with {{replacement}}.",
         },
         type: "suggestion", // "problem",
     },
@@ -917,6 +956,8 @@ export default createRule("no-dupe-disjunctions", {
         const report: ReportOption =
             context.options[0]?.report ?? ReportOption.trivial
 
+        const allowedRanges = getAllowedCharRanges(undefined, context)
+
         /**
          * Create visitor
          */
@@ -1104,6 +1145,103 @@ export default createRule("no-dupe-disjunctions", {
                 }
             }
 
+            /** Prints the given character. */
+            function printChar(char: number): string {
+                if (inRange(allowedRanges, char)) {
+                    return String.fromCodePoint(char)
+                }
+
+                if (char === 0) return "\\0"
+                if (char <= 0xff)
+                    return `\\x${char.toString(16).padStart(2, "0")}`
+                if (char <= 0xffff)
+                    return `\\u${char.toString(16).padStart(4, "0")}`
+
+                return `\\u{${char.toString(16)}}`
+            }
+
+            /** Returns suggestions for fixing the given report */
+            function getSuggestions(
+                result: Result,
+            ): Rule.SuggestionReportDescriptor[] {
+                if (result.type === "Overlap" || result.type === "Superset") {
+                    // the types of results cannot be trivially fixed by
+                    // removing an alternative.
+                    return []
+                }
+
+                const alternative =
+                    result.type === "NestedSubset" ||
+                    result.type === "PrefixNestedSubset"
+                        ? result.nested
+                        : result.alternative
+
+                const containsCapturingGroup = hasSomeDescendant(
+                    alternative,
+                    (d) => d.type === "CapturingGroup",
+                )
+                if (containsCapturingGroup) {
+                    // we can't just remove a capturing group
+                    return []
+                }
+
+                if (
+                    alternative.type === "Character" &&
+                    alternative.parent.type === "CharacterClassRange"
+                ) {
+                    const range = alternative.parent
+
+                    let replacement
+                    if (range.min.value + 1 === range.max.value) {
+                        replacement =
+                            range.min === alternative
+                                ? range.max.raw
+                                : range.min.raw
+                    } else {
+                        if (range.min === alternative) {
+                            // replace with {min+1}-{max}
+                            const min = printChar(range.min.value + 1)
+                            replacement = `${min}-${range.max.raw}`
+                        } else {
+                            // replace with {min}-{max-1}
+                            const max = printChar(range.max.value - 1)
+                            replacement = `${range.min.raw}-${max}`
+                        }
+                    }
+
+                    return [
+                        {
+                            messageId: "replaceRange",
+                            data: {
+                                range: mentionChar(range),
+                                replacement: mention(replacement),
+                            },
+                            fix: regexpContext.fixReplaceNode(
+                                range,
+                                replacement,
+                            ),
+                        },
+                    ]
+                }
+
+                return [
+                    {
+                        messageId: "remove",
+                        data: {
+                            alternative: mentionNested(alternative),
+                            type:
+                                alternative.type === "Alternative"
+                                    ? "alternative"
+                                    : "element",
+                        },
+                        fix: fixRemoveNestedAlternative(
+                            regexpContext,
+                            alternative,
+                        ),
+                    },
+                ]
+            }
+
             /** Report the given result. */
             function reportResult(result: Result, { stared }: FilterInfo) {
                 let exp
@@ -1136,6 +1274,8 @@ export default createRule("no-dupe-disjunctions", {
                     result.others.map((a) => a.raw).join("|"),
                 )
 
+                const suggest = getSuggestions(result)
+
                 switch (result.type) {
                     case "Duplicate":
                         context.report({
@@ -1143,6 +1283,7 @@ export default createRule("no-dupe-disjunctions", {
                             loc,
                             messageId: "duplicate",
                             data: { exp, cap, others },
+                            suggest,
                         })
                         break
 
@@ -1152,6 +1293,7 @@ export default createRule("no-dupe-disjunctions", {
                             loc,
                             messageId: "subset",
                             data: { exp, cap, others },
+                            suggest,
                         })
                         break
 
@@ -1167,6 +1309,7 @@ export default createRule("no-dupe-disjunctions", {
                                 root: mention(result.alternative),
                                 nested: mentionNested(result.nested),
                             },
+                            suggest,
                         })
                         break
 
@@ -1176,6 +1319,7 @@ export default createRule("no-dupe-disjunctions", {
                             loc,
                             messageId: "prefixSubset",
                             data: { exp, cap, others },
+                            suggest,
                         })
                         break
 
@@ -1191,6 +1335,7 @@ export default createRule("no-dupe-disjunctions", {
                                 root: mention(result.alternative),
                                 nested: mentionNested(result.nested),
                             },
+                            suggest,
                         })
                         break
 
@@ -1200,6 +1345,7 @@ export default createRule("no-dupe-disjunctions", {
                             loc,
                             messageId: "superset",
                             data: { exp, cap, others },
+                            suggest,
                         })
                         break
 
@@ -1216,6 +1362,7 @@ export default createRule("no-dupe-disjunctions", {
                                     faToSource(result.overlap, flags),
                                 ),
                             },
+                            suggest,
                         })
                         break
 
diff --git a/lib/utils/index.ts b/lib/utils/index.ts
index 0215203..5ed5d44 100644
--- a/lib/utils/index.ts
+++ b/lib/utils/index.ts
@@ -2,7 +2,9 @@ import type * as ESTree from "estree"
 import type { RuleListener, RuleModule, PartialRuleModule } from "../types"
 import type { RegExpVisitor } from "regexpp/visitor"
 import type {
+    Alternative,
     CapturingGroup,
+    CharacterClassElement,
     Element,
     Node,
     Pattern,
@@ -1166,6 +1168,94 @@ export function mightCreateNewElement(
     return false
 }
 
+/**
+ * Removes the given character class element from its character class.
+ *
+ * If the given element is not a direct element of a character class, an error
+ * will be thrown.
+ */
+export function fixRemoveCharacterClassElement(
+    context: RegExpContext,
+    element: CharacterClassElement,
+): (fixer: Rule.RuleFixer) => Rule.Fix | null {
+    const cc = element.parent
+    if (cc.type !== "CharacterClass") {
+        throw new Error("Only call this function for character class elements.")
+    }
+
+    return context.fixReplaceNode(element, () => {
+        const textBefore = cc.raw.slice(0, element.start - cc.start)
+        const textAfter = cc.raw.slice(element.end - cc.start)
+
+        if (mightCreateNewElement(textBefore, textAfter)) {
+            return null
+        }
+
+        const elementBefore: CharacterClassElement | undefined =
+            cc.elements[cc.elements.indexOf(element) - 1]
+        const elementAfter: CharacterClassElement | undefined =
+            cc.elements[cc.elements.indexOf(element) + 1]
+
+        if (
+            elementBefore &&
+            elementAfter &&
+            elementBefore.type === "Character" &&
+            elementBefore.raw === "-" &&
+            elementAfter.type === "Character"
+        ) {
+            // e.g. [\s0-\s9] -> [\s0-9] is incorrect
+            return null
+        }
+
+        // add a backslash if ...
+        if (
+            // ... the text character is a dash
+            // e.g. [a\w-b] -> [a\-b], [\w-b] -> [-b], [\s\w-b] -> [\s-b]
+            (textAfter.startsWith("-") &&
+                elementBefore &&
+                elementBefore.type === "Character") ||
+            // ... the next character is a caret and the caret will then be the
+            // first character in the character class
+            // e.g. [a^b] -> [\^b], [ba^] -> [b^]
+            (textAfter.startsWith("^") && !cc.negate && !elementBefore)
+        ) {
+            return "\\"
+        }
+
+        return ""
+    })
+}
+
+/**
+ * Removes the given alternative from its parent.
+ */
+export function fixRemoveAlternative(
+    context: RegExpContext,
+    alternative: Alternative,
+): (fixer: Rule.RuleFixer) => Rule.Fix | null {
+    const { parent } = alternative
+    if (parent.alternatives.length === 1) {
+        // We can't really remove an alternative if the parent only has one.
+        // So instead, we will replace the alternative with an empty character
+        // set. This ensure that the alternative is *functionally* removed.
+        return context.fixReplaceNode(alternative, "[]")
+    }
+
+    return context.fixReplaceNode(parent, () => {
+        let { start, end } = alternative
+        if (parent.alternatives[0] === alternative) {
+            end++
+        } else {
+            start--
+        }
+
+        const before = parent.raw.slice(0, start - parent.start)
+        const after = parent.raw.slice(end - parent.start)
+
+        return before + after
+    })
+}
+
 /**
  * Check the siblings to see if the regex doesn't change when unwrapped.
  */


Test patch:
diff --git a/tests/lib/rules/no-dupe-disjunctions.ts b/tests/lib/rules/no-dupe-disjunctions.ts
index 6d25f34..34bc3ee 100644
--- a/tests/lib/rules/no-dupe-disjunctions.ts
+++ b/tests/lib/rules/no-dupe-disjunctions.ts
@@ -398,6 +398,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '(?:ba|ac)' that go through 'ba' are a strict subset of '(?:ab|ba)'. This element can be removed.",
                     column: 16,
+                    suggestions: [{ output: String(/((?:ab|ba)|(?:ac))/) }],
                 },
             ],
         },
@@ -408,6 +409,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless alternative. This alternative is a strict subset of 'a+' and can be removed.",
                     column: 5,
+                    suggestions: [{ output: String(/a+|b|c/) }],
                 },
             ],
         },
@@ -418,6 +420,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '(?:a|b|c)' that go through 'a' are a strict subset of 'a+'. This element can be removed.",
                     column: 8,
+                    suggestions: [{ output: String(/a+|(?:b|c)/) }],
                 },
             ],
         },
@@ -428,6 +431,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '[abc]' that go through 'a' (U+0061) are a strict subset of 'a+'. This element can be removed.",
                     column: 6,
+                    suggestions: [{ output: String(/a+|[bc]/) }],
                 },
             ],
         },
@@ -438,6 +442,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '[a-c]' that go through 'a' (U+0061) are a strict subset of 'a+'. This element can be removed.",
                     column: 6,
+                    suggestions: [{ output: String(/a+|[b-c]/) }],
                 },
             ],
         },
@@ -448,6 +453,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless alternative. This alternative is already covered by 'a' and can be removed.",
                     column: 4,
+                    suggestions: [{ output: String(/a|ba/) }],
                 },
             ],
         },
@@ -458,6 +464,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '(a|b)a' that go through 'a' are already covered by 'a'. This element can be removed.",
                     column: 5,
+                    suggestions: [{ output: String(/a|(b)a/) }],
                 },
             ],
         },
@@ -468,6 +475,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '(?:(a)|b)a' that go through '(a)' are already covered by 'a'. This element can be removed. Careful! This alternative contains capturing groups which might be difficult to remove.",
                     column: 7,
+                    suggestions: [],
                 },
             ],
         },
@@ -478,6 +486,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
                     message:
                         "Unexpected useless element. All paths of '[ab]a' that go through 'a' (U+0061) are already covered by 'a'. This element can be removed.",
                     column: 5,
+                    suggestions: [{ output: String(/a|[b]a/) }],
                 },
             ],
         },