Repository navigation
feat(lint): cap comprehensions at one for and one if clause (LIT014) - #42650
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ning it Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ension Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
…number to LIT014)
…hension_clause_limit
9f5c9b5 to
b31ae1d
Compare
|
bugbot run |
…rehension-ok markers Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d47f08c. Configure here.
…_iterable LIT014 (#42650) caps a comprehension at one for and one if clause. The four nested walks in the helper now chain their iterables instead, with the same order and results.
…er (#42629) * ci: warn on SQL IN lists with no written bound Postgres caps a prepared statement at 32,767 bind parameters and a membership filter binds one per value, so an IN list built from table data breaks once the table outgrows the cap. That is how the budget reset job froze every due budget (LIT-7535, #40564). check_unbounded_in_lists.py reports every Prisma "in" / "not_in" filter whose value has no fixed size and every raw SQL literal that splices a list in after "IN (", unless the line carries "# bounded-ok: <reason>". It only warns for now: the output is the inventory for RCA action item AI-1, and it exits 0. * ci: decide a constant IN list by its module binding, not its casing An ALL_CAPS name imported or filled at runtime is as unbounded as any other, so a name now passes only when the module binds it once to a value of fixed size. Adds Final to the locals a loop does not forbid. * ci: only a frozen module value makes an IN list constant A module list bound once could still grow through append or extend, so a name now counts as fixed only when it is bound to a tuple, frozenset or constant. Trims the module docstring to what a reader needs. * ci: chunk Prisma IN lists with a shared helper and fail on new unbounded ones Add litellm.repositories.bounded_in: find_many_in, count_in, update_many_in and delete_many_in split a deduplicated value list into 5,000-value chunks, AND each chunk with the caller's where, run them in order (a transaction handle works) and combine the results. Writes take a required atomicity argument, and a where that already filters the chunked field is refused. check_unbounded_in_lists.py now fails CI on any finding missing from unbounded_in_baseline.txt and on any stale baseline entry, so the baseline only shrinks. Entries are keyed by path, enclosing scope, kind, field and occurrence, not line numbers. The helper module is exempt, a constant spread into a frozen tuple counts as fixed, and messages point at the helper for "in" and at an array parameter for "not_in" and raw SQL. A real-Postgres integration test shows a raw 40,000-value filter rejected for too many bind variables while the helpers handle it. * refactor: rename bounded_in to chunked_in and let callers pick a chunk size The helper module is litellm.repositories.chunked_in, and its unit and integration tests, the checker's exemption path and its finding messages follow the new name. The `# bounded-ok` marker is unchanged. find_many_in, count_in, update_many_in and delete_many_in take a keyword-only chunk_size, defaulting to IN_LIST_CHUNK_SIZE (5,000). A value below 1 or above MAX_IN_LIST_CHUNK_SIZE (30,000) raises ValueError before any query, which leaves the rest of the filter headroom under Postgres's 32,767 bind-parameter cap. * refactor: flatten chunked_in's stacked comprehensions with chain.from_iterable LIT014 (#42650) caps a comprehension at one for and one if clause. The four nested walks in the helper now chain their iterables instead, with the same order and results. * refactor: recover user details with find_many_in, sending chunks as lists _details_for_user_ids reads users through find_many_in instead of a raw "in" filter, so its lookup stays under the bind-parameter cap for any number of recovered keys. Up to 5,000 ids it still sends one find_many with the same where dict, and a PrismaError from any chunk is still logged and treated as no details. The helper now sends each chunk as a list, so a chunked filter equals the dict a hand-written call would send and a migrated call site's existing assertions keep passing. The site's baseline entry is gone. * ci: skip functional TypedDict field maps in the unbounded IN list check The dict passed as the field map of TypedDict("Name", {...}), or as its fields= keyword, names fields: an "in" or "notIn" key there is a type, not a filter. Only that dict is skipped, for TypedDict, typing.TypedDict and typing_extensions.TypedDict; a filter nested in a field value or passed to any other call is still reported. The two types/proxy/management_endpoints/team_endpoints.py entries leave the baseline, which is now 156. * fix: refuse an update_many_in whose data writes the chunked field Chunks run one after another, so an update that sets the chunked field can move a row into a later chunk, which updates it again and counts it twice: values ["old", "new"] with chunk_size=1 and data={"id": "new"} does exactly that. update_many_in now raises ChunkedFieldWriteError before any query when data has the chunked field as a top-level key, in any form, including Prisma operators such as {"set": ...}. * docs: cut the unbounded IN list checker's docstring to what it flags and how to clear it It now says what is reported, the three ways to clear a finding, and how the baseline and --update-baseline work, in 11 lines. The per-shape detail lives in the tests. * ci: key an unbounded IN list finding by its filtered expression too A baseline key of path, scope, kind, field and occurrence let a PR delete a baselined filter and add a different unbounded one on the same field in the same function, and the new one took over the old key. The key now also carries the filtered expression's source, whitespace-normalized (the Prisma value, or a raw-SQL `IN (...)` slot), so that swap reads as one new and one stale entry and fails the run. The same expression re-added in the same function is still the same finding. Every baseline entry is rewritten in the new form; the 156 findings are unchanged, and only occurrence indexes renumber where one field had several different expressions.
TLDR
Problem this solves:
fors andifs are hard to readHow it solves it:
scripts/check_type_discipline.pyforor more than oneif# comprehension-ok: <reason>on any line the comprehension spans (reasonless one trips LIT005, one that suppresses nothing trips LIT013)User Flow
Before: a contributor adds a two-for comprehension and lint stays green
[(a, b) for a in xs for b in ys if a if b]in a file underlitellm/After: the same push fails lint with a message telling them how to fix it
LIT014 comprehension with 2 \for` clauses and 2 `if` clauses: at most one of each is allowed. Split it into a helper generator, a named intermediate, or a plain loop (suppress: `# comprehension-ok: `)`# comprehension-ok: <reason>when the stacked form is the clearest, and lint passesRelevant issues
Affected release
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This change is a lint rule, so the end-user surface is the checker and the gate, not the proxy. Video of the run below at tip d47f08c is posted in the Slack thread: https://berriaillm.slack.com/archives/C0B302ZJU05/p1790128816643949?thread_ts=1790128816.643949&cid=C0B302ZJU05
Shared setup:
Before (37f5267)
python3 scripts/check_type_discipline.py /tmp/lit013_snippet.py | grep -v LIT010After (d47f08c)
python3 scripts/check_type_discipline.py /tmp/lit013_snippet.py | grep -v LIT010for, oneif) does notuv run --no-sync python scripts/type_discipline_gate.py --base origin/mainOK: every LIT rule is within its codebase ceiling (base origin/main)Type
🚄 Infrastructure
Caveats (if any)
Low
# comprehension-okmarker on a line inside a multi-line violating comprehension suppresses only the innermost violating comprehension spanning that line plus any single-line violating one on it; a multi-line outer needs its own marker on a line the inner does not coverlitellm/, so the 4 stacked comprehensions underenterprise/are neither counted nor gated; pre-existing gate scope, unchanged hereruff checkandruff format --checkonscripts/check_type_discipline.pyfail identically on main (house format); the new code is format cleanFinal Attestation
Note
Low Risk
Lint-only infrastructure with no runtime or production behavior changes; existing violations are budget-grandfathered.
Overview
Adds LIT014 to the type-discipline linter: comprehensions (list/set/dict/generator) with more than one
foror more than oneifare flagged, with escape hatch# comprehension-ok: <reason>(wired into LIT005/LIT013 like other*-okmarkers). The checker uses AST analysis with nuanced ownership rules so suppressions apply to the innermost violating comprehension on a line, not sibling/outer violations.AGENTS.md documents the new style rule.
type-discipline-budget.jsonseeds LIT014 at 369 (grandfathered count; gate blocks net-new violations).type_discipline_gate.pyhelp text mentions the new suppression token. Tests cover clause counting, nested comprehensions, and suppression edge cases.Reviewed by Cursor Bugbot for commit d47f08c. Bugbot is set up for automated code reviews on this repo. Configure here.
Link to Devin session: https://app.devin.ai/sessions/adb960f503044c199f338d646b889b1b
Open in Devin Desktop: https://app.devin.ai/desktop/session/adb960f503044c199f338d646b889b1b?variant=devin
Requested by: @mateo-berri