Repository navigation
THRIFT-3669: Keep the lib/js TJSONProtocol read stack aligned after empty lists - #4059
Merged
Merged
Conversation
…mpty lists Client: js readListBegin pushes the first element of a list onto the read stack, and readListEnd popped a single entry before it pushed the next element of the enclosing list. After a non-empty list that entry is the list itself. An empty list has no first element and leaves an undefined entry above itself, so the single pop left the emptied list on the stack, and the next element went on top of it. The stack stayed out of step from there, and the elements of the enclosing list that followed were lost or read from the wrong place: [[],[],[1,2]] came back as [[],[],[]]. Sets end through readListEnd and were affected the same way. readListEnd now pops back to the enclosing container before it pushes the next element, as the Node.js library does since THRIFT-6409. test-nested-empty-lists.html reads nested lists, a list of sets and a map of nested lists with empty lists before, between and after non-empty ones, and round-trips list<list<string>> and list<list<list<string>>> through the code generated from JsDeepConstructorTest.thrift. Gruntfile.js runs it with the other QUnit pages. A comment in container_size_limit.test.js that described the old behaviour is updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
THRIFT-3669
readListEndinlib/js/src/thrift.jspopped a single entry off the read stack before it pushed the next element of the enclosing list. An empty list has no first element and leaves anundefinedentry above itself, so after an empty list the single pop left the emptied list on the stack, and the next element went on top of it. From there the elements of the enclosing list were lost or read from the wrong place:[[],[],[1,2]]came back as[[],[],[]]. Sets end throughreadListEndand were affected the same way.readListEndnow pops back to the enclosing container before it pushes the next element, as the Node.js library does since THRIFT-6409 (#4054).readMapEndalready pops back to its saved position, so a map is only affected through the lists it holds.New QUnit page
lib/js/test/test-nested-empty-lists.html, registered inGruntfile.jswith the other pages:[[],[],[1,2]],[[1],[],[],[2]],[[[1,2],[]],[],[[3]]][[],[],[5,6]]{"0":[[],[]],"1":[[7]]}"undefined"list<list<string>>andlist<list<list<string>>>through generated codeRun as the
lib-jsCI job does, inthrift:jammy:make -C lib/js checkpasses with 12 QUnit pages, 189 tests and 763 assertions; jshint passes. On master the new page fails 7 of 7 tests. A comment incontainer_size_limit.test.jsthat described the old behaviour is updated.🤖 Generated with Claude Code