Fix properties line continuation handling - #8431
Merged
Merged
Conversation
…ashes The parser decided a line continued by looking only at whether the previous character was a backslash. That misread two cases: - With CRLF line endings the character before the newline is the carriage return, so `key=a \<CRLF> b` was not treated as a continuation: the value was truncated to `a \` and the continuation line was absorbed as whitespace prefix of the next content. - An escaped backslash at the end of a line (`path=C:\\dir\\`) was treated as a continuation, swallowing the following entry into the value. Track the number of consecutive trailing backslashes instead, ignoring an intervening carriage return, so a line continues only on an odd count. Relates to #8417
…hind Replaces the trailing-backslash counter with a check of the buffered line itself, which drops the unbounded carriage-return exemption (a run of stray CRs kept the count alive and paired a backslash with a newline it did not precede) and lets the same rule cover comments. Also fixes four adjacent defects of the same kind: - Comments were continued. java.util.Properties never continues a comment, so `# install dir: C:\App\` swallowed the entry on the following line, which then vanished from the LST entirely. - A blank line after a continuation did not terminate the logical line: the next property was absorbed into the previous value, and rewriting that value through withText() erased the absorbed line from the file. - entryFromLine used the same single-character escape lookback for `=` and `:` in keys, so `a\\=b` parsed as one escaped key with no delimiter and an empty value instead of key `a\` and value `b`. - isDelimitedByWhitespace passed the index `line.length() - 1` to Character.isWhitespace rather than the character at it, so a whitespace-delimited line whose length happened to fall in 10-14 or 29-33 was dropped from the LST.
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
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.
What's changed?
PropertiesParserdecided whether a line continued from a single character of lookbehind (prev == '\\'). This replaces that with a check of the buffered line itself — an odd number of trailing backslashes, ignoring the CR of a CRLF pair, and never on a comment line — and fixes the neighbouring defects of the same kind that surfaced while testing it.Note on the reported symptom
The round-trip in the issue's repro does not reproduce on
main— the LST already keeps the raw, continued text inProperties.Value.text(getSource()returns it with continuations,getText()returns it flattened), andPropertiesPrinterprintsgetSource(). I verified this end-to-end:parses and prints back byte-for-byte, and
ChangePropertyValue/ChangePropertyKey/AddProperty/DeletePropertyall leave the continued entry's formatting intact when they touch a different property. (Changing the continued value itself does replace the whole value, which is expected — the new value has no continuation layout to keep.) That case is now covered bylineContinuationWithWhitespaceBeforeBackslashas a characterization test.What digging around it did turn up is a cluster of continuation bugs that are invisible to a round-trip — the raw text is always printed back verbatim — but corrupt the LST, so recipes either silently skip a property or delete it. All of these drop an
EntryfromProperties.File.getContent():1. CRLF line endings broke continuation detection. With
\r\nthe character before the newline is\r, not\\, soorg.gradle.jvmargs=-Xms1g \<CRLF> -Xmx4g<CRLF>parsed the value as the truncated-Xms1g \and absorbed the continuation line as whitespace prefix.getText()returned-Xms1g \instead of-Xms1g -Xmx4g, so any recipe reading or rewriting that value operated on a wrong value — the kind of thing that forces the text-splicing workaround described in the issue.2. An escaped backslash at end of line was treated as a continuation.
\\is a literal backslash, sopath=C:\\dir\\followed byother=valueis two entries; the parser mergedother=valueintopath's value and dropped theotherentry.3. Comments were continued.
java.util.Propertiesnever continues a comment, so# install dir: C:\App\followed bykey=valueswallowed the entry into the comment message. Worse than a missed match: a recipe that edits that comment deletes the real property.4. A blank line after a continuation did not terminate the logical line.
key=a \/ blank /next=valueabsorbednext=valueintokey's value. SinceChangePropertyValuedoesvalue.withText(newValue)over that whole multi-line text, running it onkeyemitskey=band silently deletes thenext=valueline from the user's file.5.
entryFromLinehad the same single-character escape lookback for=/:in keys.a\\=bparsed as one escaped key withDelimiter.NONEand an empty value, instead of keya\and valueb.6.
isDelimitedByWhitespacepassed an index where a char was wanted —Character.isWhitespace(line.length() - 1)instead ofcharAt(...). A whitespace-delimited line whose length happened to land in 10-14 or 29-33 (those minus one being the whitespace code points) was dropped from the LST entirely.3-6 are pre-existing rather than new, but they're all the same defect — deciding a lexical property of a line from one character — and 3 and 4 become much easier to hit once CRLF files reach the continuation path, so fixing them together seemed better than leaving landmines behind the fix.
Anything in particular you'd like reviewers to focus on?
Relates tovsFixesis right for Properties parser/printer flattens backslash line continuations #8417: I couldn't reproduce the exact reported flattening onmain, so I'd rather leave the issue open for the reporter to confirm against a real file (CRLF endings would be my first guess at the difference) than auto-close it.a=b\leavesgetText()asb\wherejava.util.Propertiesreportsb. Fixing it properly means teachingProperties.Continuationto do real escape processing (getText()today doesn't unescape\n,\tor\:either), which changes the semantics of a public API for every recipe that matches on values — too big to smuggle into this PR.Checklist
./gradlew :rewrite-properties:test— 135 tests, 0 failures; every new test verified to fail without its fix:rewrite-gradle:test --tests "*Propert*" --tests "*Wrapper*",:rewrite-maven:test --tests "*Propert*",:rewrite-android:test— all green