Merge pull request #46350 from Qrox/json-check

Show source location in error messages of deferred json

src/generic_factory.h

@@ -221,7 +221,8 @@ class generic_factory
                         def = ab->second;
                     } else {
                         def.was_loaded = false;
-                        deferred.emplace_back( jo.str(), src );
+                        deferred.emplace_back( jo.get_source_location(), src );
+                        jo.allow_omitted_members();
                         return false;
                     }
                 }

src/init.cpp

@@ -44,6 +44,7 @@
 #include "item_factory.h"
 #include "json.h"
 #include "loading_ui.h"
+#include "lru_cache.h"
 #include "magic.h"
 #include "magic_enchantment.h"
 #include "magic_ter_furn_transform.h"
@@ -119,30 +120,65 @@ void DynamicDataLoader::load_object( const JsonObject &jo, const std::string &sr
     it->second( jo, src, base_path, full_path );
 }
 
+struct DynamicDataLoader::cached_streams {
+    lru_cache<std::string, shared_ptr_fast<std::istringstream>> cache;
+};
+
+shared_ptr_fast<std::istream> DynamicDataLoader::get_cached_stream( const std::string &path )
+{
+    cata_assert( !finalized &&
+                 "Cannot open data file after finalization." );
+    if( !stream_cache ) {
+        stream_cache = std::make_unique<cached_streams>();
+    }
+    shared_ptr_fast<std::istringstream> cached = stream_cache->cache.get( path, nullptr );
+    // Create a new stream if the file is not opened yet, or if some code is still
+    // using the previous stream (in such case, `cached` and `stream_cache` have
+    // two references to the stream, hence the test for > 2).
+    if( !cached ) {
+        cached = make_shared_fast<std::istringstream>( read_entire_file( path ) );
+    } else if( cached.use_count() > 2 ) {
+        cached = make_shared_fast<std::istringstream>( cached->str() );
+    }
+    stream_cache->cache.insert( 8, path, cached );
+    return cached;
+}
+
 void DynamicDataLoader::load_deferred( deferred_json &data )
 {
     while( !data.empty() ) {
         const size_t n = data.size();
         auto it = data.begin();
         for( size_t idx = 0; idx != n; ++idx ) {
-            try {
-                std::istringstream str( it->first );
-                JsonIn jsin( str );
-                JsonObject jo = jsin.get_object();
-                load_object( jo, it->second );
-            } catch( const std::exception &err ) {
-                debugmsg( "Error loading data from json: %s", err.what() );
+            if( !it->first.path ) {
+                debugmsg( "JSON source location has null path, data may load incorrectly" );
+            } else {
+                try {
+                    shared_ptr_fast<std::istream> stream = get_cached_stream( *it->first.path );
+                    JsonIn jsin( *stream, it->first );
+                    JsonObject jo = jsin.get_object();
+                    load_object( jo, it->second );
+                } catch( const JsonError &err ) {
+                    debugmsg( "(json-error)\n%s", err.what() );
+                }
             }
             ++it;
         }
         data.erase( data.begin(), it );
         if( data.size() == n ) {
-            std::string discarded;
             for( const auto &elem : data ) {
-                discarded += elem.first;
+                if( !elem.first.path ) {
+                    debugmsg( "JSON source location has null path when reporting circular dependency" );
+                } else {
+                    try {
+                        shared_ptr_fast<std::istream> stream = get_cached_stream( *it->first.path );
+                        JsonIn jsin( *stream, elem.first );
+                        jsin.error( "JSON contains circular dependency, this object is discarded" );
+                    } catch( const JsonError &err ) {
+                        debugmsg( "(json-error)\n%s", err.what() );
+                    }
+                }
             }
-            debugmsg( "JSON contains circular dependency.  Discarded %i objects:\n%s",
-                      data.size(), discarded );
             data.clear();
             return; // made no progress on this cycle so abort
         }
@@ -640,6 +676,7 @@ void DynamicDataLoader::finalize_loaded_data( loading_ui &ui )
     }
 
     check_consistency( ui );
+    stream_cache.reset();
     finalized = true;
 }
 

src/init.h

@@ -9,6 +9,9 @@
 #include <vector>
 #include <utility>
 
+#include "json.h"
+#include "memory_fast.h"
+
 class loading_ui;
 class JsonObject;
 class JsonIn;
@@ -59,13 +62,16 @@ class DynamicDataLoader
 
         /**
          * JSON data dependent upon as-yet unparsed definitions
-         * first: JSON data, second: source identifier
+         * first: JSON source location, second: source identifier
          */
-        using deferred_json = std::list<std::pair<std::string, std::string>>;
+        using deferred_json = std::list<std::pair<json_source_location, std::string>>;
 
     private:
         bool finalized = false;
 
+        struct cached_streams;
+        std::unique_ptr<cached_streams> stream_cache;
+
     protected:
         /**
          * Maps the type string (coming from json) to the
@@ -161,6 +167,14 @@ class DynamicDataLoader
         bool is_data_finalized() const {
             return finalized;
         }
+
+        /**
+         * Get a possibly cached stream for deferred data loading. If the cached
+         * stream is still in use by outside code, this returns a new stream to
+         * avoid conflict of stream cursor. The stream cursor is not reset if a
+         * cached stream is returned.
+         */
+        shared_ptr_fast<std::istream> get_cached_stream( const std::string &path );
 };
 
 #endif // CATA_SRC_INIT_H

src/item_factory.cpp

@@ -1611,7 +1611,8 @@ bool Item_factory::load_definition( const JsonObject &jo, const std::string &src
         return true;
     }
 
-    deferred.emplace_back( jo.str(), src );
+    deferred.emplace_back( jo.get_source_location(), src );
+    jo.allow_omitted_members();
     return false;
 }
 

src/json.cpp

@@ -263,6 +263,21 @@ JsonIn *JsonObject::get_raw( const std::string &name ) const
     return jsin;
 }
 
+json_source_location JsonObject::get_source_location() const
+{
+    if( !jsin ) {
+        throw JsonError( "JsonObject::get_source_location called when stream is null" );
+    }
+    json_source_location loc;
+    loc.path = jsin->get_path();
+    if( !loc.path ) {
+        jsin->seek( start );
+        jsin->error( "JsonObject::get_source_location called but the path is unknown" );
+    }
+    loc.offset = start;
+    return loc;
+}
+
 /* returning values by name */
 
 bool JsonObject::get_bool( const std::string &name ) const
@@ -1640,6 +1655,7 @@ bool JsonIn::read( JsonDeserializer &j, bool throw_on_error )
 // WARNING: for occasional use only.
 std::string JsonIn::line_number( int offset_modifier )
 {
+    const std::string &name = path ? *path : "<unknown source file>";
     if( stream && stream->eof() ) {
         return name + ":EOF";
     } else if( !stream || stream->fail() ) {

src/json.h

@@ -17,6 +17,7 @@
 
 #include "colony.h"
 #include "enum_conversions.h"
+#include "memory_fast.h"
 #include "string_id.h"
 
 /* Cataclysm-DDA homegrown JSON tools
@@ -93,6 +94,11 @@ struct number_sci_notation {
     int64_t exp = 0;
 };
 
+struct json_source_location {
+    shared_ptr_fast<std::string> path;
+    int offset = 0;
+};
+
 /* JsonIn
  * ======
  *
@@ -177,8 +183,7 @@ class JsonIn
 {
     private:
         std::istream *stream;
-        // Used for error message and thus intentionally untranslated
-        std::string name = "<unknown source file>";
+        shared_ptr_fast<std::string> path;
         bool ate_separator = false;
 
         void skip_separator();
@@ -187,10 +192,19 @@ class JsonIn
 
     public:
         JsonIn( std::istream &s ) : stream( &s ) {}
-        JsonIn( std::istream &s, const std::string &name ) : stream( &s ), name( name ) {}
+        JsonIn( std::istream &s, const std::string &path )
+            : stream( &s ), path( make_shared_fast<std::string>( path ) ) {}
+        JsonIn( std::istream &s, const json_source_location &loc )
+            : stream( &s ), path( loc.path ) {
+            seek( loc.offset );
+        }
         JsonIn( const JsonIn & ) = delete;
         JsonIn &operator=( const JsonIn & ) = delete;
 
+        shared_ptr_fast<std::string> get_path() const {
+            return path;
+        }
+
         bool get_ate_separator() {
             return ate_separator;
         }
@@ -895,6 +909,7 @@ class JsonObject
         // seek to a value and return a pointer to the JsonIn (member must exist)
         JsonIn *get_raw( const std::string &name ) const;
         JsonValue get_member( const std::string &name ) const;
+        json_source_location get_source_location() const;
 
         // values by name
         // variants with no fallback throw an error if the name is not found.

src/lru_cache.cpp

@@ -1,10 +1,13 @@
 #include "lru_cache.h"
 
 #include <cstddef>
+#include <sstream>
 #include <iterator>
 #include <memory>
+#include <string>
 
 #include "map_memory.h"
+#include "memory_fast.h"
 #include "point.h"
 
 template<typename Key, typename Value>
@@ -73,3 +76,4 @@ const std::list<typename lru_cache<Key, Value>::Pair> &lru_cache<Key, Value>::li
 template class lru_cache<tripoint, memorized_terrain_tile>;
 template class lru_cache<tripoint, int>;
 template class lru_cache<point, char>;
+template class lru_cache<std::string, shared_ptr_fast<std::istringstream>>;

src/mapgen.cpp

@@ -9,7 +9,6 @@
 #include <map>
 #include <memory>
 #include <set>
-#include <sstream>
 #include <stdexcept>
 #include <unordered_map>
 
@@ -443,9 +442,11 @@ load_mapgen_function( const JsonObject &jio, const std::string &id_base, const p
             jio.throw_error( "function does not exist", "name" );
         }
     } else if( mgtype == "json" ) {
-        const std::string jstr = jio.get_object( "object" ).str();
+        JsonObject jo = jio.get_object( "object" );
+        const json_source_location jsrc = jo.get_source_location();
+        jo.allow_omitted_members();
         ret = std::make_shared<mapgen_function_json>(
-                  jstr, mgweight, "mapgen " + id_base, offset );
+                  jsrc, mgweight, "mapgen " + id_base, offset );
         oter_mapgen.add( id_base, ret );
     } else {
         jio.throw_error( R"(invalid value: must be "builtin" or "json")", "method" );
@@ -460,9 +461,10 @@ static void load_nested_mapgen( const JsonObject &jio, const std::string &id_bas
         if( jio.has_object( "object" ) ) {
             int weight = jio.get_int( "weight", 1000 );
             JsonObject jo = jio.get_object( "object" );
-            std::string jstr = jo.str();
+            const json_source_location jsrc = jo.get_source_location();
+            jo.allow_omitted_members();
             nested_mapgen[id_base].add(
-                std::make_shared<mapgen_function_json_nested>( jstr, "nested mapgen " + id_base ),
+                std::make_shared<mapgen_function_json_nested>( jsrc, "nested mapgen " + id_base ),
                 weight );
         } else {
             debugmsg( "Nested mapgen: Invalid mapgen function (missing \"object\" object)", id_base.c_str() );
@@ -479,10 +481,11 @@ static void load_update_mapgen( const JsonObject &jio, const std::string &id_bas
     if( mgtype == "json" ) {
         if( jio.has_object( "object" ) ) {
             JsonObject jo = jio.get_object( "object" );
-            std::string jstr = jo.str();
+            const json_source_location jsrc = jo.get_source_location();
+            jo.allow_omitted_members();
             update_mapgen[id_base].push_back(
                 std::make_unique<update_mapgen_function_json>(
-                    jstr, "update mapgen " + id_base ) );
+                    jsrc, "update mapgen " + id_base ) );
         } else {
             debugmsg( "Update mapgen: Invalid mapgen function (missing \"object\" object)",
                       id_base.c_str() );
@@ -588,8 +591,8 @@ bool mapgen_function_json_base::check_inbounds( const jmapgen_int &x, const jmap
 }
 
 mapgen_function_json_base::mapgen_function_json_base(
-    const std::string &s, const std::string &context )
-    : jdata( s )
+    const json_source_location &jsrcloc, const std::string &context )
+    : jsrcloc( jsrcloc )
     , context_( context )
     , do_format( false )
     , is_ready( false )
@@ -600,10 +603,10 @@ mapgen_function_json_base::mapgen_function_json_base(
 
 mapgen_function_json_base::~mapgen_function_json_base() = default;
 
-mapgen_function_json::mapgen_function_json( const std::string &s, const int w,
+mapgen_function_json::mapgen_function_json( const json_source_location &jsrcloc, const int w,
         const std::string &context, const point &grid_offset )
     : mapgen_function( w )
-    , mapgen_function_json_base( s, context )
+    , mapgen_function_json_base( jsrcloc, context )
     , fill_ter( t_null )
     , rotation( 0 )
 {
@@ -613,8 +616,8 @@ mapgen_function_json::mapgen_function_json( const std::string &s, const int w,
 }
 
 mapgen_function_json_nested::mapgen_function_json_nested(
-    const std::string &s, const std::string &context )
-    : mapgen_function_json_base( s, context )
+    const json_source_location &jsrcloc, const std::string &context )
+    : mapgen_function_json_base( jsrcloc, context )
     , rotation( 0 )
 {
 }
@@ -2431,8 +2434,13 @@ void mapgen_function_json_base::setup_common()
     if( is_ready ) {
         return;
     }
-    std::istringstream iss( jdata );
-    JsonIn jsin( iss );
+    if( !jsrcloc.path ) {
+        debugmsg( "null json source location path" );
+        return;
+    }
+    shared_ptr_fast<std::istream> stream = DynamicDataLoader::get_instance().get_cached_stream(
+            *jsrcloc.path );
+    JsonIn jsin( *stream, jsrcloc );
     JsonObject jo = jsin.get_object();
     mapgen_defer::defer = false;
     if( !setup_common( jo ) ) {
@@ -6484,8 +6492,8 @@ void add_corpse( map *m, const point &p )
 
 //////////////////// mapgen update
 update_mapgen_function_json::update_mapgen_function_json(
-    const std::string &s, const std::string &context ) :
-    mapgen_function_json_base( s, context )
+    const json_source_location &jsrcloc, const std::string &context ) :
+    mapgen_function_json_base( jsrcloc, context )
 {
 }
 
@@ -6578,7 +6586,8 @@ mapgen_update_func add_mapgen_update_func( const JsonObject &jo, bool &defer )
         return update_function;
     }
 
-    update_mapgen_function_json json_data( "", "unknown object in add_mapgen_update_func" );
+    update_mapgen_function_json json_data( json_source_location {},
+                                           "unknown object in add_mapgen_update_func" );
     mapgen_defer::defer = defer;
     if( !json_data.setup_update( jo ) ) {
         const auto null_function = []( const tripoint_abs_omt &, mission * ) {