Repository navigation
fix(read): track Python triple-quoted strings in minimal filter - #3336
Conversation
Python has no block comments, but the minimal filter treats """ as one. Two things go wrong. A line like `QUERY = """` contains the delimiter without starting with it, so it is taken for a block-comment opener. The same """ then satisfies the closing test on that one line, so the line is dropped and its string body is left behind as loose text. A single-line docstring such as """Module doc.""" toggles the docstring flag once and never back, so the filter stays in docstring mode for the rest of the file and stops stripping comments entirely. Give Python a string-aware path instead. Minimal keeps docstrings, so the only thing to remove is # comments and the only state needed is whether we are inside a triple-quoted string. Delimiter kinds are tracked separately so a ''' inside a """ string does not close it. Other languages keep the existing block-comment walk untouched. Refs rtk-ai#3260
…iple quotes
The triple-quote tracker scanned raw bytes, so a `"""` written inside a
one-line string (`marker = '"""'`), after a trailing `#` comment, or
preceded by a backslash toggled the string state. From there every later
real delimiter flipped the wrong way, and a `#` line inside a real
triple-quoted string was stripped as a comment.
Skip one-line string literals whole, stop at `#` outside a string, and let
a backslash escape the next byte inside any string. In an f-string or
t-string a replacement field may reuse the outer quote (PEP 701), so the
quote only ends the string outside `{...}`.
The README said `aggressive` drops docstrings. It has no string awareness
and keeps a string line that looks like an import or a signature, so say
that instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
KuSh
left a comment
There was a problem hiding this comment.
Round 1 of 3 — APPROVED, follow-ups filed
Claim: -l minimal gives Python its own string-aware path. A QUERY = """ line is no longer deleted, and a one-line docstring no longer stops comment stripping for the rest of the file.
Scope: accept. This fixes #1322 and finding 2 of #3425.
Ran: rtk read -l minimal on 9,690 real .py files (the CPython 3.14 stdlib and /usr/lib/python3/dist-packages), scored against a reference built with Python's tokenize. That reference drops only comment-only lines outside strings. Other runs:
- Under the hook,
cat/head/tailon a.pyfile rewrite tortk readat levelnone, which never reaches this path. So only an explicit-l minimalor-l aggressivedoes. - Non-Python output: 10,131 Rust, C, JS and Go files at minimal and aggressive, 0 differences.
- Python at
-l aggressive, which runs minimal first: no signature or import line lost.
Python savings go from 6.4% to 9.4%. That is below the 20% floor, but this is a correctness fix to an existing level, and savings go up.
CI @875333c0: all 11 checks pass.
| develop | this PR as opened | with the fixup | |
|---|---|---|---|
| code lines lost | 1,029 in 202 files | 8 in 2 files | 0 |
| comment lines left in | 76,589 in 3,073 files | 1,165 in 57 files | 158 in 21 files (comment-only files, which rtk read shows raw by design) |
Blocks merge (1, closed by the fixup ea1ead8)
-
src/core/filter.rs:163:advance_triple_quotescanned raw bytes, so any of these toggled the state:- a
"""inside a one-line string (marker = '"""') - a
"""after a trailing# - a
"""right after a backslash
From then on every real delimiter flipped the wrong way, and a
#line inside a real string was stripped. That is howpdb.py(line 1526) andapsw/tests/ftstests.py(line 1259) lost lines. Inftstests.pythe PR as opened dropped 6 lines of test data inside anf"""block that develop kept.The fixup changes the scan in three ways:
- it skips one-line strings whole;
- it stops at
#outside a string; - a backslash escapes the next byte.
In f-strings and t-strings it also counts
{...}depth, so a quote reused inside a replacement field (PEP 701) does not end the string. Four tests were added, and each fails when its branch of the scan is disabled. The fixup also rewords one README sentence, becauseaggressivehas no string awareness: it keeps a docstring line that looks like an import or a signature. - a
875333c merges develop into the branch. The semgrep job diffs against the branch's merge-base, which was from July, so it flagged a test cleanup line in src/cmds/git/git_cmd.rs that came from develop. After the merge it passes.
Optional — will not hold merge
- Consider
Fixes #1322in the description.
Filed as follow-ups
- #4251:
rtk read -(stdin) and.pyifiles never reach the Python path, so they still get the C-style walk. This is outside this PR's hunks. The related case of extensionless files is #4249. - #4252: five Python 3.12+ f-string shapes still flip the state: a replacement field spanning lines, and
{or}inside a string nested in a field. Develop loses these lines too, and none of the 9,690 files contains one. - Stripping the shebang and
# -*- codinglines is already covered by #3427.
Checked and correct — no need to re-verify
- Both repros from the description, before and after.
'''inside"""and the reverse.#lines inside docstrings.- CRLF files, empty files, comment-only files (the raw fallback still applies), and non-UTF-8 files (same error and exit code as develop).
- The branch merges cleanly with develop, which now includes #2397, and every test passes.
Hypotheses built and dropped
- Runs of blank lines inside strings collapsing, and
aggressivekeeping import-shaped string lines: both behave the same on develop. - The
/*case in brace languages belongs to #2397 and #2385.
Your questions
- "Happy to take it as a separate PR" (the
/*case): no need. #2397 has merged and covers the line-start form, and the remaining shapes are tracked on #2385. - "I will rebase whichever one lands second" (#3335): still needed. #3335 conflicts with this PR only in
src/core/README.md.
Next
I pushed two commits. ea1ead8 is the string-aware scan, its tests and the README sentence described above; 875333c merges develop. Approving.
Rounds: 1/3. Threads: 0 open.
…after the Python early return Since rtk-ai#3336, `MinimalFilter::filter` returns early for Python at `src/core/filter.rs:280` (`filter_python_minimal`). The generic loop below still carr... Signed-off-by: chenzeyan54-commits <chenzeyan54-commits@users.noreply.github.com>
Summary
The minimal filter treats
"""as a block-comment delimiter for Python. Python has no block comments."""opens a string, which may be a docstring or an ordinary value. Matching it with the line-oriented block-comment rules goes wrong in two ways.First, a line like
QUERY = """contains the delimiter without starting with it, so it is taken for a block-comment opener. The same"""then satisfies the closing test on that one line. The line is dropped and its string body is left behind as loose text.Second, a one-line docstring such as
"""Module doc."""toggles the docstring flag once and never back. The filter stays in docstring mode for the rest of the file and stops stripping comments at all. Most Python modules open with a docstring, so this is the common case rather than the corner case.The fix gives Python a string-aware path. Minimal keeps docstrings, so the only thing to remove is
#comments, and the only state needed is whether we are inside a triple-quoted string. Delimiter kinds are tracked separately so a'''inside a"""string does not close it. Comment lines are skipped before the scan, so a delimiter mentioned inside a comment does not open a string.Every other language keeps the existing block-comment walk. The change is an early return guarded on
Language::Python, so no other language can reach different code.Blast radius
-l minimalaffects all Python, so I checked the surrounding behavior rather than assuming.Measured reduction at
-l minimalon Python 3.14 stdlib files, before and after:json/decoder.pyargparse.pytyping.pydataclasses.pydataclasses.pyis unchanged because its module docstring spans several lines, which the old toggle happened to handle.Correctness over 60 stdlib files, comparing filtered output against the set of real code lines in each source, where a real code line means non-blank, not a whole-line
#comment, and not inside a string according totokenize:Non-Python output is unchanged. I diffed both binaries over 294 comparisons and found no differences:
minimalandaggressive, covering Rust, Shell, TOML and Markdownnone,minimalandaggressive, covering JavaScript, TypeScript, Go, Ruby, Java, C, C++ and an unknown extensionOne thing I did not change, so it is not covered by the above. The same
contains()matching misfires for the brace languages when a string literal holds comment-like text. That is #2384, which is closed as completed, buttrimmed.contains(start)is still ondevelopatsrc/core/filter.rs:173, and the same-line open and close case at 178-183 still runscontinueunconditionally. Both behaviours reproduce today:The declaration is gone while the use of
patternremains, which is the "field names that do not exist" symptom from that issue. An unterminated marker is worse, because it swallows the rest of the file:I have left this out of this PR because it is a wider change with its own risk profile, and this one is scoped to Python. Flagging it here because #2384 reads as resolved and is not. Happy to take it as a separate PR if you want it.
Test plan
cargo fmt --all -- --checkclean.cargo clippy --all-targetsreports 0 warnings and 0 errors.cargo test --allpasses 2580 tests across 8 test binaries, 8 ignored, 0 failed.bash scripts/check-test-presence.sh origin/developpasses.Five tests added in
src/core/filter.rs, taking the main binary from 2511 to 2516. Three of them fail on unmodifieddevelop:'''written inside a"""docstring does not end it (fails before)#line inside a docstring is kept as prose (passes before, guards the fix)#comments are still stripped (passes before, guards the fix)Relationship to #3335
This is a follow-up to #3335 and does not depend on it. I found this bug while testing that one. The two are independent and can merge in either order. This branch is cut from
developand shares no code with #3335.They both touch
src/core/filter.rs, which auto-merges cleanly. They both add a section tosrc/core/README.md, which conflicts. I will rebase whichever one lands second.This does not fix #3260. That issue is about
-l aggressiveand XML.🤖 Opened with Superhuman, an open-source contribution agent.