Skip to content

Fix tail(n, seq) returning whole sequence for n <= 0#627

Open
Sanjays2402 wants to merge 2 commits into
pytoolz:masterfrom
Sanjays2402:fix/tail-nonpositive-n
Open

Fix tail(n, seq) returning whole sequence for n <= 0#627
Sanjays2402 wants to merge 2 commits into
pytoolz:masterfrom
Sanjays2402:fix/tail-nonpositive-n

Conversation

@Sanjays2402

Copy link
Copy Markdown

Fixes #626

The bug

tail(n, seq) is implemented as seq[-n:] with a deque(seq, n) fallback for non-sliceable inputs. For n == 0, seq[-0:] is seq[0:] — the entire sequence — so tail(0, seq) returned everything instead of the last zero elements.

The two input paths also disagreed with each other:

>>> from toolz import tail
>>> tail(0, [10, 20, 30])            # sliceable -> seq[0:]
[10, 20, 30]                         # WRONG: whole sequence
>>> tuple(tail(0, iter([10, 20, 30])))  # non-sliceable -> deque(seq, 0)
()                                   # correct: empty

Negative n was inconsistent too — tail(-1, [10, 20, 30]) returned [20, 30] (seq[1:]), while tail(-1, iter([10, 20, 30])) raised ValueError: maxlen must be non-negative.

By contrast the sibling take(0, seq) already correctly returns empty.

Root cause

One line: seq[-n:] with n == 0 slices seq[0:] (the whole sequence, since -0 == 0). Negative n slices from the front and, on the fallback path, passes a negative maxlen to deque.

The fix

toolz/itertoolz.py — guard n <= 0 up front and return an empty tuple, matching both take(0, seq) and the value the deque fallback already produced:

 def tail(n, seq):
     ...
+    if n <= 0:
+        return ()
     try:
         return seq[-n:]
     except (TypeError, KeyError):
         return tuple(collections.deque(seq, n))

The n >= 1 slice path is untouched, so type-preserving behaviour is unchanged (str in → str out, tupletuple, listlist). last(seq) — which calls tail(1, seq)[0] — is unaffected because it always passes n == 1.

Before / after

call before after
tail(0, [10, 20, 30]) [10, 20, 30] ()
tuple(tail(0, iter([10, 20, 30]))) () ()
tail(-1, [10, 20, 30]) [20, 30] ()
tuple(tail(-1, iter([10, 20, 30]))) ValueError ()
tail(2, 'ABCDE') 'DE' 'DE' (unchanged)
tail(2, (3, 2, 1)) (2, 1) (2, 1) (unchanged)

Test

Extended test_tail with regression assertions covering n == 0 and n < 0 across both sliceable and non-sliceable inputs. Proven to guard the bug:

  • Stash the source fix, keep the test → test_tail FAILS: assert ['A', 'B', 'C', 'D', 'E'] == []
  • Restore the fix → test_tail PASSES

Full suite green, no regressions:

$ python -m pytest toolz/tests/ -q
181 passed in 0.22s

pycodestyle --ignore="E731,W503,W504,E402" toolz/itertoolz.py is clean.

Note: the same latent bug exists in cytoolz; happy to open a companion PR there if you'd like to keep the two implementations in parity.

tail(n, seq) is implemented as seq[-n:] with a deque(seq, n) fallback for
non-sliceable inputs. For n == 0, seq[-0:] is seq[0:] -- the entire
sequence -- so tail(0, seq) returned everything instead of the last zero
elements. Worse, the two input paths disagreed: sliceable inputs returned
the whole sequence while non-sliceable iterables hit deque(seq, 0) and
correctly returned empty. Negative n was inconsistent too (seq[1:] for a
list vs. ValueError: maxlen must be non-negative for an iterator).

Guard n <= 0 up front and return an empty tuple, matching both the
sibling take(0, seq) (which already returns empty) and the value the
deque fallback already produced. The n >= 1 slice path -- and its
type-preserving behaviour (str/tuple/list in, same type out) -- is
unchanged.

Add regression coverage in test_tail asserting empty and consistent
results across sliceable and non-sliceable inputs for n == 0 and n < 0.

Fixes pytoolz#626
@hunterhogan

Copy link
Copy Markdown

@Sanjays2402, tell your AI agent that the code is inefficient and wrong.

wrong

If the user passes a negative number for n, the function needs to throw an exception. return () hides erroneous input and/or an upstream bug in the user's flow.

inefficient

How often is n=0 passed to this function? It can't be more often than 1 out of 1 billion calls, but your AI wrote code that checks all 1 billion calls.

Z0Z_tools

	try:
		return seq[len(seq) - n: None]
	except (TypeError, KeyError):
		return tuple(deque(seq, n))

If an object doesn't have __len__, it is probably unordered (e.g., set) so the "tail" of the object is nonsense or doesn't have __getitem__ so the except branch would be activated without len(). I'm not claiming this is the optimal solution: I'm saying that nearly every if is a waste, but len() has the same utility as the slice operation.

Per maintainer feedback on the PR:

- A negative n now raises ValueError instead of silently returning empty,
  matching nth's contract and surfacing the caller's upstream bug rather
  than masking it.

- Replace the `if n <= 0: return ()` special-case with a single slice
  `seq[max(len(seq) - n, 0):]`. This handles n == 0 (empty tail) inline
  without a branch that runs on every call, while the max(..., 0) clamp
  keeps n > len(seq) returning the whole sequence (a plain
  seq[len(seq) - n:] would wrap to a negative index and truncate).

Tests updated to assert the raise-on-negative contract and add explicit
n > len coverage for both sliceable and iterator inputs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tail(0, seq) returns the whole sequence instead of empty (and is inconsistent across iterable types)

2 participants