Java: fix two comment-scanning defects that drop modifiers from the LST - #8564
Open
timtebeek wants to merge 2 commits into
Open
Java: fix two comment-scanning defects that drop modifiers from the LST#8564timtebeek wants to merge 2 commits into
timtebeek wants to merge 2 commits into
Conversation
`sortedModifiersAndAnnotations` and `collectAnnotations` scan the source
between an annotation and the declaration it precedes to recover modifiers
in source order. Their hand-rolled comment scanner sets `inMultilineComment`
on `/*` without first checking whether it is already inside a `//` comment,
so a line comment containing `/*` latches the flag; nothing clears it short
of a literal `*/`.
The scan then finds no modifier keywords, so the modifiers never make it
into the LST and the declaration's type is printed at the modifier's offset:
@deprecated //*not a block comment
public String value() { ... }
prints as `String String value()`, and with a type shorter than `public`
the leftover characters are emitted as the name's prefix (`Step` over
`public` yields `Stepic`).
Guard both scanners against re-entering comment state, in all five parser
modules.
timtebeek
force-pushed
the
tim/java-print-idempotency-defects
branch
from
August 19, 2026 19:42
e45ae28 to
0235435
Compare
timtebeek
marked this pull request as ready for review
August 19, 2026 19:49
/*, markdown doc comment EOI sentinel)The close condition in `sortedModifiersAndAnnotations` and
`collectAnnotations` only checked that the previous character was `*`, so
the third character of `/*/` satisfied it and the scanner left comment
state while still inside the comment. It then read the comment body as
modifiers and dropped the real ones:
@deprecated /*/ not the end of the comment */ public String value()
printed as `... String String value()` -- the same silent corruption as the
`//*` case, from the opposite direction.
Track where the opener started and require the closing `/` to be at least
three characters past it, so the opener's own `/` can never close the
comment. This also subsumes the `i > 0` bounds check, which was present
only in `collectAnnotations`; `sortedModifiersAndAnnotations` could read
`source.charAt(-1)`.
Adds a regression test for the inverse case too, `/* // */` before
modifiers, which the preceding commit fixes but nothing covered.
timtebeek
force-pushed
the
tim/java-print-idempotency-defects
branch
from
August 19, 2026 20:23
e5df27e to
611efaf
Compare
timtebeek
marked this pull request as draft
August 19, 2026 20:24
timtebeek
marked this pull request as ready for review
August 19, 2026 21:07
greg-at-moderne
approved these changes
Aug 20, 2026
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.
Two Java parser print-idempotency defects, found while triaging
ParseFailuresrows from a large ingest. Both surface as:Both are broken comment-state tracking in the same two scanners, reached from opposite directions, so they're one PR with a commit each.
A third, unrelated defect from the same triage is split out to #8565.
1.
/*inside a line comment latches the modifier scannersortedModifiersAndAnnotationsandcollectAnnotationswalk the source between an annotation and the declaration that follows, to recover modifiers in source order. Their comment scanner setsinMultilineCommenton/*without checking whether it is already inside a//comment:Walking
//*x: the first/setsinComment, the second seesnext == '*'and setsinMultilineComment. At the newlineinCommentis cleared butinMultilineCommentis not, and nothing clears it short of a literal*/. The scan finds no modifier keywords, so the modifiers never enter the LST and the declaration's type is printed at the modifier's offset.Reproducer:
prints as:
Stringandpublicare both 6 characters, so the clobber lands cleanly here. With a shorter type the leftover tail ofpublicis emitted as the name's prefix — aStepreturn type prints asStepic Step, and a 7-characterTaskletswallows the following space and prints asTaskletTasklet.The
/*can appear anywhere in the comment, not just immediately after the//. Fields are affected too (there the modifier is simply lost). A real/* ... */later on clears the state and masks the bug.Scope: not a regression — the scanner dates back to
a59e6e970f(2021) and this reproduces on every release since. Present identically in the Java 8, 11, 17, 21 and 25 parsers; all five are fixed here, and the test is in the TCK so it runs against each.Fix: guard the opener against re-entering comment state.
2.
/*/closes a block comment it shouldn'tSame two scanners, opposite direction from (1). The close condition only checked that the previous character was
*:The third character of
/*/satisfies that, so the scanner leaves comment state while still inside the comment, then reads the comment body as modifiers:prints as
... String String value()— the same corruption as (1), reached the other way.Fix: record where the opener started and require the closing
/to be at least three characters past it, so the opener's own/can never close the comment. This also subsumes thei > 0bounds check, which was only present incollectAnnotations—sortedModifiersAndAnnotationscould readsource.charAt(-1).Scope: pre-existing, same five parser modules.
Testing
Five new TCK tests in
CommentTest; all five fail onmainand pass with the fix, and being in the TCK they run against every parser module. Beyond the reproducers above, the/*/change was checked against/**/,/***/,/*/*/,/* /* */, back-to-back/*/ a */ /*/ b */, a multi-line/*/-opened comment, and no-whitespace@Deprecated/*/ x */public. FullcompatibilityTestgreen acrossrewrite-java-8/11/17/21/25, plus therewrite-java,rewrite-java-testandrewrite-java-lomboktest suites.Aside, not touched here:
StringUtils(rewrite-core) has a scanner of the same shape that can also latchinMultilineCommentfrom inside a//comment, e.g. on// /*. It consumes two characters per delimiter so//*doesn't trip it, and I have no failing case for it, so I've left it alone rather than widen this PR.