JSON Object unvisited member checking no longer works

[anothersimulacrum]

Describe the bug

JSON unvisited member checking was introduced in #34735 and turned on outside of tests in #45635.
It provides an error on loading the game when a JSON object has data in it that the game makes no attempt to read, which is a significant help with debugging.

A bisect gives the first bad commit as any of
97d6a48
ec3f896
2833d7a
ca9428d
8987088

Clearly, the new flexbuffer JsonObject visited member reporting is not working.

Attach save file

N/A

Steps to reproduce

Apply this patch

diff --git a/data/json/items/obsolete.json b/data/json/items/obsolete.json
index 2b023a1ab1..1f2afd98da 100644
--- a/data/json/items/obsolete.json
+++ b/data/json/items/obsolete.json
@@ -26,6 +26,7 @@
     "price": 500,
     "price_postapoc": 100,
     "weight": "300 g",
+    "BOB": "ALICE",
     "volume": "250 ml",
     "material": [ "plastic" ],
     "looks_like": "mre_beef_box",
diff --git a/data/json/monsters/mi-go.json b/data/json/monsters/mi-go.json
index c3208f5c72..777e919e3c 100644
--- a/data/json/monsters/mi-go.json
+++ b/data/json/monsters/mi-go.json
@@ -5,6 +5,7 @@
     "name": { "str": "mi-go" },
     "description": "An alien creature of uncertain origin.  Its shapeless pink body bears numerous sets of paired appendages of unknown function, and a pair of ribbed, membranous wings which seem to be quite useless.  Its odd, vaguely pyramid-shaped head bristles with numerous wavering antennae, and it moves with an uncanny fluidity on its many legs.",
     "default_faction": "mi-go",
+    "BAR": "FOO",
     "bodytype": "migo",
     "species": [ "MIGO" ],
     "volume": "92500 ml",

The game does not report that it fails to visit the BAR and BOB members, when there is no code reading them.
There's also existing unvisited JSON in the game source already: #64408

Cataclysm-DDA/data/json/items/book/driving.json

Lines 236 to 239 in 2d6713b

	"skill": "chemistry",
	"required_level": 2,
	"max_level": 5,
	"fun": 0,

Expected behavior

Unvisited JSON members are reported.

Screenshots

No response

Versions and configuration

Any versions since #50143 was merged

#64408 shows it is not working on multiple platforms (tested on Linux).

Additional context

CC @akrieger , since you're the subject matter expert here.

[akrieger]

As Aivean commented on the original PR, the naming of the guard variables confused me and made me think it was tautologically disabled :(

The relevant code is just ifdef'd out. It should be trivial to reenable. Unfortunately fixing the resulting shown issues might be harder, sorry :(

[anothersimulacrum]

Oh, it's just that easy? I'll handle it, thanks for the pointer.

[akrieger]

Yeah I made sure to implement and test it but left it (mistakenly) off because I thought it was already off.

[anothersimulacrum]

Curse past me for not renaming those variables :)

[PatrikLundell]

@anothersimulacrum: Consider past you cursed. It can be noted that the curse has since expired. Sorry for the inconvenience during that period.

[akrieger]

Before I forget, the original code had two bools, one for 'should report unvisited' and one for 'did report unvisited'. I didn't implement the latter. We can reimplement the logic by putting at the end of the conditional here after error_skipped_members(...), the line visited_fields_bitset_.set_all();

Cataclysm-DDA/src/flexbuffer_json.cpp

Lines 262 to 272 in 32774b3

	for( size_t bit_idx = 0, end = visited_fields_bitset_.size() % tiny_bitset::kBitsPerBlock;
	bit_idx < end; ++bit_idx ) {
	if( !( block & mask ) ) {
	skipped_members.emplace_back( block_idx * tiny_bitset::kBitsPerBlock + bit_idx );
	}
	mask >>= 1;
	}
	error_skipped_members( skipped_members );
	}
	}

[anothersimulacrum]

I've got some sort of issue with deferred JSON reporting failing to visit members (like copy-from, proportional, ammo). I haven't had the time look enough into it. (Adding the line you recommend doesn't change that).

[akrieger]

Feel free to hit me up on discord anytime to take a look w/ repro steps.

[anothersimulacrum]

Figured it out! It looks like:

diff --git a/src/init.cpp b/src/init.cpp
index fa7e2b7bbf..78cb2173dd 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -162,7 +162,7 @@ void DynamicDataLoader::load_deferred( deferred_json &data )
         auto it = data.begin();
         for( size_t idx = 0; idx != n; ++idx ) {
             try {
-                JsonObject jo = it->first;
+                const JsonObject &jo = it->first;
                 load_object( jo, it->second );
             } catch( const JsonError &err ) {
                 debugmsg( "(json-error)\n%s", err.what() );
diff --git a/src/mapgen.cpp b/src/mapgen.cpp
index 3221abbf70..893bb87a94 100644
--- a/src/mapgen.cpp
+++ b/src/mapgen.cpp
@@ -4263,7 +4263,7 @@ void mapgen_function_json_base::setup_common()
     if( is_ready ) {
         return;
     }
-    JsonObject jo = jsobj;
+    const JsonObject &jo = jsobj;
     mapgen_defer::defer = false;
     if( !setup_common( jo ) ) {
         jo.throw_error( "format: no terrain map" );

I do have a new issue, though. Because we're saving JsonObjects in game entities now, these unvisited member warnings trigger for some things when the game is saved.

[akrieger]

Change

Cataclysm-DDA/src/mapgen.cpp

Line 810 in 32774b3

jsobj.allow_omitted_members();

to this->jsobj.allow_omitted_members(); to fix the mapgen error on save.

I'm not sold on the two changes you listed above but I trust you have verified they Do The Right Thing. I guess incrementally re-loading an object can only incrementally visit more members, so it's likely fine. But I think the original intent with those is to start fresh so that if an object passes load, we can properly assert all members were visited. But I don't remember if those objects are allow_omitted_members()'d when inserted into deferred_data or not.

Edit: The DynamicDataLoader one is correct. The mapgen one can be reverted with my fix.