Skip to content

Commit

Permalink
Improve pattern handling by merging multiple patterns into a union, c… (
Browse files Browse the repository at this point in the history
  • Loading branch information
gcanti authored Jan 14, 2025
1 parent b514200 commit d7dac48
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 2 deletions.
26 changes: 26 additions & 0 deletions .changeset/cool-parrots-draw.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
"effect": patch
---

Improve pattern handling by merging multiple patterns into a union, closes #4243.

Previously, the algorithm always prioritized the first pattern when multiple patterns were encountered.

This fix introduces a merging strategy that combines patterns into a union (e.g., `(?:${pattern1})|(?:${pattern2})`). By doing so, all patterns have an equal chance to generate values when using `FastCheck.stringMatching`.

**Example**

```ts
import { Arbitrary, FastCheck, Schema } from "effect"

// /^[^A-Z]*$/ (given by Lowercase) + /^0x[0-9a-f]{40}$/
const schema = Schema.Lowercase.pipe(Schema.pattern(/^0x[0-9a-f]{40}$/))

const arb = Arbitrary.make(schema)

// Before this fix, the first pattern would always dominate,
// making it impossible to generate values
const sample = FastCheck.sample(arb, { numRuns: 100 })

console.log(sample)
```
14 changes: 12 additions & 2 deletions packages/effect/src/Arbitrary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ export const toOp = (
return new Succeed((fc) => fc.constant(null).chain(() => get()(fc)))
}
case "Transformation":
return new Succeed(go(ast.to, ctx, path))
return toOp(ast.to, ctx, path)
}
}

Expand Down Expand Up @@ -577,6 +577,16 @@ const getOr = (a: boolean | undefined, b: boolean | undefined): boolean | undefi
return a === undefined ? b : b === undefined ? a : a || b
}

function mergePattern(pattern1: string | undefined, pattern2: string | undefined): string | undefined {
if (pattern1 === undefined) {
return pattern2
}
if (pattern2 === undefined) {
return pattern1
}
return `(?:${pattern1})|(?:${pattern2})`
}

const merge = (c1: Config, c2: Constraints | undefined): Config => {
if (c2) {
switch (c1._tag) {
Expand All @@ -585,7 +595,7 @@ const merge = (c1: Config, c2: Constraints | undefined): Config => {
return makeStringConstraints({
minLength: getMax(c1.constraints.minLength, c2.constraints.minLength),
maxLength: getMin(c1.constraints.maxLength, c2.constraints.maxLength),
pattern: c1.pattern ?? c2.pattern
pattern: mergePattern(c1.pattern, c2.pattern)
})
}
break
Expand Down
11 changes: 11 additions & 0 deletions packages/effect/test/Schema/Arbitrary/Arbitrary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,17 @@ details: Generating an Arbitrary for this schema requires at least one enum`)
expectConstraints(schema, Arbitrary.makeStringConstraints({ minLength: 1, pattern: regex.source }))
expectValidArbitrary(schema)
})

it("pattern + pattern", () => {
const regexp1 = /^[^A-Z]*$/
const regexp2 = /^0x[0-9a-f]{40}$/
const schema = S.String.pipe(S.pattern(regexp1), S.pattern(regexp2))
expectConstraints(
schema,
Arbitrary.makeStringConstraints({ pattern: `(?:${regexp1.source})|(?:${regexp2.source})` })
)
expectValidArbitrary(schema)
})
})

describe("number filters", () => {
Expand Down

0 comments on commit d7dac48

Please sign in to comment.