diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..3de6915 --- /dev/null +++ b/PR.md @@ -0,0 +1,91 @@ +# Fix `fill` growing the array when `start` comes after `end` + +## Problem + +`pydash.fill(array, value, start, end)` is documented to fill elements "from +`start` up to, but not including, `end`" and to mutate the array in place. When +`start` is greater than the effective `end`, it instead **grows the array** and +duplicates elements: + +```python +>>> import pydash +>>> pydash.fill([1, 2, 3, 4], "x", 4, 1) +[1, 2, 3, 4, 3, 2, 3] # expected [1, 2, 3, 4] (empty fill range) +>>> pydash.fill([1, 2, 3], "x", -1, -2) +[1, 2, 3, 3] # expected [1, 2, 3] +``` + +An empty fill range should leave the array unchanged (this also matches +lodash's `_.fill`, the API pydash mirrors). + +## Root cause + +`src/pydash/arrays.py`, in `fill`: + +```python +if end is None: + end = len(array) +else: + end = min(end, len(array)) + +array[:] = array[:start] + [value] * len(array[start:end]) + array[end:] +``` + +The array is rebuilt from three slices: the head `array[:start]`, the filled +middle, and the tail `array[end:]`. This only works when +`0 <= start <= end <= len(array)`. When `start > end`, `array[:start]` and +`array[end:]` overlap, so the shared elements appear twice and the array grows. +The fill range length is computed from `array[start:end]`, which is empty in +that case, so no value is actually written either. + +## Fix + +Normalize `start` and `end` into the `[0, length]` range (supporting negative +indices as slicing does) and fill in place with an index loop, keeping the +array length fixed: + +```python +length = len(array) +if end is None: + end = length +start = max(length + start, 0) if start < 0 else min(start, length) +end = max(length + end, 0) if end < 0 else min(end, length) +for index in range(start, end): + array[index] = value +return array +``` + +When `start >= end` the loop simply does nothing, leaving the array untouched. + +## Test + +Extended `test_fill` in `tests/test_arrays.py` with reversed and negative spans: + +```python +(([1, 2, 3, 4], "x", 4, 1), [1, 2, 3, 4]), +(([1, 2, 3, 4, 5], 0, 3, 2), [1, 2, 3, 4, 5]), +(([1, 2, 3, 4, 5], 0, -2), [1, 2, 3, 0, 0]), +(([1, 2, 3, 4, 5], 0, -2, -1), [1, 2, 3, 0, 5]), +(([1, 2, 3], "x", -1, -2), [1, 2, 3]), +``` + +`test_fill` also asserts the passed array is mutated in place, which the fix +preserves. + +## Gate + +- `python -m pytest tests/test_arrays.py -k test_fill` -> 11 passed +- `python -m pytest tests/ --ignore=tests/pytest_mypy_testing` -> 1920 passed + +Counterfactual (fix reverted, tests kept): the reversed-span cases fail, e.g. +`assert [1, 2, 3, 4, 3, 2, 3] == [1, 2, 3, 4]` (the array was grown). + +## Flags for the account owner + +- The `tests/pytest_mypy_testing/` suite fails locally with `NameError` (the + `reveal_type` typing plugin is not resolvable in a bare `pip install -e .` + environment). Pre-existing and unrelated to this change. +- Related but not fixed here: `pad_start` pads multi-character `chars` using the + suffix of the repeated pattern (e.g. `pad_start("abc", 4, "-.")` -> `".abc"`), + whereas `pad_end` and lodash use the pattern prefix (`"-abc"`). Left out to + keep this change to a single concern. diff --git a/src/pydash/arrays.py b/src/pydash/arrays.py index 15fda4b..75a227f 100644 --- a/src/pydash/arrays.py +++ b/src/pydash/arrays.py @@ -531,13 +531,21 @@ def fill( .. versionadded:: 3.1.0 """ - if end is None: - end = len(array) - else: - end = min(end, len(array)) + length = len(array) - # Use this style of assignment so that `array` is mutated. - array[:] = array[:start] + [value] * len(array[start:end]) + array[end:] # type: ignore + if end is None: + end = length + + # Normalize indices to the ``[0, length]`` range (supporting negatives the + # same way slicing does). Filling in place keeps the array length fixed: + # the previous ``array[:start] + fill + array[end:]`` reconstruction + # duplicated the overlap and grew the array whenever ``start`` came after + # ``end`` (e.g. ``fill([1, 2, 3, 4], "x", 4, 1)``). + start = max(length + start, 0) if start < 0 else min(start, length) + end = max(length + end, 0) if end < 0 else min(end, length) + + for index in range(start, end): + array[index] = value # type: ignore return array # type: ignore diff --git a/tests/test_arrays.py b/tests/test_arrays.py index ff4d668..dedd309 100644 --- a/tests/test_arrays.py +++ b/tests/test_arrays.py @@ -158,6 +158,14 @@ def test_duplicates(case, expected): (([1, 2, 3, 4, 5], 0, 0, 5), [0, 0, 0, 0, 0]), (([1, 2, 3, 4, 5], 0, 0, 8), [0, 0, 0, 0, 0]), (([1, 2, 3, 4, 5], 0, 0, -1), [0, 0, 0, 0, 5]), + # start after end must be a no-op and must not grow the array. + (([1, 2, 3, 4], "x", 4, 1), [1, 2, 3, 4]), + (([1, 2, 3, 4, 5], 0, 3, 2), [1, 2, 3, 4, 5]), + # negative start supported the same way slicing does. + (([1, 2, 3, 4, 5], 0, -2), [1, 2, 3, 0, 0]), + (([1, 2, 3, 4, 5], 0, -2, -1), [1, 2, 3, 0, 5]), + # reversed negative span is a no-op, not an array-growing overlap. + (([1, 2, 3], "x", -1, -2), [1, 2, 3]), ], ) def test_fill(case, expected):