d-string v2 - #122
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds d-string support across tokenizing, parsing, interpolation handling, and tests, including new prefix validation, dedent tracking, and dedented handling for plain, f-string, and t-string forms. Changesd-string language feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
|
@cursor review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Lib/test/test_dstring.py (1)
165-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
sourcebetweentest_raw_dstringandtest_raw_dbstring.Both methods build the identical
sourceand differ only in checkingd()vsdb(). Consider consolidating via the existingcheck_dbstringhelper (used elsewhere in this class) to avoid the duplicated triple-quoted fixture.♻️ Proposed consolidation
- def test_raw_dstring(self): - source = r""" - path\\to\\file - keep\\n - """ - self.assertEqual(d(source), "path\\to\\file\nkeep\\n\n") - - def test_raw_dbstring(self): - source = r""" - path\\to\\file - keep\\n - """ - self.assertEqual(db(source), b"path\\to\\file\nkeep\\n\n") + def test_raw_dbstring(self): + source = r""" + path\\to\\file + keep\\n + """ + self.check_dbstring(source, "path\\to\\file\nkeep\\n\n")🤖 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 `@Lib/test/test_dstring.py` around lines 165 - 177, The two tests duplicate the same raw triple-quoted source fixture in test_raw_dstring and test_raw_dbstring; consolidate them using the existing check_dbstring helper in the test class. Update the dstring test methods to share the common source setup through check_dbstring while keeping the distinct expectations for d() and db(), so the fixture is defined once and both behaviors are still verified.
🤖 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 `@Parser/action_helpers.c`:
- Around line 1339-1342: The early return after `PyUnicodeWriter_Create` failure
in the parser helper leaves `p->call_invalid_rules` set to 1, so restore the
previous value there just like the `error:` path and normal success flow do.
Update the `action_helpers.c` logic around `PyUnicodeWriter_Create` to reset
`p->call_invalid_rules` from `_prev_call_invalid` before returning `NULL`,
keeping the parser state consistent across recovery/second-pass handling.
---
Nitpick comments:
In `@Lib/test/test_dstring.py`:
- Around line 165-177: The two tests duplicate the same raw triple-quoted source
fixture in test_raw_dstring and test_raw_dbstring; consolidate them using the
existing check_dbstring helper in the test class. Update the dstring test
methods to share the common source setup through check_dbstring while keeping
the distinct expectations for d() and db(), so the fixture is defined once and
both behaviors are still verified.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cdea12c-829d-4d03-bf3d-418d835f610c
📒 Files selected for processing (9)
Lib/test/test_dstring.pyLib/test/test_tokenize.pyLib/tokenize.pyObjects/unicodeobject.cParser/action_helpers.cParser/lexer/lexer.cParser/lexer/state.cParser/lexer/state.hParser/string_parser.c
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 8700c54. Configure here.
039ac27 to
2357f93
Compare
|
@codex[agent] review this |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 907c747bd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!_prev_call_invald && !rawmode) { | ||
| PyObject *temp; | ||
| if (bytesmode) { | ||
| temp = decode_bytes_with_escapes(p, s, len, t); |
There was a problem hiding this comment.
For db literals that contain both a non-ASCII source character and an invalid escape, this preliminary decode emits (or, under -Werror::SyntaxWarning, raises) the invalid-escape warning before the later bytes-ASCII check runs. Regular bytes literals report only bytes can only contain ASCII literal characters in that scenario, so db'''\n é\z\n ''' changes error precedence and can fail for the wrong reason when warnings are errors. Validate the raw bytes for non-ASCII before calling decode_bytes_with_escapes() here.
Useful? React with 👍 / 👎.
| if (dedent_bytes == NULL) { | ||
| return NULL; | ||
| } | ||
| if (PyBytes_AsStringAndSize(dedent_bytes, (char**)&s, (Py_ssize_t*)&len) < 0) { |
There was a problem hiding this comment.
This lets PyBytes_AsStringAndSize() write a Py_ssize_t through storage declared as size_t (and similarly writes a char * through a const char ** after the cast). That is undefined behavior under C aliasing/type rules even though it happens to work on typical platforms; use local char * and Py_ssize_t variables for the call, then assign them back to s and len.
Useful? React with 👍 / 👎.
907c747 to
a450eae
Compare
|
@codex review this |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
476f8f0 to
904a7cc
Compare
1 participant
Summary by CodeRabbit
dstrings (d,db,df,dt) including raw variants.dstrings, including correct handling inside f-/t-string interpolation and format specifications.d/D.dinteractions with interpolation and format handling.Note
High Risk
Large changes to core tokenizer/parser/string literal handling affect all source parsing; mistakes could cause subtle syntax or dedent bugs in f/t/d combined literals.
Overview
Adds d-strings (PEP 822): triple-quoted literals with a
dprefix that strip common leading indentation from every physical line (including lines inside{...}/ format specs / t-string interpolation text), normalize whitespace-only lines to a single newline, and require the body to start with a newline after the opening quotes.The lexer recognizes
d(withb,r,f,tcombinations where allowed), rejects single-quoted and non-tripledforms, tracks longest common leading whitespace per active d-string, and attaches it toFSTRING_END/TSTRING_ENDmetadata. Plaind/db/drliterals dedent instring_parser.c; df/dt defer dedent until parse end inaction_helpers.c(after escape checks).u+dis rejected;Lib/tokenize.pyprefix lists and prefix-discovery tests use multiline"""\n"""probes.Exports
_Py_search_longest_common_leading_whitespacefromunicodeobject.cfor indent calculation. NewLib/test/test_dstring.pycovers syntax rules, dedent edge cases, and f/t interactions.Reviewed by Cursor Bugbot for commit 8700c54. Bugbot is set up for automated code reviews on this repo. Configure here.