Repository navigation
Infer list augmented concatenation with tuples (#1865) - #3357
Conversation
(cherry picked from commit bc8cd9d)
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
The CodSpeed flag looks like hosted-runner walltime noise rather than a real regression. Its own report warns about the unknown walltime environment on standard hosted runners, and 3 of the 4 benchmarks were skipped with baselines substituted. Checked locally to be sure: ran the cold-lint subprocess benchmark 7 times each on base cf52489 and on this branch's head d1e7ccb. Base median 447ms (mean 460), head median 438ms (mean 460) - identical within noise. That matches the shape of the change: the only added hot-path cost is one short-circuited isinstance chain inside tl_infer_binary_op, nothing at import time, so a cold-start regression of this size would be surprising. Happy to re-run anything if you'd like. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3357 +/- ##
=======================================
Coverage 91.04% 91.04%
=======================================
Files 93 93
Lines 11808 11808
Branches 1945 1945
=======================================
Hits 10751 10751
Misses 737 737
Partials 320 320
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Pierre-Sassoulas
left a comment
Pierre-Sassoulas
left a comment
There was a problem hiding this comment.
Let's discuss the isinstance check, rest LGTM
| if isinstance(other, self.__class__) and operator == "+": | ||
| # list.__iadd__ accepts a tuple, unlike list.__add__; tuples have no __iadd__. | ||
| if (isinstance(other, self.__class__) and operator == "+") or ( | ||
| isinstance(self, nodes.List) |
There was a problem hiding this comment.
| isinstance(self, nodes.List) |
# list.__iadd__ accepts a tuple, unlike list.__add__; tuples have no __iadd__.
=> So this check is always true and we can remove it ?
"+=" reaches tl_infer_binary_op only for lists: tuples have no __iadd__, so the augmented flow falls back to "+" for them. Pointed out in review. Signed-off-by: Charan Rathore <[email protected]>
|
You're right, the check was always true. "+=" reaches tl_infer_binary_op only through the augmented flow after a successful iadd lookup, and tuples have no iadd, so for a Tuple the flow falls back to "+" before getting here. Dropped isinstance(self, nodes.List) and reworded the comment to say that instead. Pushed as 6908e1b. Verified: the augassign parametrized tests (12) pass, tests/test_inference.py fully green (487 passed), and the rest of the suite is unchanged apart from pre-existing environment failures here (pydantic version and missing gi libs, failing identically without this change). black and ruff clean on the touched file. |
Supersedes #3331 (re-filed; the fork behind that PR was removed).
Type of Changes
Description
Closes #1865
list.__iadd__accepts a tuple, unlikelist.__add__. The sequence inference handler currently only handles+between two lists or two tuples. As a result,values = []; values += (1, 2)infers asUninferable.Handle
+=when the left operand is a list and the right operand is a list or tuple. The result remains a list. Normal list-plus-tuple and tuple-plus-list operations remain unsupported, and tuple augmented addition keeps its existing fallback to+.This reuses the existing size guard and element inference, including unknown placeholders for ambiguous elements. Tests cover empty and populated operands, invalid operations, ambiguous elements, repeated elements, and keeping the source list node unchanged.
Tests: 12 regression/control cases pass; full suite 2257 passed, 60 skipped, 15 xfailed. Ruff is clean and pylint rates the changed source file 10.00/10.
Scope: only list
+=with a list or tuple operand is handled. list + tuple, tuple + list, tuple+=list and list+=int stay unsupported on purpose, and other iterable operands (strings, sets, custom iterables) are out of scope. Tested on Python 3.10 only; other versions are left to CI.