Skip to content

fix: accept start as keyword arg in enumerate builtin#395

Open
MukundaKatta wants to merge 1 commit into
pydantic:mainfrom
MukundaKatta:fix/enumerate-keyword-start
Open

fix: accept start as keyword arg in enumerate builtin#395
MukundaKatta wants to merge 1 commit into
pydantic:mainfrom
MukundaKatta:fix/enumerate-keyword-start

Conversation

@MukundaKatta

Copy link
Copy Markdown

Closes #394.

The enumerate(iterable, start) shim only accepted start positionally, but stdlib enumerate allows start as a keyword (enumerate(iter, start=0)). Switched the parser to walk kwargs so start= works as a keyword while preserving the existing positional and arity errors, mirroring the kwarg-handling pattern used in zip and sorted.

Test added in crates/monty/test_cases/builtin__more_iter_funcs.py covering the issue's reproducer and an unknown-kwarg rejection.

Matches CPython's `enumerate(iterable, start=0)` signature so that `start`
works as both a positional and a keyword argument. Closes pydantic#394.
@codspeed-hq

codspeed-hq Bot commented Apr 25, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 15 skipped benchmarks1


Comparing MukundaKatta:fix/enumerate-keyword-start (92aa59d) with main (33e47c6)

Open in CodSpeed

Footnotes

  1. 15 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +24 to 27
let (iterable, start) = parse_enumerate_args(args, vm)?;
let iter = MontyIter::new(iterable, vm)?;
defer_drop_mut!(iter, vm);
defer_drop!(start, vm);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Reference count leak / panic: start not protected by defer_drop! before fallible MontyIter::new call

If MontyIter::new(iterable, vm)? fails (e.g. enumerate(42, [1, 2, 3]) where 42 is not iterable), the ? operator causes an early return before defer_drop!(start, vm) on line 27 is reached. When start holds a Value::Ref (heap-allocated), this means drop_with_heap is never called — with the memory-model-checks feature (used in CI), this panics; without it, it silently leaks the reference count.

The correct pattern — used by analogous builtins sum (crates/monty/src/builtins/sum.rs:23) and filter (crates/monty/src/builtins/filter.rs:35) — is to register the secondary value with defer_drop! before the fallible MontyIter::new call.

Suggested change
let (iterable, start) = parse_enumerate_args(args, vm)?;
let iter = MontyIter::new(iterable, vm)?;
defer_drop_mut!(iter, vm);
defer_drop!(start, vm);
let (iterable, start) = parse_enumerate_args(args, vm)?;
defer_drop!(start, vm);
let iter = MontyIter::new(iterable, vm)?;
defer_drop_mut!(iter, vm);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@samuelcolvin

Copy link
Copy Markdown
Member

@MukundaKatta please can you fix the review and solve conflicts.

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.

enumerate builtin: start parameter can only be passed as positional argument, not keyword argument

2 participants