Repository: dubzzz/fast-check
Pull request #1822 (2021-05-04 07:05:02)

🐛 `.map` should check `canGenerate` on source before calling `shrink`
<!-- Why is this PR for? -->
<!-- Describe the reason why you opened this PR or give a link towards the associated issue. -->

## In a nutshell

❌ New feature
✔️ Fix an issue
❌ Documentation improvement
❌ Other: *please explain*

(✔️: yes, ❌: no)

## Potential impacts

None - Not release

codesandbox[bot]: This pull request is automatically built and testable in [CodeSandbox](https://codesandbox.io).

  To see build info of the built libraries, click [here](https://ci.codesandbox.io/status/dubzzz/fast-check/pr/1822/builds/126458) or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f5c906240220a1076974042b20e474cac2d12975:

|Sandbox| Source |
|--|--|
|[Vanilla](https://codesandbox.io/s/vanilla-8yeqp)| Configuration |

codecov[bot]: # [Codecov](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN) Report
> Merging [#1822](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN) (f5c9062) into [main](https://codecov.io/gh/dubzzz/fast-check/commit/2bd2302064055e2f585ff0b84799d065510fdbdd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN) (2bd2302) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/dubzzz/fast-check/pull/1822/graphs/tree.svg?width=650&height=150&src=pr&token=fRi2fJN730&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN)](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN)

```diff
@@           Coverage Diff           @@
##             main    #1822   +/-   ##
=======================================
  Coverage   97.26%   97.27%           
=======================================
  Files         139      139           
  Lines        3772     3774    +2     
  Branches      740      741    +1     
=======================================
+ Hits         3669     3671    +2     
  Misses         99       99           
  Partials        4        4           
```

| Flag | Coverage Δ | |
|---|---|---|
| unit-tests | `97.27% <100.00%> (+<0.01%)` | :arrow_up: |
| unit-tests-10.x-Linux | `97.27% <100.00%> (+<0.01%)` | :arrow_up: |
| unit-tests-12.x-Linux | `?` | |
| unit-tests-14.x-Linux | `?` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN) | Coverage Δ | |
|---|---|---|
| [src/check/arbitrary/definition/NextArbitrary.ts](https://codecov.io/gh/dubzzz/fast-check/pull/1822/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN#diff-c3JjL2NoZWNrL2FyYml0cmFyeS9kZWZpbml0aW9uL05leHRBcmJpdHJhcnkudHM=) | `96.89% <100.00%> (+0.04%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN). Last update [2bd2302...f5c9062](https://codecov.io/gh/dubzzz/fast-check/pull/1822?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Nicolas+DUBIEN).


Patch:
diff --git a/src/check/arbitrary/definition/NextArbitrary.ts b/src/check/arbitrary/definition/NextArbitrary.ts
index eb77076d..ee1fd43c 100644
--- a/src/check/arbitrary/definition/NextArbitrary.ts
+++ b/src/check/arbitrary/definition/NextArbitrary.ts
@@ -288,9 +288,20 @@ class MapArbitrary<T, U> extends NextArbitrary<U> {
       return this.arb.shrink(context.originalValue, context.originalContext).map(this.bindValueMapper);
     }
     if (this.unmapper !== undefined) {
-      // WARNING: canGenerate must have been called first
-      //          shrink should only be called for safe values
-      return this.arb.shrink(this.unmapper(value)).map(this.bindValueMapper);
+      // As `shrink` might be called with correct values where the unmapper has sevaral choices even
+      // entries looking correct could be invalid. If the user has not called `canGenerate` first, we
+      // might easily fall in such cases.
+      //
+      // Let's imagine: `mapper = (arr: string[]) => arr.join(" ")`
+      // >  mapper(["Hello", "how are", "you?"]) = "Hello how are you?"
+      // Unfortunately our unmapper might not unmap it correctly as several inputs give the same output.
+      // In order to be able to execute `shrink` safely on the unmapped value, we also need to check the
+      // unmapped value is correct for the source arbitrary. In the example above, we might imagine that
+      // our source arbitrary can shrink ["Hello", "how are", "you?"] but not ["Hello", "how", "are", "you?"].
+      const unmapped = this.unmapper(value);
+      if (this.arb.canGenerate(unmapped)) {
+        return this.arb.shrink(unmapped).map(this.bindValueMapper);
+      }
     }
     return Stream.nil();
   }


Test patch:
diff --git a/test/unit/check/arbitrary/definition/NextArbitrary.utest.spec.ts b/test/unit/check/arbitrary/definition/NextArbitrary.utest.spec.ts
index 9a46e176..1c8f0658 100644
--- a/test/unit/check/arbitrary/definition/NextArbitrary.utest.spec.ts
+++ b/test/unit/check/arbitrary/definition/NextArbitrary.utest.spec.ts
@@ -388,6 +388,7 @@ describe('NextArbitrary', () => {
       // Assert
       expect([...shrinks]).toHaveLength(0);
       expect(shrink).not.toHaveBeenCalled();
+      expect(canGenerate).not.toHaveBeenCalled();
     });
 
     it.each`
@@ -457,7 +458,7 @@ describe('NextArbitrary', () => {
         new NextValue('tutu')
       );
       const generate = jest.fn();
-      const canGenerate = jest.fn();
+      const canGenerate = jest.fn().mockReturnValue(true);
       const shrink = jest.fn().mockReturnValueOnce(expectedStreamValuesFromSource);
       const originalValue = Symbol();
       const unmapperOutput = 'tata';
@@ -476,9 +477,68 @@ describe('NextArbitrary', () => {
       expect(shrinks.map((s) => s.value)).toEqual([Symbol.for('titi'), Symbol.for('toto'), Symbol.for('tutu')]);
       expect(unmapper).toHaveBeenCalledTimes(1);
       expect(unmapper).toHaveBeenCalledWith(originalValue);
+      expect(canGenerate).toHaveBeenCalledTimes(1);
+      expect(canGenerate).toHaveBeenCalledWith(unmapperOutput);
       expect(shrink).toHaveBeenCalledTimes(1);
       expect(shrink).toHaveBeenCalledWith(unmapperOutput);
     });
+
+    it('should return an empty stream if source arbitrary tells it cannot generate the value', () => {
+      // Arrange
+      const expectedStreamValuesFromSource = Stream.of(
+        new NextValue('titi'),
+        new NextValue('toto'),
+        new NextValue('tutu')
+      );
+      const generate = jest.fn();
+      const canGenerate = jest.fn().mockReturnValue(false);
+      const shrink = jest.fn().mockReturnValueOnce(expectedStreamValuesFromSource);
+      const originalValue = Symbol();
+      const unmapperOutput = 'tata';
+      const unmapper = jest.fn().mockReturnValue('tata');
+      class MyNextArbitrary extends NextArbitrary<any> {
+        generate = generate;
+        canGenerate = (canGenerate as any) as (value: unknown) => value is any;
+        shrink = shrink;
+      }
+
+      // Act
+      const arb = new MyNextArbitrary().map((tag) => Symbol.for(tag), unmapper);
+      const shrinks = [...arb.shrink(originalValue)];
+
+      // Assert
+      expect(shrinks.map((s) => s.value)).toEqual([]);
+      expect(unmapper).toHaveBeenCalledTimes(1);
+      expect(unmapper).toHaveBeenCalledWith(originalValue);
+      expect(canGenerate).toHaveBeenCalledTimes(1);
+      expect(canGenerate).toHaveBeenCalledWith(unmapperOutput);
+      expect(shrink).not.toHaveBeenCalled();
+    });
+
+    it('should not catch exceptions emit by unmapper if unmapper throws (shrink should be called with supported values)', () => {
+      // Arrange
+      const expectedStreamValuesFromSource = Stream.of(
+        new NextValue('titi'),
+        new NextValue('toto'),
+        new NextValue('tutu')
+      );
+      const generate = jest.fn();
+      const canGenerate = jest.fn().mockReturnValue(false);
+      const shrink = jest.fn().mockReturnValueOnce(expectedStreamValuesFromSource);
+      const originalValue = Symbol();
+      const unmapper = jest.fn().mockImplementation(() => {
+        throw new Error('Unsupported value, cannot unmap');
+      });
+      class MyNextArbitrary extends NextArbitrary<any> {
+        generate = generate;
+        canGenerate = (canGenerate as any) as (value: unknown) => value is any;
+        shrink = shrink;
+      }
+
+      // Act / Assert
+      const arb = new MyNextArbitrary().map((tag) => Symbol.for(tag), unmapper);
+      expect(() => arb.shrink(originalValue)).toThrowError();
+    });
   });
 
   describe('chain', () => {