Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to allow only partial use to the allowTop option of regexp/no-useless-non-capturing-group rule. #215

Merged
merged 4 commits into from
Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 67 additions & 22 deletions docs/rules/no-useless-non-capturing-group.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ This rule reports unnecessary non-capturing group
/* eslint regexp/no-useless-non-capturing-group: "error" */

/* ✓ GOOD */
var foo = /(?:abcd)?/
var foo = /a(?:ab|cd)/
var foo = /(?:abcd)?/.test(str)
var foo = /a(?:ab|cd)/.test(str)

/* ✗ BAD */
var foo = /(?:ab|cd)/
var foo = /(?:abcd)/
var foo = /(?:[a-d])/
var foo = /(?:[a-d])|e/
var foo = /(?:a|(?:b|c)|d)/
var foo = /(?:ab|cd)/.test(str)
var foo = /(?:abcd)/.test(str)
var foo = /(?:[a-d])/.test(str)
var foo = /(?:[a-d])|e/.test(str)
var foo = /(?:a|(?:b|c)|d)/.test(str)
```

</eslint-code-block>
Expand All @@ -39,33 +39,78 @@ var foo = /(?:a|(?:b|c)|d)/
```json5
{
"regexp/no-useless-non-capturing-group": ["error", {
"allowTop": true
"allowTop": "partial" // or "always" or "never"
}]
}
```

- `"allowTop"`:
Whether a top-level non-capturing group is allowed. Defaults to `false`.
Whether a top-level non-capturing group is allowed. Defaults to `"partial"`.

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.
- `"partial"`:
Allows top-level non-capturing groups of patterns used as strings via `.source`.

<eslint-code-block fix>
<eslint-code-block fix>

```js
/* eslint regexp/no-useless-non-capturing-group: ["error", { allowTop: true }] */
```js
/* eslint regexp/no-useless-non-capturing-group: ["error", { allowTop: "partial" }] */

/* ✓ GOOD */
var foo = /(?:abcd)/
var foo = /(?:ab|cd)/
var foo = /(?:abcd)/
var foo = /(?:[a-d])/
/* ✓ GOOD */
var foo = /(?:ab|cd)/;
var bar = /(?:ab|cd)/; // We still don't know how it will be used.

/* ✗ BAD */
var foo = /(?:[a-d])|e/
var foo = /(?:a|(?:b|c)|d)/
```
/* ✗ BAD */
/(?:ab|cd)/.test(str);

</eslint-code-block>
/*-------*/
var baz = new RexExp(foo.source + 'e');
baz.test(str);
```

</eslint-code-block>

- `"always"`:
Always allow top-level non-capturing groups.

<eslint-code-block fix>

```js
/* eslint regexp/no-useless-non-capturing-group: ["error", { allowTop: "always" }] */

/* ✓ GOOD */
var foo = /(?:abcd)/.test(str)
var foo = /(?:ab|cd)/.test(str)
var foo = /(?:abcd)/.test(str)
var foo = /(?:[a-d])/.test(str)

/* ✗ BAD */
var foo = /(?:[a-d])|e/.test(str)
var foo = /(?:a|(?:b|c)|d)/.test(str)
```

</eslint-code-block>

- `"never"`:
Never allow top-level non-capturing groups.

<eslint-code-block fix>

```js
/* eslint regexp/no-useless-non-capturing-group: ["error", { allowTop: "never" }] */

/* ✗ BAD */
var foo = /(?:ab|cd)/;
var bar = /(?:ab|cd)/;

/(?:ab|cd)/.test(str);

/*-------*/
var baz = new RexExp(foo.source + 'e');
baz.test(str);
```

</eslint-code-block>

## :rocket: Version

Expand Down
48 changes: 44 additions & 4 deletions lib/rules/no-useless-non-capturing-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Group } from "regexpp/ast"
import type { RegExpVisitor } from "regexpp/visitor"
import type { RegExpContext } from "../utils"
import { canUnwrapped, createRule, defineRegexpVisitor } from "../utils"
import { UsageOfPattern } from "../utils/get-usage-of-pattern"

/**
* Returns whether the given group is the top-level group of its pattern.
Expand Down Expand Up @@ -36,7 +37,15 @@ export default createRule("no-useless-non-capturing-group", {
{
type: "object",
properties: {
allowTop: { type: "boolean" },
allowTop: {
anyOf: [
{
// backward compatibility
type: "boolean",
},
{ enum: ["always", "never", "partial"] },
],
},
},
additionalProperties: false,
},
Expand All @@ -47,7 +56,12 @@ export default createRule("no-useless-non-capturing-group", {
type: "suggestion", // "problem",
},
create(context) {
const allowTop = context.options[0]?.allowTop ?? false
const allowTop: "always" | "never" | "partial" =
context.options[0]?.allowTop === true
? "always"
: context.options[0]?.allowTop === false
? "never"
: context.options[0]?.allowTop ?? "partial"

/**
* Create visitor
Expand All @@ -56,10 +70,25 @@ export default createRule("no-useless-non-capturing-group", {
node,
getRegexpLocation,
fixReplaceNode,
getUsageOfPattern,
}: RegExpContext): RegExpVisitor.Handlers {
let isIgnored: (gNode: Group) => boolean
if (allowTop === "always") {
isIgnored = isTopLevel
} else if (allowTop === "partial") {
if (getUsageOfPattern() !== UsageOfPattern.whole) {
isIgnored = isTopLevel
} else {
isIgnored = () => false
}
} else {
// allowTop === "never"
isIgnored = () => false
}

return {
onGroupEnter(gNode) {
if (allowTop && isTopLevel(gNode)) {
if (isIgnored(gNode)) {
return
}

Expand Down Expand Up @@ -98,7 +127,18 @@ export default createRule("no-useless-non-capturing-group", {
node,
loc: getRegexpLocation(gNode),
messageId: "unexpected",
fix: fixReplaceNode(gNode, gNode.raw.slice(3, -1)),
fix: fixReplaceNode(gNode, () => {
if (
allowTop === "never" &&
isTopLevel(gNode) &&
getUsageOfPattern() !== UsageOfPattern.whole
) {
// Top-level group and potentially partially used patterns
// do not autofix because they can cause side effects.
return null
}
return gNode.raw.slice(3, -1)
}),
})
},
}
Expand Down
Loading