Fix comment confusion bug in update_lib#7802
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR changes COMMENT reason extraction: the code compiles the COMMENT regex once, checks the decorator line first, then only the immediately preceding line if it starts with ChangesPatch Reason Extraction Logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_descr.py (TODO: 36) dependencies: dependent tests: (no tests depend on descr) Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/update_lib/tests/test_patch_spec.py (1)
152-174: ⚡ Quick winConsider adding edge case: both decorator line and previous line have comments.
The current test validates extraction when the comment is on the decorator line. Consider adding another test case to clarify expected behavior when both the decorator line AND the previous line contain
COMMENT:# TODO: RUSTPYTHON; reason1 `@unittest.expectedFailure` # TODO: RUSTPYTHON; reason2 def test_one(self): passShould this extract
reason1(previous line) orreason2(inline)? This would help validate the fix proposed in the previous review comments and prevent regression.Suggested additional test case
def test_comment_on_both_lines(self): """Test extraction precedence when comment appears on both decorator and previous line.""" code = f""" class TestFoo(unittest.TestCase): # {COMMENT}; previous line `@unittest.expectedFailure` # {COMMENT}; inline def test_one(self): pass """ patches = extract_patches(code) specs = patches["TestFoo"]["test_one"] self.assertEqual(len(specs), 1) # Verify which comment takes precedence (inline expected based on "check decorator first") self.assertIn("inline", specs[0].reason) self.assertNotIn("previous", specs[0].reason)🤖 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 `@scripts/update_lib/tests/test_patch_spec.py` around lines 152 - 174, Add a new unit test to cover the edge case where both the decorator line and the previous line contain the marker comment to assert which comment wins; create a test method (e.g., test_comment_on_both_lines) alongside test_comment_confusion that builds a class with a commented previous line and an inline decorator comment, calls extract_patches(code), inspects patches["TestFoo"]["test_one"], and asserts there is exactly one PatchSpec and that its reason matches the inline decorator comment (i.e., contains the inline text and not the previous-line text) to enforce the "check decorator first" precedence implemented by extract_patches.
🤖 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 `@scripts/update_lib/patch_spec.py`:
- Line 203: The code risks indexing out of bounds when dec_line == 0 because
prev_line = lines[dec_line - 1] will access lines[-1]; update the logic around
dec_line to defensively check bounds (e.g., if dec_line > 0 then set prev_line =
lines[dec_line - 1] else set prev_line to a safe default like "" or None) so
that the decorator-at-first-line case is handled without reading the last file
line; apply this change where prev_line and dec_line are used in patch_spec.py
to ensure safe access.
- Around line 193-211: The code currently assigns reason from the decorator line
then unconditionally re-checks the previous line which can overwrite it; update
the logic in the block that uses pattern, dec_line, reason and lines so the
previous-line search only runs if reason is still None and dec_line > 0 (i.e.,
use an elif/guard: only compute prev_line and run pattern.search(prev_line) when
reason is None and dec_line-1 is in bounds and
prev_line.strip().startswith("#")), and when a match is found set reason =
found.group() as before.
---
Nitpick comments:
In `@scripts/update_lib/tests/test_patch_spec.py`:
- Around line 152-174: Add a new unit test to cover the edge case where both the
decorator line and the previous line contain the marker comment to assert which
comment wins; create a test method (e.g., test_comment_on_both_lines) alongside
test_comment_confusion that builds a class with a commented previous line and an
inline decorator comment, calls extract_patches(code), inspects
patches["TestFoo"]["test_one"], and asserts there is exactly one PatchSpec and
that its reason matches the inline decorator comment (i.e., contains the inline
text and not the previous-line text) to enforce the "check decorator first"
precedence implemented by extract_patches.
🪄 Autofix (Beta)
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
Run ID: b563e144-15cd-48b3-8f05-ee10a9d6f0ce
⛔ Files ignored due to path filters (1)
Lib/test/test_descr.pyis excluded by!Lib/**
📒 Files selected for processing (2)
scripts/update_lib/patch_spec.pyscripts/update_lib/tests/test_patch_spec.py
|
|
||
| # Search the previous line of the decorator, | ||
| # only take the comment if the line starts with a `#` | ||
| prev_line = lines[dec_line - 1] |
There was a problem hiding this comment.
Index out of bounds risk when dec_line == 0.
If a decorator somehow appears on line 1 of the file (lineno=1), then dec_line = 0 and lines[dec_line - 1] becomes lines[-1], which incorrectly accesses the last line of the file instead of checking bounds.
While unlikely in practice (decorators are inside class methods), defensive programming requires a bounds check.
Proposed fix (standalone)
-# Search the previous line of the decorator,
-# only take the comment if the line starts with a `#`
-prev_line = lines[dec_line - 1]
-if prev_line.strip().startswith("#") and (
- found := pattern.search(prev_line)
-):
- reason = found.group()
+# Search the previous line of the decorator,
+# only take the comment if the line starts with a `#`
+if dec_line > 0:
+ prev_line = lines[dec_line - 1]
+ if prev_line.strip().startswith("#") and (
+ found := pattern.search(prev_line)
+ ):
+ reason = found.group()Note: This issue is also addressed in the previous comment's combined fix.
🤖 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 `@scripts/update_lib/patch_spec.py` at line 203, The code risks indexing out of
bounds when dec_line == 0 because prev_line = lines[dec_line - 1] will access
lines[-1]; update the logic around dec_line to defensively check bounds (e.g.,
if dec_line > 0 then set prev_line = lines[dec_line - 1] else set prev_line to a
safe default like "" or None) so that the decorator-at-first-line case is
handled without reading the last file line; apply this change where prev_line
and dec_line are used in patch_spec.py to ensure safe access.
ShaharNaveh
force-pushed
the
comment-confusion-bug
branch
from
5e6ef4a to
6b9f9e3
Compare
ShaharNaveh
force-pushed
the
comment-confusion-bug
branch
from
6b9f9e3 to
4789717
Compare
2 participants
Before this fix. the script would do:
Because the logic was: "search the current and the previous line for the comment, if you find it, take it".
Now we have a different logic
Summary by CodeRabbit
Bug Fixes
Tests