Fix map no iterables - #8478
Open
william-goode wants to merge 2 commits into
Open
Conversation
Assisted-by: Claude Code:claude-fable-5
Assisted-by: Claude Code:claude-fable-5
Contributor
📝 WalkthroughWalkthrough
ChangesMap arity validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extra_tests/snippets/builtin_map.py`:
- Around line 27-33: Extend the no-iterables test around map construction to
also call map with the callable and strict=False as keyword arguments. Assert
that construction immediately raises TypeError with the same "map() must have at
least two arguments." message, preserving the existing positional-argument
assertion.
- Around line 30-32: Replace the `assert False` fallback in the map-construction
exception test with an explicit `AssertionError` raise, ensuring the test cannot
pass when `map()` does not raise. Preserve the `except TypeError as e` path and
explicitly validate that `str(e)` equals the required message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: ddabcb4e-559c-4cd8-aaf6-7f53c853a1f4
📒 Files selected for processing (2)
crates/vm/src/builtins/map.rsextra_tests/snippets/builtin_map.py
Comment on lines
+27
to
+33
| # test for no iterables | ||
| try: | ||
| map(lambda: 1) | ||
| assert False, "TypeError expected at map construction" | ||
| except TypeError as e: | ||
| assert str(e) == "map() must have at least two arguments." | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the keyword construction path before merging.
This test only exercises positional construction. Add a case such as map(lambda: 1, strict=False) and verify the same construction-time TypeError and message. This ensures the named-argument path cannot bypass the arity check.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 30-30: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extra_tests/snippets/builtin_map.py` around lines 27 - 33, Extend the
no-iterables test around map construction to also call map with the callable and
strict=False as keyword arguments. Assert that construction immediately raises
TypeError with the same "map() must have at least two arguments." message,
preserving the existing positional-argument assertion.
Comment on lines
+30
to
+32
| assert False, "TypeError expected at map construction" | ||
| except TypeError as e: | ||
| assert str(e) == "map() must have at least two arguments." |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '^\s*assert\b' extra_tests/snippets/builtin_map.pyRepository: RustPython/RustPython
Length of output: 485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- builtin_map.py lines 1-50 ---\n'
sed -n '1,50p' extra_tests/snippets/builtin_map.py | nl -ba
printf '\n--- assert usage in extra_tests/snippets/builtin_map.py (context) ---\n'
for line in 2 6 8 24 25 30 32 41; do
start=$(( line > 4 ? line-3 : 1 ))
end=$(( line+5 ))
[ "$start" -eq "$end" ] && end=$((end+1))
sed -n "${start},${end}p" extra_tests/snippets/builtin_map.py | nl -ba -v"$start"
printf '\n'
done
printf '\n--- ruff availability and B011 config availability ---\n'
command -v ruff || true
if command -v ruff >/dev/null 2>&1; then
ruff check extra_tests/snippets/builtin_map.py --select B011 || true
fi
printf '\n--- python optimized behavior for assert false / test assertion removal ---\n'
python3 - <<'PY'
import ast
src_normal = '''try:
x += 1
assert False, "TypeError expected at map construction"
except TypeError as e:
assert str(e) == "message"
'''
src_opt_only = '''try:
x += 1
assert False, "TypeError expected at map construction"
'''
mod_not_optimized = ast.parse(src_normal)
mod_optimized = ast.parse(src_opt_normal, optimize=2)
print("parsed_nodes_normal_function_call_count:",
sum(1 for _ in ast.walk(mod_normal) if isinstance(_, ast.Call) and
isinstance(getattr(getattr(_, "func", None), "attr", None), str) and _.func.attr == "assert"))
print("optimized_module_type:", mod_optimized.__module__)
def count_calls(node):
n=0
for child in ast.walk(node):
if isinstance(child, (ast.Assert, ast.Call)): n += 1
return n
print("normal_asserts:", sum(1 for node in ast.walk(mod_normal) if isinstance(node, ast.Assert)))
print("normal_calls:", sum(1 for node in ast.walk(mod_normal) if isinstance(node, ast.Call)))
print("optimized_asserts:", sum(1 for node in ast.walk(mod_optimized) if isinstance(node, ast.Assert)))
print("optimized_calls:", sum(1 for node in ast.walk(mod_optimized) if isinstance(node, ast.Call)))
PYRepository: RustPython/RustPython
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- builtin_map.py lines 1-50 ---'
sed -n '1,50p' extra_tests/snippets/builtin_map.py | nl -ba
printf '%s\n' '--- assert usage in extra_tests/snippets/builtin_map.py (context) ---'
for line in 2 6 8 24 25 30 32 41; do
start=$(( line > 4 ? line-3 : 1 ))
end=$(( line+5 ))
[ "$start" -eq "$end" ] && end=$((end+1))
sed -n "${start},${end}p" extra_tests/snippets/builtin_map.py | nl -ba -v"$start"
printf '%s\n' ''
done
printf '%s\n' '--- ruff availability and B011 config availability ---'
command -v ruff || true
if command -v ruff >/dev/null 2>&衣; then
ruff check extra_tests/snippets/builtin_map.py --select B011 || true
fi
printf '%s\n' '--- python optimized behavior for assert false / test assertion removal ---'
python3 - <<'PY'
import ast
src_normal = '''try:
x += 1
assert False, "TypeError expected at map construction"
except TypeError as e:
assert str(e) == "message"
'''
src_opt_only = '''try:
x += 1
assert False, "TypeError expected at map construction"
'''
mod_normal = ast.parse(src_normal)
mod_optimized = ast.parse(src_opt_only, optimize=2)
def count_nodes(node, cls):
return sum(1 for child in ast.walk(node) if isinstance(child, cls))
print("normal_asserts:", count_nodes(mod_normal, ast.Assert))
print("normal_calls:", count_nodes(mod_normal, ast.Call))
print("optimized_asserts:", count_nodes(mod_optimized, ast.Assert))
print("optimized_return_value:", mod_optimized.body[0].value.value)
PYRepository: RustPython/RustPython
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- builtin_map.py lines 1-50 ---'
sed -n '1,50p' extra_tests/snippets/builtin_map.py
printf '%s\n' '--- assert usage in extra_tests/snippets/builtin_map.py (line numbers via grep -n) ---'
grep -n 'assert False' extra_tests/snippets/builtin_map.py || true
grep -n 'assert str(e)' extra_tests/snippets/builtin_map.py || true
printf '%s\n' '--- python optimized behavior for assert False ---'
python3 - <<'PY'
import ast
import dis
import io
import sys
src = '''try:
x += 1
assert False, "TypeError expected at map construction"
except TypeError as e:
pass
'''
# Python 2.3 removed assert statements at parse time when optimization is selected.
# The function return value is None because the function body after optimization has no instructions.
mod_optimized = ast.parse(src, optimize=2)
comp = compile(mod_optimized, "<optimized>", "exec")
dis.dis(comp)
print("optimized_return_opcodes:", [name for name in (instr.opname for instr in dis.get_instructions(comp)) if name == "RETURN_VALUE"])
src_no_assert_success = '''try:
x += 1
except TypeError as e:
pass
'''
mod_no_assert = ast.parse(src_no_assert_success, optimize=0)
comp2 = compile(mod_no_assert, "<no_assert>", "exec")
print("optimized_code_is_same_as_no_assert:")
print(comp.co_code == comp2.co_code)
print(dis.dis(comp2))
PYRepository: RustPython/RustPython
Length of output: 1368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate ruff config files ---'
git ls-files | grep -E '(^|/)(pyproject\.toml|ruff\.toml|\.ruff\.toml)$' || true
printf '%s\n' '--- search for ruff B011 settings ---'
if [ -f pyproject.toml ]; then git grep -n '\bB011\b\|assert\.' pyproject.toml || true; fi
printf '%s\n' '--- Python compile optimization via command-line flag ---'
python3 - <<'PY'
import dis
import pycompile
src = '''try:
x += 1
assert False, "TypeError expected at map construction"
except TypeError as e:
pass
'''
src_no_assert = '''try:
x += 1
except TypeError as e:
pass
'''
try:
print("pycompile.get_source_from_code exists:", hasattr(pycompile.get_source_from_code, '__call__'))
except Exception as exc:
print("pycompile_access_error:", type(exc).__name__, str(exc))
code = compile(src, '<no_opt>', 'exec')
code_opt = compile(src, '<opt>', 'exec', optimize=2)
print("normal_code_bytes:", code.co_code.hex())
print("optimized_code_bytes:", code_opt.co_code.hex())
print("optimized_is_same_as_no_assert:", code_opt.co_code == compile(src_no_assert, '<no_assert>', 'exec').co_code)
print("disopt:")
dis.dis(code_opt)
PYRepository: RustPython/RustPython
Length of output: 490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ruff.toml relevant sections ---'
cat ruff.toml
printf '%s\n' '--- Ruff B011 status if ruff is available ---'
if command -v ruff >/dev/null 2>&1; then
ruff check extra_tests/snippets/builtin_map.py --select B011 || true
command -v ruff
else
command -v ruff || true
fi
printf '%s\n' '--- Python compile optimization via command-line flag ---'
python3 - <<'PY'
import dis
import sys
src = '''try:
x += 1
assert False, "TypeError expected at map construction"
except TypeError as e:
pass
'''
src_no_assert = '''try:
x += 1
except TypeError as e:
pass
'''
code = compile(src, '<no_opt>', 'exec')
code_opt = compile(src, '<opt>', 'exec', optimize=2)
code_no_assert = compile(src_no_assert, '<no_assert>', 'exec')
print("normal_code_bytes:", code.co_code.hex())
print("optimized_code_bytes:", code_opt.co_code.hex())
print("optimized_is_same_as_no_assert:", code_opt.co_code == code_no_assert.co_code)
print("optimized_return_opcodes:", [instr.opname for instr in dis.get_instructions(code_opt) if instr.opname == "RETURN_VALUE"])
PYRepository: RustPython/RustPython
Length of output: 1728
Use explicit AssertionError control flow.
assert False raises a lint error, and python -O removes it, so this branch can pass without catching the constructor TypeError or comparing the required message. Raise AssertionError for the expected failure, then check str(e) == "map() must have at least two arguments." explicitly.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 30-30: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extra_tests/snippets/builtin_map.py` around lines 30 - 32, Replace the
`assert False` fallback in the map-construction exception test with an explicit
`AssertionError` raise, ensuring the test cannot pass when `map()` does not
raise. Preserve the `except TypeError as e` path and explicitly validate that
`str(e)` equals the required message.
Sources: Coding guidelines, Linters/SAST tools
Contributor
|
@william-goode Thanks for fixing this! Much appreciated. I also have fixes for some of my other AI-generated issues that I’ve manually reviewed. Would it be okay for me to open PRs for them? I’ve held off because of the AI policy. |
Author
Hi @jseop-lim, Of course - apologies for scooping your issue. I'll leave the rest to you! Best, |
Contributor
@william-goode No need to apologize at all. I really appreciate you taking an interest in the issue and contributing a fix. I’m still new to Rust, so help from others is very welcome. If you’re planning to work on any of the other issues, please feel free to do so! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
map(f)with a single argument builds amapobject instead of raisingTypeError#8462Summary
This PR is to bring RustPython to CPython parity in the handling of maps constructed without iterables. CPython raises a TypeError if a map is constructed without an iterable. RustPython had no such check, so while constructing something like
map(lambda: 1)would succeed, iterating it would loop infinitely. Added a no-iterable check and an accompanying test.AI Usage
Code, commit messages, and this description are hand-written. Fable 5 was used for review.
Acknowledgements
Thank you @jseop-lim for raising the issue.
Summary by CodeRabbit
Bug Fixes
mapwithout iterables now correctly raises aTypeError.map()requires at least two arguments.Tests
mapconstruction.