Skip to content

Add support for v flag to regexp/no-dupe-disjunctions #612

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

Merged
merged 2 commits into from
Sep 22, 2023
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
5 changes: 5 additions & 0 deletions .changeset/beige-fireants-dress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-regexp": patch
---

Add support for `v` flag to `regexp/no-dupe-disjunctions`
61 changes: 46 additions & 15 deletions lib/rules/no-dupe-disjunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { RegExpVisitor } from "@eslint-community/regexpp/visitor"
import type {
Alternative,
CapturingGroup,
CharacterClassElement,
Group,
LookaroundAssertion,
Node,
Expand All @@ -15,6 +16,7 @@ import {
fixRemoveCharacterClassElement,
fixRemoveAlternative,
assertValidFlags,
fixRemoveStringAlternative,
} from "../utils"
import { getParser, isCoveredNode, isEqualNodes } from "../utils/regexp-ast"
import type { Expression, FiniteAutomaton, NoParent, ReadonlyNFA } from "refa"
Expand Down Expand Up @@ -221,21 +223,46 @@ function* iterateNestedAlternatives(

if (e.type === "CharacterClass" && !e.negate) {
const nested: NestedAlternative[] = []
for (const charElement of e.elements) {
if (charElement.type === "CharacterClassRange") {
const min = charElement.min
const max = charElement.max
if (min.value === max.value) {

// eslint-disable-next-line func-style -- x
const addToNested = (charElement: CharacterClassElement) => {
switch (charElement.type) {
case "CharacterClassRange": {
const min = charElement.min
const max = charElement.max
if (min.value === max.value) {
nested.push(charElement)
} else if (min.value + 1 === max.value) {
nested.push(min, max)
} else {
nested.push(charElement, min, max)
}
break
}
case "ClassStringDisjunction": {
nested.push(...charElement.alternatives)
break
}
case "CharacterClass": {
if (!charElement.negate) {
charElement.elements.forEach(addToNested)
} else {
nested.push(charElement)
}
break
}
case "Character":
case "CharacterSet":
case "ExpressionCharacterClass": {
nested.push(charElement)
} else if (min.value + 1 === max.value) {
nested.push(min, max)
} else {
nested.push(charElement, min, max)
break
}
} else {
nested.push(charElement)
default:
throw assertNever(charElement)
}
}
e.elements.forEach(addToNested)

if (nested.length > 1) yield* nested
}
}
Expand Down Expand Up @@ -825,7 +852,7 @@ function deduplicateResults(
}

function mentionNested(nested: NestedAlternative): string {
if (nested.type === "Alternative") {
if (nested.type === "Alternative" || nested.type === "StringAlternative") {
return mention(nested)
}
return mentionChar(nested)
Expand All @@ -842,9 +869,15 @@ function fixRemoveNestedAlternative(
case "Alternative":
return fixRemoveAlternative(context, alternative)

case "StringAlternative":
return fixRemoveStringAlternative(context, alternative)

case "Character":
case "CharacterClassRange":
case "CharacterSet": {
case "CharacterSet":
case "CharacterClass":
case "ExpressionCharacterClass":
case "ClassStringDisjunction": {
if (alternative.parent.type !== "CharacterClass") {
// This isn't supposed to happen. We can't just remove the only
// alternative of its parent
Expand All @@ -855,8 +888,6 @@ function fixRemoveNestedAlternative(
}

default:
// FIXME: TS Error
// @ts-expect-error -- FIXME
throw assertNever(alternative)
}
}
Expand Down
38 changes: 32 additions & 6 deletions lib/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type {
Node,
Pattern,
Quantifier,
StringAlternative,
} from "@eslint-community/regexpp/ast"
import { RegExpParser, visitRegExpAST } from "@eslint-community/regexpp"
import {
Expand Down Expand Up @@ -1206,14 +1207,13 @@ export function fixRemoveCharacterClassElement(
return null
}

const elements: CharacterClassElement[] = cc.elements
const elementIndex = elements.indexOf(element)

const elementBefore: CharacterClassElement | undefined =
// FIXME: TS Error
// @ts-expect-error -- FIXME
cc.elements[cc.elements.indexOf(element) - 1]
cc.elements[elementIndex - 1]
const elementAfter: CharacterClassElement | undefined =
// FIXME: TS Error
// @ts-expect-error -- FIXME
cc.elements[cc.elements.indexOf(element) + 1]
cc.elements[elementIndex + 1]

if (
elementBefore &&
Expand Down Expand Up @@ -1275,6 +1275,32 @@ export function fixRemoveAlternative(
})
}

export function fixRemoveStringAlternative(
context: RegExpContext,
alternative: StringAlternative,
): (fixer: Rule.RuleFixer) => Rule.Fix | null {
const { parent } = alternative
if (parent.alternatives.length === 1) {
// We have to remove the string disjunction instead.
// We replace it with `[]` to avoid situations like `[&\q{a}&]` -> `[&&]`
return context.fixReplaceNode(parent, "[]")
}

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.
*/
Expand Down
104 changes: 81 additions & 23 deletions lib/utils/partial-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,15 @@ import { JS } from "refa"
import type { AST } from "@eslint-community/regexpp"
import { assertNever } from "./util"

export type NestedAlternative = AST.Alternative | AST.CharacterClassElement
export type NestedAlternative =
| AST.Alternative
| AST.CharacterClassElement
| AST.StringAlternative

class Context {
/**
* The ancestor nodes of the alternative + the alternative itself.
*/
public readonly ancestors: ReadonlySet<AST.Node>

public readonly alternative: NestedAlternative
Expand All @@ -27,15 +33,14 @@ class Context {
}
}

type Parsable = JS.ParsableElement | AST.CharacterClassElement

export class PartialParser {
private readonly parser: JS.Parser

private readonly options: JS.ParseOptions

private readonly nativeCache = new WeakMap<
JS.ParsableElement,
NoParent<Element>
>()
private readonly nativeCache = new WeakMap<Parsable, NoParent<Element>>()

public constructor(parser: JS.Parser, options: JS.ParseOptions = {}) {
this.parser = parser
Expand Down Expand Up @@ -117,8 +122,34 @@ export class PartialParser {
}
}

private parseStringAlternatives(
alternatives: readonly AST.StringAlternative[],
context: Context,
): NoParent<Concatenation>[] {
const ancestor = alternatives.find((a) => context.ancestors.has(a))
if (ancestor) {
return [this.parseStringAlternative(ancestor)]
}

return alternatives.map((a) => this.parseStringAlternative(a))
}

private parseStringAlternative(
alternative: AST.StringAlternative,
): NoParent<Concatenation> {
return {
type: "Concatenation",
elements: alternative.elements.map((e) =>
this.nativeParseElement(e),
),
}
}

private parseElement(
element: AST.Element,
element:
| AST.Element
| AST.CharacterClassRange
| AST.ClassStringDisjunction,
context: Context,
): NoParent<Element> {
if (!context.ancestors.has(element)) {
Expand All @@ -130,11 +161,29 @@ export class PartialParser {
case "Backreference":
case "Character":
case "CharacterSet":
case "ExpressionCharacterClass":
return this.nativeParseElement(element)

case "CharacterClassRange":
if (context.alternative === element.min) {
return this.nativeParseElement(context.alternative)
} else if (context.alternative === element.max) {
return this.nativeParseElement(context.alternative)
}
return this.nativeParseElement(element)

case "CharacterClass":
return this.parseCharacterClass(element, context)

case "ClassStringDisjunction":
return {
type: "Alternation",
alternatives: this.parseStringAlternatives(
element.alternatives,
context,
),
}

case "Group":
case "CapturingGroup":
return {
Expand Down Expand Up @@ -175,8 +224,6 @@ export class PartialParser {
}

default:
// FIXME: TS Error
// @ts-expect-error -- FIXME
throw assertNever(element)
}
}
Expand All @@ -193,22 +240,17 @@ export class PartialParser {
return this.nativeParseElement(cc)
}

if (context.alternative.type === "CharacterClassRange") {
const range: CharRange = {
min: context.alternative.min.value,
max: context.alternative.max.value,
}
return {
type: "CharacterClass",
characters: JS.createCharSet([range], this.parser.flags),
for (const e of cc.elements) {
if (context.ancestors.has(e)) {
return this.parseElement(e, context)
}
}
// FIXME: TS Error
// @ts-expect-error -- FIXME
return this.nativeParseElement(context.alternative)

// this means that cc === context.alternative
return this.nativeParseElement(cc)
}

private nativeParseElement(element: JS.ParsableElement): NoParent<Element> {
private nativeParseElement(element: Parsable): NoParent<Element> {
let cached = this.nativeCache.get(element)
if (!cached) {
cached = this.nativeParseElementUncached(element)
Expand All @@ -217,9 +259,25 @@ export class PartialParser {
return cached
}

private nativeParseElementUncached(
element: JS.ParsableElement,
): NoParent<Element> {
private nativeParseElementUncached(element: Parsable): NoParent<Element> {
if (element.type === "CharacterClassRange") {
const range: CharRange = {
min: element.min.value,
max: element.max.value,
}
return {
type: "CharacterClass",
characters: JS.createCharSet([range], this.parser.flags),
}
} else if (element.type === "ClassStringDisjunction") {
return {
type: "Alternation",
alternatives: element.alternatives.map((a) =>
this.parseStringAlternative(a),
),
}
}

const { expression } = this.parser.parseElement(element, this.options)

if (expression.alternatives.length === 1) {
Expand Down
15 changes: 14 additions & 1 deletion tests/lib/rules/no-dupe-disjunctions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import rule from "../../../lib/rules/no-dupe-disjunctions"

const tester = new RuleTester({
parserOptions: {
ecmaVersion: 2020,
ecmaVersion: "latest",
sourceType: "module",
},
})
Expand All @@ -19,6 +19,7 @@ tester.run("no-dupe-disjunctions", rule as any, {
`/(?:js|json)?abc/`,
`/<("[^"]*"|'[^']*'|[^'">])*>/g`,
`/c+|[a-f]/`,
`/c+|[a-f]/v`,
String.raw`/b+(?:\w+|[+-]?\d+)/`,
String.raw`/\d*\.\d+_|\d+\.\d*_/`,
String.raw`/\d*\.\d+|\d+\.\d*/`,
Expand Down Expand Up @@ -149,6 +150,12 @@ tester.run("no-dupe-disjunctions", rule as any, {
"Unexpected duplicate alternative. This alternative can be removed.",
],
},
{
code: `/((?:ab|ba)|(?:ab|ba))/v`,
errors: [
"Unexpected duplicate alternative. This alternative can be removed.",
],
},
{
code: `/((?:ab|ba)|(?:ba|ab))/`,
errors: [
Expand Down Expand Up @@ -514,6 +521,12 @@ tester.run("no-dupe-disjunctions", rule as any, {
"Unexpected useless alternative. This alternative is a strict subset of 'A*' and can be removed.",
],
},
{
code: String.raw`/[\q{a|bb}]|bb/v`,
errors: [
"Unexpected useless alternative. This alternative is a strict subset of '[\\q{a|bb}]' and can be removed.",
],
},
{
// reportExponentialBacktracking: 'potential'
code: String.raw`
Expand Down