glideapps / quicktype

Generate types and converters from JSON, Schema, and GraphQL
https://app.quicktype.io
Apache License 2.0
11.77k stars 1.04k forks source link

Incorrect namespace in C++ to/from json (nlohmann) #1185

Closed hemantvallabh closed 1 year ago

hemantvallabh commented 5 years ago

The to_json/from_json functions should be in the same namespace as the structure. I'm using Visual Studio 2017 (VC++)

Additional information: https://github.com/nlohmann/json/issues/780#issuecomment-376696058

QuickType Code Gen

#pragma once

#include "json.hpp"

namespace quicktype {
    using nlohmann::json;

    inline json get_untyped(const json & j, const char * property) {
        if (j.find(property) != j.end()) {
            return j.at(property).get<json>();
        }
        return json();
    }

    inline json get_untyped(const json & j, std::string property) {
        return get_untyped(j, property.data());
    }

    struct Welcome {
        int64_t response_code;
        std::string response_message;
    };
}

namespace nlohmann {
    void from_json(const json & j, quicktype::Welcome & x);
    void to_json(json & j, const quicktype::Welcome & x);

    inline void from_json(const json & j, quicktype::Welcome& x) {
        x.response_code = j.at("response_code").get<int64_t>();
        x.response_message = j.at("response_message").get<std::string>();
    }

    inline void to_json(json & j, const quicktype::Welcome & x) {
        j = json::object();
        j["response_code"] = x.response_code;
        j["response_message"] = x.response_message;
    }
}

Working Code

#pragma once
namespace quicktype {
    using nlohmann::json;

    inline json get_untyped(const json & j, const char * property) {
        if (j.find(property) != j.end()) {
            return j.at(property).get<json>();
        }
        return json();
    }

    inline json get_untyped(const json & j, std::string property) {
        return get_untyped(j, property.data());
    }

    struct Welcome {
        int64_t response_code;
        std::string response_message;
    };

    void from_json(const json & j, quicktype::Welcome & x);
    void to_json(json & j, const quicktype::Welcome & x);

    inline void from_json(const json & j, quicktype::Welcome& x) {
        x.response_code = j.at("response_code").get<int64_t>();
        x.response_message = j.at("response_message").get<std::string>();
    }

    inline void to_json(json & j, const quicktype::Welcome & x) {
        j = json::object();
        j["response_code"] = x.response_code;
        j["response_message"] = x.response_message;
    }
}
smalhotra-spirent commented 5 years ago

I am facing a similar issue with Visual Studio 2017. What I have done is to add an additional useTopLevelNamespace option for C++. This basically reuses the top level namespace provided for wrapping to_json and from_json functions.

--[no-]useTopLevelNamespace                       Moves to_json and from_json functions under the top level namespace (off by default)

I can create a PR for this, but haven't had time to run tests.

Here's the diff:

commit 3053f81167b8686ce4646a977bfcdb63463cde40 (HEAD -> smalhotra/devel)
Author: Sumeet Malhotra <sumeet.malhotra@spirent.com>
Date:   Fri Jun 28 11:23:00 2019 +0530

    Add useTopLevelNamespace option to all VS2017 compiles

diff --git a/src/quicktype-core/language/CPlusPlus.ts b/src/quicktype-core/language/CPlusPlus.ts
index 20a5c7cd..c628d868 100644
--- a/src/quicktype-core/language/CPlusPlus.ts
+++ b/src/quicktype-core/language/CPlusPlus.ts
@@ -73,6 +73,10 @@ export const cPlusPlusOptions = {
         "not-permissive",
         "secondary"
     ),
+    useTopLevelNamespace: new BooleanOption(
+        "useTopLevelNamespace",
+        "Moves to_json and from_json functions under the top level namespace",
+        false),
     westConst : new EnumOption(
       "const-style",
       "Put const to the left/west (const T) or right/east (T const)",
@@ -121,6 +125,7 @@ export class CPlusPlusTargetLanguage extends TargetLanguage {
             cPlusPlusOptions.codeFormat,
             cPlusPlusOptions.wstring,
             cPlusPlusOptions.msbuildPermissive,
+            cPlusPlusOptions.useTopLevelNamespace,
             cPlusPlusOptions.westConst,
             cPlusPlusOptions.typeSourceStyle,
             cPlusPlusOptions.includeLocation,
@@ -2050,6 +2055,9 @@ export class CPlusPlusRenderer extends ConvenienceRenderer {
             if (this._options.msbuildPermissive) {
                 namespaces = ["nlohmann", "detail"];
             }
+            if (this._options.useTopLevelNamespace) {
+                namespaces = this._namespaceNames.slice();
+            }
             this.emitNamespaces(namespaces, () => {
                 this.forEachObject("leading-and-interposing", (_: any, className: Name) =>
                     this.emitClassHeaders(className)
llilakoblock commented 4 years ago

Confirmed in Visual Studio 2019.

jushar commented 3 years ago

@schani As to the linked issue (https://github.com/nlohmann/json/issues/780#issuecomment-376696058) the fix described by @smalhotra-spirent correctly solves the problem and better complies with the C++ standard.

His fix only adds a new option, thus it can't even break backwards compatibility and is the only chance to get it to compile with the latest Microsoft compiler (I think after a short test phase, it should get the default though).

If it helps to speed things up, I could also create a pull request with @smalhotra-spirent's fix.

LeStahL commented 2 years ago

Using MSVC-2022 with c++-20 enabled resolves the problem.

SylvainFogale commented 2 years ago

While the solution is very simple, the issue cause recurring support in the popular Nlohmann repo https://github.com/nlohmann/json/discussions/3280 Indeed the generated code need to be corrected so that the to and from are not in the lib namespace, or at least in the generated header comments, please mention the possibility that the compilation error "no matching overloaded function found when implementing from_json function" relate to the choice of namespace where "to" and "from" are implemented in the autogenerated code. The combination of the quicktype tool and the Nlohmann is a very powerfull tool that should gain in popularity but we can't evaluate how many users could be discouraged at the first occurence of the issue !