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

Issue:
`no-useless-flag` needs better error messages
![image](https://user-images.githubusercontent.com/20878432/122079371-a1ce3d80-cdfd-11eb-8f84-3e3170220509.png)

> The 'g' flag is unnecessary because not using global testing.

The current error message doesn't really say anything. A better error message would be: "The 'g' flag is unnecessary because `String.prototype.split` ignores the 'g' flag." or something like that.

Same problem for other string methods and flags.

Also, now that I got the error message: What's "global testing" @ota-meshi?

---

I wound how we could fix this.

The current implementation seems to mark flags as used. Maybe we could keep a list of everything that uses the regex? If we know that the regex is used by exactly one method and that method doesn't use a flag, we can give a more specific error message. Would that work?

Pull request description:
Improves reporting of the `g` and `y` flags in `regexp/no-useless-flag` rule.
This PR improves the reporting of the `g` and `y` flags in `regexp/no-useless-flag` rule.

- Improved the report message to make it easier to see how the g and y flags are used incorrectly.
- Improved the rule to autofix when a regex instance is used only once and given directly to a method.

close #253



Patch:
diff --git a/lib/rules/no-useless-flag.ts b/lib/rules/no-useless-flag.ts
index 58921b6..5b19422 100644
--- a/lib/rules/no-useless-flag.ts
+++ b/lib/rules/no-useless-flag.ts
@@ -34,7 +34,19 @@ type CodePathId = {
     loopNode: Statement | undefined
 }
 
+type RegExpMethodKind =
+    | "search"
+    | "split"
+    | "exec"
+    | "test"
+    | "match"
+    | "matchAll"
+    | "replace"
+    | "replaceAll"
+
 class RegExpReference {
+    public regExpContext: RegExpContext
+
     public readonly defineNode: RegExpExpression
 
     public defineId?: CodePathId
@@ -43,8 +55,6 @@ class RegExpReference {
         Expression,
         {
             marked?: boolean
-            usedInSearch?: boolean
-            usedInSplit?: boolean
             usedInExec?: { id: CodePathId }
             usedInTest?: { id: CodePathId }
         }
@@ -52,23 +62,18 @@ class RegExpReference {
 
     private readonly state: {
         track: boolean
-        usedIn: {
-            search?: boolean
-            split?: boolean
-            exec?: boolean
-            test?: boolean
-            match?: boolean
-            matchAll?: boolean
-            replace?: boolean
-            replaceAll?: boolean
-        }
+        usedNodes: Map<RegExpMethodKind, Expression[]>
         hasUnusedExpression?: boolean
     } = {
-        usedIn: {},
+        usedNodes: new Map(),
         track: true,
     }
 
-    public constructor(defineNode: RegExpExpression) {
+    public constructor(
+        regExpContext: RegExpContext,
+        defineNode: RegExpExpression,
+    ) {
+        this.regExpContext = regExpContext
         this.defineNode = defineNode
     }
 
@@ -84,18 +89,16 @@ class RegExpReference {
         const exprState = this.readNodes.get(node)
         if (exprState) {
             exprState.marked = true
-            exprState.usedInSearch = true
         }
-        this.state.usedIn.search = true
+        this.addUsedNode("search", node)
     }
 
     public markAsUsedInSplit(node: Expression) {
         const exprState = this.readNodes.get(node)
         if (exprState) {
             exprState.marked = true
-            exprState.usedInSplit = true
         }
-        this.state.usedIn.split = true
+        this.addUsedNode("split", node)
     }
 
     public markAsUsedInExec(
@@ -108,7 +111,7 @@ class RegExpReference {
             exprState.marked = true
             exprState.usedInExec = { id: { codePathId, loopNode } }
         }
-        this.state.usedIn.exec = true
+        this.addUsedNode("exec", node)
     }
 
     public markAsUsedInTest(
@@ -121,23 +124,12 @@ class RegExpReference {
             exprState.marked = true
             exprState.usedInTest = { id: { codePathId, loopNode } }
         }
-        this.state.usedIn.test = true
+        this.addUsedNode("test", node)
     }
 
-    public isUsed(
-        kinds: (
-            | "search"
-            | "split"
-            | "exec"
-            | "test"
-            | "match"
-            | "matchAll"
-            | "replace"
-            | "replaceAll"
-        )[],
-    ) {
+    public isUsed(kinds: RegExpMethodKind[]) {
         for (const kind of kinds) {
-            if (this.state.usedIn[kind]) {
+            if (this.state.usedNodes.has(kind)) {
                 return true
             }
         }
@@ -148,13 +140,29 @@ class RegExpReference {
         return !this.state.track
     }
 
-    public markAsUsed(kind: "match" | "matchAll" | "replace" | "replaceAll") {
-        this.state.usedIn[kind] = true
+    public markAsUsed(
+        kind: "match" | "matchAll" | "replace" | "replaceAll",
+        exprNode: Expression,
+    ) {
+        this.addUsedNode(kind, exprNode)
     }
 
     public markAsCannotTrack() {
         this.state.track = false
     }
+
+    public getUsedNodes() {
+        return this.state.usedNodes
+    }
+
+    private addUsedNode(kind: RegExpMethodKind, exprNode: Expression) {
+        const list = this.state.usedNodes.get(kind)
+        if (list) {
+            list.push(exprNode)
+        } else {
+            this.state.usedNodes.set(kind, [exprNode])
+        }
+    }
 }
 
 /**
@@ -187,7 +195,7 @@ function getFlagLocation(
  */
 function fixRemoveFlag(
     { flagsString, fixReplaceFlags }: RegExpContext,
-    flag: "i" | "m" | "s" | "g",
+    flag: "i" | "m" | "s" | "g" | "y",
 ) {
     if (flagsString) {
         return fixReplaceFlags(flagsString.replace(flag, ""))
@@ -334,33 +342,65 @@ function createUselessDotAllFlagVisitor(context: Rule.RuleContext) {
  * Create visitor for verify unnecessary g flag
  */
 function createUselessGlobalFlagVisitor(context: Rule.RuleContext) {
+    const enum ReportKind {
+        // It is used only in "split".
+        usedOnlyInSplit,
+        // It is used only in "search".
+        usedOnlyInSearch,
+        // It is used only once in "exec".
+        usedOnlyOnceInExec,
+        // It is used only once in "test".
+        usedOnlyOnceInTest,
+        // The global flag is given, but it is not used.
+        unused,
+    }
+
+    type ReportData = { kind: ReportKind; fixable?: boolean }
+
     /**
      * Report for useless global flag
      */
-    function reportUselessGlobalFlag(regExpReference: RegExpReference) {
+    function reportUselessGlobalFlag(
+        regExpReference: RegExpReference,
+        data: ReportData,
+    ) {
         const node = regExpReference.defineNode
         context.report({
             node,
             loc: getFlagLocation(context, node, "g"),
-            messageId: "uselessGlobalFlag",
+            messageId:
+                data.kind === ReportKind.usedOnlyInSplit
+                    ? "uselessGlobalFlagForSplit"
+                    : data.kind === ReportKind.usedOnlyInSearch
+                    ? "uselessGlobalFlagForSearch"
+                    : data.kind === ReportKind.usedOnlyOnceInTest
+                    ? "uselessGlobalFlagForTest"
+                    : data.kind === ReportKind.usedOnlyOnceInExec
+                    ? "uselessGlobalFlagForExec"
+                    : "uselessGlobalFlag",
+            fix: data.fixable
+                ? fixRemoveFlag(regExpReference.regExpContext, "g")
+                : null,
         })
     }
 
     /**
-     * Checks Check whether the flag needs to be reported.
+     * Checks if it needs to be reported and returns the report data if it needs to be reported.
      */
-    function isNeedReport(regExpReference: RegExpReference): boolean {
+    function getReportData(
+        regExpReference: RegExpReference,
+    ): null | ReportData {
         let countOfUsedInExecOrTest = 0
         for (const readData of regExpReference.readNodes.values()) {
             if (!readData.marked) {
                 // There is expression node whose usage is unknown.
-                return false
+                return null
             }
             const usedInExecOrTest = readData.usedInExec || readData.usedInTest
             if (usedInExecOrTest) {
                 if (!regExpReference.defineId) {
                     // The definition scope is unknown. Probably broken.
-                    return false
+                    return null
                 }
                 if (
                     regExpReference.defineId.codePathId ===
@@ -371,25 +411,53 @@ function createUselessGlobalFlagVisitor(context: Rule.RuleContext) {
                     countOfUsedInExecOrTest++
                     if (countOfUsedInExecOrTest > 1) {
                         // Multiple `exec` or `test` have been used.
-                        return false
+                        return null
                     }
                     continue
                 } else {
                     // Used `exec` or` test` in different scopes. It may be called multiple times.
-                    return false
+                    return null
                 }
             }
         }
         // Need to report
-        return true
+        return buildReportData(regExpReference)
+    }
+
+    /**
+     * Build the report data.
+     */
+    function buildReportData(regExpReference: RegExpReference) {
+        const usedNodes = regExpReference.getUsedNodes()
+        if (usedNodes.size === 1) {
+            const [[method, nodes]] = usedNodes
+            // Make it fixable only if the regex is used directly.
+            const fixable =
+                nodes.length === 1 && nodes.includes(regExpReference.defineNode)
+            if (method === "split") {
+                return {
+                    kind: ReportKind.usedOnlyInSplit,
+                    fixable,
+                }
+            }
+            if (method === "search") {
+                return { kind: ReportKind.usedOnlyInSearch, fixable }
+            }
+            if (method === "exec" && nodes.length === 1)
+                return { kind: ReportKind.usedOnlyOnceInExec, fixable }
+            if (method === "test" && nodes.length === 1)
+                return { kind: ReportKind.usedOnlyOnceInTest, fixable }
+        }
+        return { kind: ReportKind.unused }
     }
 
     return createRegExpReferenceExtractVisitor(context, {
         flag: "global",
         exit(regExpReferenceList) {
             for (const regExpReference of regExpReferenceList) {
-                if (isNeedReport(regExpReference)) {
-                    reportUselessGlobalFlag(regExpReference)
+                const report = getReportData(regExpReference)
+                if (report != null) {
+                    reportUselessGlobalFlag(regExpReference, report)
                 }
             }
         },
@@ -408,38 +476,67 @@ function createUselessGlobalFlagVisitor(context: Rule.RuleContext) {
  * Create visitor for verify unnecessary y flag
  */
 function createUselessStickyFlagVisitor(context: Rule.RuleContext) {
+    type ReportData = { fixable?: boolean }
+
     /**
      * Report for useless sticky flag
      */
-    function reportUselessGlobalFlag(regExpReference: RegExpReference) {
+    function reportUselessStickyFlag(
+        regExpReference: RegExpReference,
+        data: ReportData,
+    ) {
         const node = regExpReference.defineNode
         context.report({
             node,
             loc: getFlagLocation(context, node, "y"),
             messageId: "uselessStickyFlag",
+            fix: data.fixable
+                ? fixRemoveFlag(regExpReference.regExpContext, "y")
+                : null,
         })
     }
 
     /**
-     * Checks Check whether the flag needs to be reported.
+     * Checks if it needs to be reported and returns the report data if it needs to be reported.
      */
-    function isNeedReport(regExpReference: RegExpReference): boolean {
+    function getReportData(
+        regExpReference: RegExpReference,
+    ): null | ReportData {
         for (const readData of regExpReference.readNodes.values()) {
             if (!readData.marked) {
                 // There is expression node whose usage is unknown.
-                return false
+                return null
+            }
+        } // Need to report
+        return buildReportData(regExpReference)
+    }
+
+    /**
+     * Build the report data.
+     */
+    function buildReportData(regExpReference: RegExpReference) {
+        const usedNodes = regExpReference.getUsedNodes()
+        if (usedNodes.size === 1) {
+            const [[method, nodes]] = usedNodes
+            // Make it fixable only if the regex is used directly.
+            const fixable =
+                nodes.length === 1 && nodes.includes(regExpReference.defineNode)
+            if (method === "split") {
+                return {
+                    fixable,
+                }
             }
         }
-        // Need to report
-        return true
+        return {}
     }
 
     return createRegExpReferenceExtractVisitor(context, {
         flag: "sticky",
         exit(regExpReferenceList) {
             for (const regExpReference of regExpReferenceList) {
-                if (isNeedReport(regExpReference)) {
-                    reportUselessGlobalFlag(regExpReference)
+                const report = getReportData(regExpReference)
+                if (report != null) {
+                    reportUselessStickyFlag(regExpReference, report)
                 }
             }
         },
@@ -526,9 +623,13 @@ function createRegExpReferenceExtractVisitor(
 
     return compositingVisitors(
         defineRegexpVisitor(context, {
-            createVisitor({ flags, regexpNode }: RegExpContext) {
+            createVisitor(regExpContext: RegExpContext) {
+                const { flags, regexpNode } = regExpContext
                 if (flags[flag]) {
-                    const regExpReference = new RegExpReference(regexpNode)
+                    const regExpReference = new RegExpReference(
+                        regExpContext,
+                        regexpNode,
+                    )
                     regExpReferenceList.push(regExpReference)
                     regExpReferenceMap.set(regexpNode, regExpReference)
                     for (const ref of extractExpressionReferences(
@@ -647,7 +748,10 @@ function createRegExpReferenceExtractVisitor(
                     const regExpReference = regExpReferenceMap.get(
                         node.arguments[0],
                     )
-                    regExpReference?.markAsUsed(node.callee.property.name)
+                    regExpReference?.markAsUsed(
+                        node.callee.property.name,
+                        node.arguments[0],
+                    )
                 }
             },
         },
@@ -688,9 +792,17 @@ export default createRule("no-useless-flag", {
             uselessDotAllFlag:
                 "The 's' flag is unnecessary because the pattern does not contain dots (.).",
             uselessGlobalFlag:
-                "The 'g' flag is unnecessary because not using global testing.",
+                "The 'g' flag is unnecessary because the regex does not use global search.",
+            uselessGlobalFlagForTest:
+                "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
+            uselessGlobalFlagForExec:
+                "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.exec'.",
+            uselessGlobalFlagForSplit:
+                "The 'g' flag is unnecessary because 'String.prototype.split' ignores the 'g' flag.",
+            uselessGlobalFlagForSearch:
+                "The 'g' flag is unnecessary because 'String.prototype.search' ignores the 'g' flag.",
             uselessStickyFlag:
-                "The 'y' flag is unnecessary because not using sticky search.",
+                "The 'y' flag is unnecessary because 'String.prototype.split' ignores the 'y' flag.",
         },
         type: "suggestion", // "problem",
     },


Test patch:
diff --git a/tests/lib/rules/no-useless-flag.ts b/tests/lib/rules/no-useless-flag.ts
index 3fae3c2..70d5eb7 100644
--- a/tests/lib/rules/no-useless-flag.ts
+++ b/tests/lib/rules/no-useless-flag.ts
@@ -330,11 +330,38 @@ tester.run("no-useless-flag", rule as any, {
 
             str.search(/foo/g);
             `,
-            output: null,
+            output: `
+            /* ✓ GOOD */
+            const regex1 = /foo/g;
+            const str = 'table football, foosball';
+            while ((array = regex1.exec(str)) !== null) {
+              //
+            }
+
+            const regex2 = /foo/g;
+            regex2.test(string);
+            regex2.test(string);
+
+            str.replace(/foo/g, 'bar');
+            str.replaceAll(/foo/g, 'bar');
+
+            /* ✗ BAD */
+            /foo/.test(string);
+            const regex3 = /foo/g;
+            regex3.test(string); // You have used it only once.
+
+            /foo/.exec(string);
+            const regex4 = /foo/g;
+            regex4.exec(string); // You have used it only once.
+
+            new RegExp('foo', '').test(string);
+
+            str.search(/foo/);
+            `,
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 17,
                     column: 18,
                     endLine: 17,
@@ -342,7 +369,7 @@ tester.run("no-useless-flag", rule as any, {
                 },
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 18,
                     column: 33,
                     endLine: 18,
@@ -350,7 +377,7 @@ tester.run("no-useless-flag", rule as any, {
                 },
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.exec'.",
                     line: 21,
                     column: 18,
                     endLine: 21,
@@ -358,7 +385,7 @@ tester.run("no-useless-flag", rule as any, {
                 },
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.exec'.",
                     line: 22,
                     column: 33,
                     endLine: 22,
@@ -366,7 +393,7 @@ tester.run("no-useless-flag", rule as any, {
                 },
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 25,
                     column: 31,
                     endLine: 25,
@@ -374,7 +401,7 @@ tester.run("no-useless-flag", rule as any, {
                 },
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because 'String.prototype.search' ignores the 'g' flag.",
                     line: 27,
                     column: 29,
                     endLine: 27,
@@ -386,11 +413,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             /foo/g.test(bar);
             `,
-            output: null,
+            output: `
+            /foo/.test(bar);
+            `,
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 2,
                     column: 18,
                 },
@@ -400,11 +429,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             /foo/g.exec(bar);
             `,
-            output: null,
+            output: `
+            /foo/.exec(bar);
+            `,
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.exec'.",
                     line: 2,
                     column: 18,
                 },
@@ -414,11 +445,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             new RegExp('foo', 'g').test(bar);
             `,
-            output: null,
+            output: `
+            new RegExp('foo', '').test(bar);
+            `,
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 2,
                     column: 31,
                 },
@@ -428,11 +461,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             "foo foo".search(/foo/g);
             `,
-            output: null,
+            output: `
+            "foo foo".search(/foo/);
+            `,
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because 'String.prototype.search' ignores the 'g' flag.",
                     line: 2,
                     column: 35,
                 },
@@ -447,7 +482,7 @@ tester.run("no-useless-flag", rule as any, {
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 2,
                     column: 32,
                 },
@@ -464,7 +499,7 @@ tester.run("no-useless-flag", rule as any, {
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex is used only once in 'RegExp.prototype.test'.",
                     line: 3,
                     column: 36,
                 },
@@ -484,12 +519,38 @@ tester.run("no-useless-flag", rule as any, {
             errors: [
                 {
                     message:
-                        "The 'g' flag is unnecessary because not using global testing.",
+                        "The 'g' flag is unnecessary because the regex does not use global search.",
+                    line: 2,
+                    column: 28,
+                },
+            ],
+        },
+        {
+            code: `
+            const a = /foo/g;
+            'str'.split(a)
+            `,
+            output: null,
+            errors: [
+                {
+                    message:
+                        "The 'g' flag is unnecessary because 'String.prototype.split' ignores the 'g' flag.",
                     line: 2,
                     column: 28,
                 },
             ],
         },
+        {
+            code: `
+            const a = /foo/g;
+            'str'.split(a)
+            'str'.split(a)
+            `,
+            output: null,
+            errors: [
+                "The 'g' flag is unnecessary because 'String.prototype.split' ignores the 'g' flag.",
+            ],
+        },
 
         // y
         {
@@ -513,11 +574,30 @@ tester.run("no-useless-flag", rule as any, {
             /* ✗ BAD */
             str.split(/foo/y);
             `,
-            output: null,
+            output: `
+            /* ✓ GOOD */
+            const regex1 = /foo/y;
+            const str = 'table football, foosball';
+            regex1.lastIndex = 6
+            var array = regex1.exec(str)
+            
+            const regex2 = /foo/y;
+            regex2.test(string);
+            regex2.test(string);
+            
+            str.replace(/foo/y, 'bar');
+            str.replaceAll(/foo/gy, 'bar');
+
+            const regexp3 = /foo/y
+            str.search(regexp3)
+            
+            /* ✗ BAD */
+            str.split(/foo/);
+            `,
             errors: [
                 {
                     message:
-                        "The 'y' flag is unnecessary because not using sticky search.",
+                        "The 'y' flag is unnecessary because 'String.prototype.split' ignores the 'y' flag.",
                     line: 19,
                     column: 28,
                 },
@@ -527,11 +607,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             "str".split(/foo/y)
             `,
-            output: null,
+            output: `
+            "str".split(/foo/)
+            `,
             errors: [
                 {
                     message:
-                        "The 'y' flag is unnecessary because not using sticky search.",
+                        "The 'y' flag is unnecessary because 'String.prototype.split' ignores the 'y' flag.",
                     line: 2,
                     column: 30,
                 },
@@ -541,11 +623,13 @@ tester.run("no-useless-flag", rule as any, {
             code: `
             "str".split(new RegExp('foo', 'y'));
             `,
-            output: null,
+            output: `
+            "str".split(new RegExp('foo', ''));
+            `,
             errors: [
                 {
                     message:
-                        "The 'y' flag is unnecessary because not using sticky search.",
+                        "The 'y' flag is unnecessary because 'String.prototype.split' ignores the 'y' flag.",
                     line: 2,
                     column: 43,
                 },
@@ -563,7 +647,7 @@ tester.run("no-useless-flag", rule as any, {
             errors: [
                 {
                     message:
-                        "The 'y' flag is unnecessary because not using sticky search.",
+                        "The 'y' flag is unnecessary because 'String.prototype.split' ignores the 'y' flag.",
                     line: 2,
                     column: 28,
                 },