Skip to content

Fix empty commented arrays in legacy Reader #1719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vaibhav0806 wants to merge 1 commit into
base: master
Choose a base branch
from
+103 −0
Open

Fix empty commented arrays in legacy Reader #1719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/lib_json/json_reader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,18 @@ bool Reader::readArray(Token& token) {
currentValue().swapPayload(init);
currentValue().setOffsetStart(token.start_ - begin_);
skipSpaces();
if (features_.allowComments_) {
while (current_ != end_ && *current_ == '/' && (current_ + 1) != end_ &&
(current_[1] == '/' || current_[1] == '*')) {
Token comment;
if (!readToken(comment)) {
return addErrorAndRecover(
"Syntax error: value, object or array expected.", comment,
tokenArrayEnd);
}
skipSpaces();
Comment on lines +489 to +494

@greptile-apps greptile-apps Bot Oct 1, 2026

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.

@vaibhav0806 vaibhav0806 Oct 1, 2026

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.

@greptile-apps greptile-apps Bot Oct 1, 2026

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.

}
}
if (current_ != end_ && *current_ == ']') // empty array
{
Token endArray;
Expand Down
91 changes: 91 additions & 0 deletions src/test_lib_json/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3007,6 +3007,11 @@ struct ReaderTest : JsonTest::TestCase {
JSONTEST_ASSERT(reader->parse(input, root));
}

template <typename Input>
void checkParse(Input&& input, bool collectComments) {
JSONTEST_ASSERT(reader->parse(input, root, collectComments));
}

template <typename Input>
void
checkParse(Input&& input,
Expand All @@ -3023,6 +3028,11 @@ struct ReaderTest : JsonTest::TestCase {
JSONTEST_ASSERT_EQUAL(formatted, reader->getFormattedErrorMessages());
}

template <typename Input>
void checkParseFailure(Input&& input, bool collectComments) {
JSONTEST_ASSERT(!reader->parse(input, root, collectComments));
}

std::unique_ptr<Json::Reader> reader{new Json::Reader()};
Json::Value root;
};
Expand Down Expand Up @@ -3095,6 +3105,87 @@ JSONTEST_FIXTURE_LOCAL(ReaderTest, parseComment) {
checkParse(" true //comment1\n//comment2\r//comment3\r\n");
}

JSONTEST_FIXTURE_LOCAL(ReaderTest, parseEmptyArrayWithComments) {
for (bool collectComments : {false, true}) {
for (const auto& doc :
{std::string("[ // line\n]"), std::string("[ /* block */ ]"),
std::string("[ // one\n/* two */ ]"),
std::string("{\"list\":[ // line\n]}"),
std::string("{\"list\":[ /* block */ ]}"),
std::string("{\"list\":[ // one\n/* two */ ]}")}) {
root = Json::Value();
checkParse(doc, collectComments);

const Json::Value& arrayValue =
doc[0] == '[' ? root : root[Json::StaticString("list")];
const std::size_t arrayStart = doc.find('[');
const std::size_t arrayLimit = doc.rfind(']') + 1;
JSONTEST_ASSERT(arrayValue.isArray());
JSONTEST_ASSERT_EQUAL(0U, arrayValue.size());
JSONTEST_ASSERT(!arrayValue.isValidIndex(0));
JSONTEST_ASSERT_EQUAL(static_cast<int>(arrayStart),
arrayValue.getOffsetStart());
JSONTEST_ASSERT_EQUAL(static_cast<int>(arrayLimit),
arrayValue.getOffsetLimit());
}
}
}

JSONTEST_FIXTURE_LOCAL(ReaderTest, parseArrayCommentBehaviorUnchanged) {
{
std::string doc = "[ /* before one */ 1 ]";
const std::size_t valueStart = doc.find('1');
const std::size_t valueLimit = valueStart + 1;
for (bool collectComments : {false, true}) {
root = Json::Value();
checkParse(doc, collectComments);
JSONTEST_ASSERT_EQUAL(1U, root.size());
JSONTEST_ASSERT_EQUAL(1, root[0].asInt());
JSONTEST_ASSERT_EQUAL(static_cast<int>(valueStart),
root[0].getOffsetStart());
JSONTEST_ASSERT_EQUAL(static_cast<int>(valueLimit),
root[0].getOffsetLimit());
JSONTEST_ASSERT_EQUAL(collectComments,
root[0].hasComment(Json::commentBefore));
if (collectComments) {
JSONTEST_ASSERT_STRING_EQUAL("/* before one */",
root[0].getComment(Json::commentBefore));
}
}
}
{
std::string doc = "// before\n[]// after\n";
root = Json::Value();
checkParse(doc, true);
JSONTEST_ASSERT(root.hasComment(Json::commentBefore));
JSONTEST_ASSERT_STRING_EQUAL("// before",
root.getComment(Json::commentBefore));
JSONTEST_ASSERT(root.hasComment(Json::commentAfterOnSameLine));
JSONTEST_ASSERT_STRING_EQUAL("// after",
root.getComment(Json::commentAfterOnSameLine));
}
}

JSONTEST_FIXTURE_LOCAL(ReaderTest, parseEmptyArrayCommentsDisallowed) {
Json::Features features = Json::Features::all();
features.allowComments_ = false;
setFeatures(features);

root = Json::Value();
checkParseFailure("[ // comment\n]", true);

root = Json::Value();
checkParseFailure("[ /* comment */ ]", false);
}

JSONTEST_FIXTURE_LOCAL(ReaderTest, parseEmptyArrayWithUnterminatedComment) {
root = Json::Value();
checkParseFailure("[ /*", true);
JSONTEST_ASSERT(root.isArray());
JSONTEST_ASSERT_EQUAL(0U, root.size());
JSONTEST_ASSERT(!root.isValidIndex(0));
}

JSONTEST_FIXTURE_LOCAL(ReaderTest, streamParseWithNoErrors) {
std::string styled = R"({ "property" : "value" })";
std::istringstream iss(styled);
Expand Down