Skip to content

Fix empty commented arrays in legacy Reader - #1719

Open
vaibhav0806 wants to merge 1 commit into
open-source-parsers:masterfrom
vaibhav0806:fix/legacy-empty-array-comments-1238
Open

vaibhav0806 wants to merge 1 commit into
open-source-parsers:masterfrom
vaibhav0806:fix/legacy-empty-array-comments-1238

Conversation

@vaibhav0806

@vaibhav0806 vaibhav0806 commented Oct 1, 2026 •

Copy link
Copy Markdown

Fixes #1238.

With comments enabled, legacy Json::Reader rejects {"list":[ //\n]} and leaves a partial array containing null. CharReader already accepts the same document as an empty array.

Consume allowed line/block comment tokens before the legacy reader's initial empty-array delimiter check, so it does not create element zero before discovering ]. Use the existing error recovery for malformed comments. Add four regression fixtures covering root/nested arrays, collection on/off, comments disallowed, unterminated comments, ordinary element/container comments, and offsets.

Only Reader::readArray and its tests change. Public headers, declarations and class layout are unchanged. Modern parsing, numeric conversion, trailing-comma handling and comment storage are unchanged.

Local validation on macOS arm64, Apple Clang 17, C++11, Debug, default configuration and static library:

  • All 3 CTests pass, including 135 unit tests. The new regression fixture fails when linked against the original library; the original example now succeeds for both reader APIs with array size zero.
  • A 152-case legacy comparison covers comment and dropped-null settings: 136 controls are identical, 12 target combinations become empty arrays, and 4 malformed-comment combinations remain errors but avoid the partial null element. Collected comment text is retained. Numeric and modern-reader probe results are unchanged.
  • The original and patched static libraries have the same 2,108 external symbol names; this is not a formal cross-platform dynamic ABI audit.
  • git diff --check and clang-format 17/20 checks pass; both formatters produce identical output for the changed files.

The seven upstream workflows currently await maintainer approval on head 167330d624e203b35d4df35a33b935f04f810bea; no jobs or checks have run. Exact clang-format 18, Linux/Windows/shared-library builds and other language standards remain unverified. cppcheck was unavailable locally, so no static-analysis success is claimed.

Prepared with AI assistance. The generated diff was independently checked and the local validation was run by an automated agent. No human review is claimed.

@vaibhav0806
vaibhav0806 marked this pull request as ready for review October 1, 2026 07:21
@greptile-apps

greptile-apps Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Fixes comment parsing in empty JSON arrays.

The PR should not merge until collected comments in empty nested arrays remain associated with the correct value.

Findings

  1. P1 Comments attach to sibling values ▶
Summary

The PR makes the legacy Reader consume leading array comments before checking for an empty array and adds regression fixtures for parsing and offsets.

  • Comment collection needs attention: comments inside newly accepted empty nested arrays can be attached to a subsequent value.

Reviews (1) · Last reviewed commit: "fix: parse empty commented arrays in leg..."

Comment on lines +489 to +494
if (!readToken(comment)) {
return addErrorAndRecover(
"Syntax error: value, object or array expected.", comment,
tokenArrayEnd);
}
skipSpaces();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Comments attach to sibling values

With comment collection enabled, {"a":[ /* inner */],"b":1} now parses as an empty array, but the comment remains pending when that array closes. Reading b then attaches the comment to b instead of the array that contains it, so the parsed comment is assigned to the wrong value.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging this; the reported placement reproduces. On original master 3347a4b, CharReader already assigns /* inner */ to b for this exact input. Legacy Reader does the same for the equivalent empty-object control, {"a":{\n/* inner */},"b":1}. Legacy Reader rejects the empty-array example and leaves a partial a:[null], so there was no successful legacy array result to preserve.

A local C++11 comparison covered 14 documents × collection on/off × both APIs (56 observations per library), checking parsed data and every node's comment placements. All 28 modern-reader observations and all 14 previously successful legacy controls were unchanged; all patched legacy results matched original-master CharReader data/comments. Cases included the exact example, line/block comments, last members, nested arrays, preceding siblings, and nonempty-array/empty-object controls. This supports compatibility with existing behavior, while the concern about comment ownership remains valid. Moving inner comments onto empty containers would change existing semantics; this PR keeps the parsing fix narrow.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for validating this against both the original legacy behavior and CharReader. I agree the comment placement is existing semantics rather than a regression introduced by this change: the same input already assigns the comment to the following sibling in CharReader, and moving it onto the empty array would make legacy behavior diverge from both the established modern behavior and the equivalent empty-object case. Given that the comparison shows no changes to previously successful controls or modern parsing, I’m withdrawing this finding; no change is needed.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parse error when "//" comment follows array and array is empty

1 participant