JSON-RPC make id explicit (#2030)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Use PersistantJSONValue for id storage.

* Make id handling explicit - so can make message processing disjoint from receiving order.

* Fix some issues on linux with templates.

* Fix typo.

* Fix call not passing id for JSON-RPC.

* Simplify getting persistent id from JSONRPCConnection.

2 files changed, 44 insertions, 43 deletions

diff --git a/source/compiler-core/slang-json-rpc-connection.cpp b/source/compiler-core/slang-json-rpc-connection.cpp
index 20283070b..d0ccfa4e9 100644
--- a/source/compiler-core/slang-json-rpc-connection.cpp
+++ b/source/compiler-core/slang-json-rpc-connection.cpp
@@ -47,7 +47,7 @@ bool JSONRPCConnection::isActive()
     return m_connection->isActive() && (m_process == nullptr || !m_process->isTerminated());
 }
 
-JSONValue JSONRPCConnection::getMessageId()
+JSONValue JSONRPCConnection::getCurrentMessageId()
 {
     SLANG_ASSERT(hasMessage());
     return JSONRPCUtil::getId(&m_container, m_jsonRoot);
@@ -103,40 +103,28 @@ SlangResult JSONRPCConnection::sendRPC(const RttiInfo* rttiInfo, const void* dat
     return m_connection->write(builder.getBuffer(), builder.getLength());
 }
 
-SlangResult JSONRPCConnection::sendError(JSONRPC::ErrorCode code)
+SlangResult JSONRPCConnection::sendError(JSONRPC::ErrorCode code, const JSONValue& id)
 {
-    return sendError(code, m_diagnosticSink.outputBuffer.getUnownedSlice());
+    return sendError(code, m_diagnosticSink.outputBuffer.getUnownedSlice(), id);
 }
 
-SlangResult JSONRPCConnection::sendError(JSONRPC::ErrorCode errorCode, const UnownedStringSlice& msg)
+SlangResult JSONRPCConnection::sendError(JSONRPC::ErrorCode errorCode, const UnownedStringSlice& msg, const JSONValue& id)
 {
     JSONRPCErrorResponse errorResponse;
     errorResponse.error.code = Int(errorCode);
     errorResponse.error.message = msg;
+    errorResponse.id = id;
 
-    // TODO(JS):
-    // This is only appropriate if the sendError is for the current input message.
-    // We might want to add function that the client uses, which take the id as a parameter.
-
-    if (m_jsonRoot.isValid())
-    {
-        errorResponse.id = JSONRPCUtil::getId(&m_container, m_jsonRoot);
-    }
-    else
-    {
-        // If we don't have valid json, we set the id to be null per the spec
-        errorResponse.id = JSONValue::makeNull();
-    }
-
+    
     return sendRPC(&errorResponse);
 }
 
-SlangResult JSONRPCConnection::toNativeOrSendError(const JSONValue& value, const RttiInfo* info, void* dst)
+SlangResult JSONRPCConnection::toNativeOrSendError(const JSONValue& value, const RttiInfo* info, void* dst, const JSONValue& id)
 {
     m_diagnosticSink.outputBuffer.Clear();
     if (SLANG_FAILED(JSONRPCUtil::convertToNative(&m_container, value, &m_diagnosticSink, info, dst)))
     {
-        return sendError(JSONRPC::ErrorCode::InvalidRequest);
+        return sendError(JSONRPC::ErrorCode::InvalidRequest, id);
     }
     return SLANG_OK;
 }
@@ -207,7 +195,8 @@ SlangResult JSONRPCConnection::tryReadMessage()
         m_connection->consumeContent();
         if (SLANG_FAILED(res))
         {
-            return sendError(JSONRPC::ErrorCode::ParseError);
+            // if we can't parse JSON, we return with id of 'null' as per the standard
+            return sendError(JSONRPC::ErrorCode::ParseError, JSONValue::makeNull());
         }
     }
 
@@ -248,7 +237,7 @@ SlangResult JSONRPCConnection::getMessageOrSendError(const RttiInfo* rttiInfo, v
     const auto res = getMessage(rttiInfo, out);
     if (SLANG_FAILED(res))
     {
-        return sendError(JSONRPC::ErrorCode::ParseError);
+        return sendError(JSONRPC::ErrorCode::ParseError, getCurrentMessageId());
     }
     return res;
 }
@@ -278,7 +267,7 @@ SlangResult JSONRPCConnection::getRPCOrSendError(const RttiInfo* rttiInfo, void*
     const auto res = getRPC(rttiInfo, out);
     if (SLANG_FAILED(res))
     {
-        return sendError(JSONRPC::ErrorCode::ParseError);
+        return sendError(JSONRPC::ErrorCode::ParseError, getCurrentMessageId());
     }
     return res;
 }
diff --git a/source/compiler-core/slang-json-rpc-connection.h b/source/compiler-core/slang-json-rpc-connection.h
index 730635493..b566140ef 100644
--- a/source/compiler-core/slang-json-rpc-connection.h
+++ b/source/compiler-core/slang-json-rpc-connection.h
@@ -15,12 +15,17 @@
 
 namespace Slang {
 
-/* TODO(JS):
+/* A type to handle communication via the JSON-RPC protocol.
 
-It's not really clear here, how to handle id handling. If a server can receive multiple messages, and then later send responses we need
-a way to queue up ids. This is complicated by JSONValue is only valid whilst the backing JSONContainer holds it.
+Uses Rtti to be able to convert between native and JSON types.
+Provides methods that work on the JSON-RPC protocol, these methods contain 'RPC' and can only
+use the JSON-RPC protocol types. These types will hold items that can vary (like parameters)
+in JSONValue parameters. Code can use regular JSON functions to access/process.
 
-We probably want to create some kind of Variant that can hold all of these values that can hold state independently.
+Doing conversions to native types and JSON manually can be a fairly monotonous task. To avoid this
+effort Rtti and JSON<->Rtti conversions can be used. For example sendCall will send a JSON-RPC 'call' method,
+with the parameters being converted from some native type. For this to work the type T must be determinable
+via GetRttiType<T>, and T must only contain types that JSON<->Rtti conversion supports. 
 */
 class JSONRPCConnection : public RefObject
 {
@@ -37,12 +42,12 @@ public:
     void disconnect();
 
         /// Convert value to dst. Will write response on fails
-    SlangResult toNativeOrSendError(const JSONValue& value, const RttiInfo* info, void* dst);
+    SlangResult toNativeOrSendError(const JSONValue& value, const RttiInfo* info, void* dst, const JSONValue& id);
     template <typename T>
-    SlangResult toNativeOrSendError(const JSONValue& value, T* data) { return toNativeOrSendError(value, GetRttiInfo<T>::get(), data); }
+    SlangResult toNativeOrSendError(const JSONValue& value, T* data, const JSONValue& id) { return toNativeOrSendError(value, GetRttiInfo<T>::get(), data, id); }
 
     template <typename T>
-    SlangResult toValidNativeOrSendError(const JSONValue& value, T* data);
+    SlangResult toValidNativeOrSendError(const JSONValue& value, T* data, const JSONValue& id);
 
         /// Send a RPC response (ie should only be one of the JSONRPC classes)
     SlangResult sendRPC(const RttiInfo* info, const void* data);
@@ -50,8 +55,8 @@ public:
     SlangResult sendRPC(const T* data) { return sendRPC(GetRttiInfo<T>::get(), (const void*)data);  }
 
         /// Send an error
-    SlangResult sendError(JSONRPC::ErrorCode code);
-    SlangResult sendError(JSONRPC::ErrorCode errorCode, const UnownedStringSlice& msg);
+    SlangResult sendError(JSONRPC::ErrorCode code, const JSONValue& id);
+    SlangResult sendError(JSONRPC::ErrorCode errorCode, const UnownedStringSlice& msg, const JSONValue& id);
 
         /// Send a call
         /// If no id is needed, id can just be invalid 
@@ -62,8 +67,8 @@ public:
     SlangResult sendCall(const UnownedStringSlice& method, const JSONValue& id = JSONValue());
 
     template <typename T>
-    SlangResult sendResult(const T* result, const JSONValue& id = JSONValue()) { return sendResult(GetRttiInfo<T>::get(), (const void*)result, id); }
-    SlangResult sendResult(const RttiInfo* rttiInfo, const void* result, const JSONValue& id = JSONValue());
+    SlangResult sendResult(const T* result, const JSONValue& id) { return sendResult(GetRttiInfo<T>::get(), (const void*)result, id); }
+    SlangResult sendResult(const RttiInfo* rttiInfo, const void* result, const JSONValue& id);
 
         /// Try to read a message. Will return if message is not available.
     SlangResult tryReadMessage();
@@ -106,11 +111,18 @@ public:
     bool isActive();
 
         /// Get the id of the current message
-    JSONValue getMessageId();
+    JSONValue getCurrentMessageId();
 
         /// Get the diagnostic sink. Can queue up errors before sending an error
     DiagnosticSink* getSink() { return &m_diagnosticSink;  }
 
+        /// Get the container
+    JSONContainer* getContainer() { return &m_container;  }
+
+        /// Turn a value into a persistant value. This will also remove any sourceLoc under the assumption that it's highly likely
+        /// it will become invalid in most usage scenarios. 
+    PersistentJSONValue getPersistentValue(const JSONValue& value) { return PersistentJSONValue(value, &m_container, SourceLoc()); }
+
         /// Dtor
     ~JSONRPCConnection() { disconnect(); }
 
@@ -119,14 +131,14 @@ public:
 
 protected:
     RefPtr<Slang::Process> m_process;                       ///< Backing process (optional)
-    RefPtr<Slang::HTTPPacketConnection> m_connection;
+    RefPtr<Slang::HTTPPacketConnection> m_connection;       ///< The underlying 'transport' connection, whilst HTTP currently doesn't have to be 
 
-    DiagnosticSink m_diagnosticSink;
+    DiagnosticSink m_diagnosticSink;                        ///< Holds any diagnostics typically generated by parsing JSON, producing JSON from native types
 
-    SourceManager m_sourceManager;
-    JSONContainer m_container;
+    SourceManager m_sourceManager;                          ///< Holds the JSON text for current message/output. Is cleared regularly.
+    JSONContainer m_container;                              ///< Holds the backing memory for jsonMemory, and used when converting input into output JSON
 
-    JSONValue m_jsonRoot;
+    JSONValue m_jsonRoot;                                   ///< The root JSON value for the currently read message. 
 
         /// Default timeout is 10 seconds
     Int m_timeOutInMs = 10 * 1000;
@@ -136,11 +148,11 @@ protected:
 
 // ---------------------------------------------------------------------------
 template <typename T>
-SlangResult JSONRPCConnection::toValidNativeOrSendError(const JSONValue& value, T* data)
+SlangResult JSONRPCConnection::toValidNativeOrSendError(const JSONValue& value, T* data, const JSONValue& id)
 {
     const RttiInfo* rttiInfo = GetRttiInfo<T>::get();
 
-    SLANG_RETURN_ON_FAIL(toNativeOrSendError(value, rttiInfo, (void*)data));
+    SLANG_RETURN_ON_FAIL(toNativeOrSendError(value, rttiInfo, (void*)data, id));
     if (!data->isValid())
     {
         // If it has a name add validation info
@@ -150,7 +162,7 @@ SlangResult JSONRPCConnection::toValidNativeOrSendError(const JSONValue& value,
             m_diagnosticSink.diagnose(SourceLoc(), JSONDiagnostics::argsAreInvalid, namedRttiInfo->m_name);
         }
 
-        return sendError(JSONRPC::ErrorCode::InvalidRequest);
+        return sendError(JSONRPC::ErrorCode::InvalidRequest, id);
     }
     return SLANG_OK;
 }