Skip to content

Commit f9a03c8

Browse files
ota-meshiota-meshi
authored
Add regexp/confusing-quantifier rule (#93)
* Add `regexp/confusing-quantifier` rule * fix * update default * Change to use regexp-ast-analysis. * Update rule description
1 parent 30e6d70 commit f9a03c8

File tree

9 files changed

+216
-14
lines changed

9 files changed

+216
-14
lines changed

‎README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ The rules with the following star :star: are included in the `plugin:regexp/reco
8383

8484
| Rule ID | Description | |
8585
|:--------|:------------|:---|
86+
| [regexp/confusing-quantifier](https://ota-meshi.github.io/eslint-plugin-regexp/rules/confusing-quantifier.html) | disallow confusing quantifiers | |
8687
| [regexp/letter-case](https://ota-meshi.github.io/eslint-plugin-regexp/rules/letter-case.html) | enforce into your favorite case | :wrench: |
8788
| [regexp/match-any](https://ota-meshi.github.io/eslint-plugin-regexp/rules/match-any.html) | enforce match any character style | :star::wrench: |
8889
| [regexp/negation](https://ota-meshi.github.io/eslint-plugin-regexp/rules/negation.html) | enforce use of escapes on negation | :wrench: |

‎docs/rules/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ The rules with the following star :star: are included in the `plugin:regexp/reco
1111

1212
| Rule ID | Description | |
1313
|:--------|:------------|:---|
14+
| [regexp/confusing-quantifier](./confusing-quantifier.md) | disallow confusing quantifiers | |
1415
| [regexp/letter-case](./letter-case.md) | enforce into your favorite case | :wrench: |
1516
| [regexp/match-any](./match-any.md) | enforce match any character style | :star::wrench: |
1617
| [regexp/negation](./negation.md) | enforce use of escapes on negation | :wrench: |

‎docs/rules/confusing-quantifier.md

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
---
2+
pageClass: "rule-details"
3+
sidebarDepth: 0
4+
title: "regexp/confusing-quantifier"
5+
description: "disallow confusing quantifiers"
6+
---
7+
# regexp/confusing-quantifier
8+
9+
> disallow confusing quantifiers
10+
11+
- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> ***This rule has not been released yet.*** </badge>
12+
13+
## :book: Rule Details
14+
15+
Confusing quantifiers are ones which imply one thing but don't deliver on that.
16+
17+
An example of this is `(?:a?b*|c+){4}`. The group is quantified with `{4}` which
18+
implies that at least 4 characters will be matched but this is not the case. The
19+
whole pattern will match the empty string. It does that because in the `a?b*`
20+
alternative, it's possible to choose 0 many `a` and `b`. So rather than `{4}`,
21+
`{0,4}` should be used to reflect the fact that the empty string can be matched.
22+
23+
<eslint-code-block>
24+
25+
```js
26+
/* eslint regexp/confusing-quantifier: "error" */
27+
28+
/* ✓ GOOD */
29+
var foo = /a*/;
30+
var foo = /(a|b|c)+/;
31+
var foo = /a?/;
32+
33+
/* ✗ BAD */
34+
var foo = /(a?){4}/; // warns about `{4}`
35+
var foo = /(a?b*)+/; // warns about `+`
36+
```
37+
38+
</eslint-code-block>
39+
40+
## :wrench: Options
41+
42+
Nothing.
43+
44+
## :heart: Compatibility
45+
46+
This rule was taken from [eslint-plugin-clean-regex].
47+
This rule is compatible with [clean-regex/confusing-quantifier] rule.
48+
49+
[eslint-plugin-clean-regex]: https://github.com/RunDevelopment/eslint-plugin-clean-regex
50+
[clean-regex/confusing-quantifier]: https://github.com/RunDevelopment/eslint-plugin-clean-regex/blob/master/docs/rules/confusing-quantifier.md
51+
52+
## :mag: Implementation
53+
54+
- [Rule source](https://github.com/ota-meshi/eslint-plugin-regexp/blob/master/lib/rules/confusing-quantifier.ts)
55+
- [Test source](https://github.com/ota-meshi/eslint-plugin-regexp/blob/master/tests/lib/rules/confusing-quantifier.ts)

‎lib/rules/confusing-quantifier.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import type { Expression } from "estree"
2+
import type { RegExpVisitor } from "regexpp/visitor"
3+
import {
4+
createRule,
5+
defineRegexpVisitor,
6+
getQuantifierOffsets,
7+
getRegexpLocation,
8+
quantToString,
9+
} from "../utils"
10+
import { isPotentiallyEmpty } from "regexp-ast-analysis"
11+
12+
export default createRule("confusing-quantifier", {
13+
meta: {
14+
docs: {
15+
description: "disallow confusing quantifiers",
16+
// TODO Switch to recommended in the major version.
17+
// recommended: true,
18+
recommended: false,
19+
default: "warn",
20+
},
21+
schema: [],
22+
messages: {
23+
confusing:
24+
"This quantifier is confusing because its minimum is {{min}} but it can match the empty string. Maybe replace it with `{{proposal}}` to reflect that it can match the empty string?",
25+
},
26+
type: "problem",
27+
},
28+
create(context) {
29+
const sourceCode = context.getSourceCode()
30+
31+
/**
32+
* Create visitor
33+
* @param node
34+
*/
35+
function createVisitor(node: Expression): RegExpVisitor.Handlers {
36+
return {
37+
onQuantifierEnter(qNode) {
38+
if (qNode.min > 0 && isPotentiallyEmpty(qNode.element)) {
39+
const proposal = quantToString({ ...qNode, min: 0 })
40+
context.report({
41+
node,
42+
loc: getRegexpLocation(
43+
sourceCode,
44+
node,
45+
qNode,
46+
getQuantifierOffsets(qNode),
47+
),
48+
messageId: "confusing",
49+
data: {
50+
min: String(qNode.min),
51+
proposal,
52+
},
53+
})
54+
}
55+
},
56+
}
57+
}
58+
59+
return defineRegexpVisitor(context, {
60+
createVisitor,
61+
})
62+
},
63+
})

‎lib/rules/prefer-quantifier.ts

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
isDigit,
1010
isLetter,
1111
isSymbol,
12+
quantToString,
1213
} from "../utils"
1314

1415
class CharBuffer {
@@ -115,19 +116,11 @@ class CharBuffer {
115116
}
116117

117118
public getQuantifier(): string {
118-
const greedy = this.greedy === false ? "?" : ""
119-
if (this.min === 0 && this.max === Number.POSITIVE_INFINITY) {
120-
return `*${greedy}`
121-
} else if (this.min === 1 && this.max === Number.POSITIVE_INFINITY) {
122-
return `+${greedy}`
123-
} else if (this.min === 0 && this.max === 1) {
124-
return `?${greedy}`
125-
} else if (this.min === this.max) {
126-
return `{${this.min}}`
127-
} else if (this.max === Number.POSITIVE_INFINITY) {
128-
return `{${this.min},}${greedy}`
129-
}
130-
return `{${this.min},${this.max}}${greedy}`
119+
return quantToString({
120+
min: this.min,
121+
max: this.max,
122+
greedy: this.greedy !== false,
123+
})
131124
}
132125
}
133126

‎lib/utils/index.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,48 @@ export function getQuantifierOffsets(qNode: Quantifier): [number, number] {
539539
return [startOffset, endOffset]
540540
}
541541

542+
export interface Quant {
543+
min: number
544+
max: number
545+
greedy?: boolean
546+
}
547+
548+
/**
549+
* Returns the string representation of the given quantifier.
550+
*/
551+
export function quantToString(quant: Readonly<Quant>): string {
552+
if (
553+
quant.max < quant.min ||
554+
quant.min < 0 ||
555+
!Number.isInteger(quant.min) ||
556+
!(Number.isInteger(quant.max) || quant.max === Infinity)
557+
) {
558+
throw new Error(
559+
`Invalid quantifier { min: ${quant.min}, max: ${quant.max} }`,
560+
)
561+
}
562+
563+
let value
564+
if (quant.min === 0 && quant.max === 1) {
565+
value = "?"
566+
} else if (quant.min === 0 && quant.max === Infinity) {
567+
value = "*"
568+
} else if (quant.min === 1 && quant.max === Infinity) {
569+
value = "+"
570+
} else if (quant.min === quant.max) {
571+
value = `{${quant.min}}`
572+
} else if (quant.max === Infinity) {
573+
value = `{${quant.min},}`
574+
} else {
575+
value = `{${quant.min},${quant.max}}`
576+
}
577+
578+
if (!quant.greedy) {
579+
return `${value}?`
580+
}
581+
return value
582+
}
583+
542584
/* eslint-disable complexity -- X( */
543585
/**
544586
* Check the siblings to see if the regex doesn't change when unwrapped.

‎lib/utils/rules.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { RuleModule } from "../types"
2+
import confusingQuantifier from "../rules/confusing-quantifier"
23
import letterCase from "../rules/letter-case"
34
import matchAny from "../rules/match-any"
45
import negation from "../rules/negation"
@@ -37,6 +38,7 @@ import preferUnicodeCodepointEscapes from "../rules/prefer-unicode-codepoint-esc
3738
import preferW from "../rules/prefer-w"
3839

3940
export const rules = [
41+
confusingQuantifier,
4042
letterCase,
4143
matchAny,
4244
negation,

‎tests/lib/rules/confusing-quantifier.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { RuleTester } from "eslint"
2+
import rule from "../../../lib/rules/confusing-quantifier"
3+
4+
const tester = new RuleTester({
5+
parserOptions: {
6+
ecmaVersion: 2020,
7+
sourceType: "module",
8+
},
9+
})
10+
11+
tester.run("confusing-quantifier", rule as any, {
12+
valid: [
13+
String.raw`/a+/`,
14+
String.raw`/a?/`,
15+
String.raw`/(a|b?)*/`,
16+
String.raw`/(a?){0,3}/`,
17+
String.raw`/(a|\b)+/`,
18+
],
19+
invalid: [
20+
{
21+
code: String.raw`/(a?){5}/`,
22+
errors: [
23+
{
24+
message:
25+
"This quantifier is confusing because its minimum is 5 but it can match the empty string. Maybe replace it with `{0,5}` to reflect that it can match the empty string?",
26+
line: 1,
27+
column: 6,
28+
},
29+
],
30+
},
31+
{
32+
code: String.raw`/(?:a?b*|c+){4}/`,
33+
errors: [
34+
{
35+
message:
36+
"This quantifier is confusing because its minimum is 4 but it can match the empty string. Maybe replace it with `{0,4}` to reflect that it can match the empty string?",
37+
line: 1,
38+
column: 13,
39+
},
40+
],
41+
},
42+
],
43+
})

‎tests/lib/utils/rules.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ describe("Check if the strict of all rules is correct", () => {
6363
it(messageId, () => {
6464
const message = rule.meta.messages[messageId]
6565
assert.ok(
66-
message.endsWith(".") || message.endsWith("}}"),
66+
message.endsWith(".") ||
67+
message.endsWith("?") ||
68+
message.endsWith("}}"),
6769
"Doesn't end with a dot.",
6870
)
6971
})

0 commit comments

Comments
 (0)