src: use better return types in KVStore

targos · targos · commit 8f1fa03c1308 · 2024-08-26T15:33:35.000+02:00
- Use `v8::Maybe<void>` instead of `v8::Maybe<bool>` and handle error from `AssignFromObject`. - An empty `v8::Maybe` is supposed to be returned when an exception is pending. Use `std::optional` instead to indicate a missing value in `Get(key)`. PR-URL: nodejs#54539 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
@@ -472,7 +472,7 @@ void StartProfilers(Environment* env) {
   }, env);
 
   std::string coverage_str =
-      env->env_vars()->Get("NODE_V8_COVERAGE").FromMaybe(std::string());
+      env->env_vars()->Get("NODE_V8_COVERAGE").value_or(std::string());
   if (!coverage_str.empty() || env->options()->test_runner_coverage) {
     CHECK_NULL(env->coverage_connection());
     env->set_coverage_connection(std::make_unique<V8CoverageConnection>(env));
diff --git a/src/node_credentials.cc b/src/node_credentials.cc
@@ -91,7 +91,10 @@ bool SafeGetenv(const char* key,
     env_vars = per_process::system_environment;
   }
 
-  return env_vars->Get(key).To(text);
+  std::optional<std::string> value = env_vars->Get(key);
+  if (!value.has_value()) return false;
+  *text = value.value();
+  return true;
 }
 
 static void SafeGetenv(const FunctionCallbackInfo<Value>& args) {
diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc
@@ -52,7 +52,7 @@ void Dotenv::SetEnvironment(node::Environment* env) {
 
     auto existing = env->env_vars()->Get(key.data());
 
-    if (existing.IsNothing()) {
+    if (!existing.has_value()) {
       env->env_vars()->Set(
           isolate,
           v8::String::NewFromUtf8(
diff --git a/src/node_env_var.cc b/src/node_env_var.cc
@@ -6,6 +6,7 @@
 #include "node_process-inl.h"
 
 #include <time.h>  // tzset(), _tzset()
+#include <optional>
 
 namespace node {
 using v8::Array;
@@ -19,6 +20,7 @@ using v8::Integer;
 using v8::Intercepted;
 using v8::Isolate;
 using v8::Just;
+using v8::JustVoid;
 using v8::Local;
 using v8::Maybe;
 using v8::MaybeLocal;
@@ -38,7 +40,7 @@ using v8::Value;
 class RealEnvStore final : public KVStore {
  public:
   MaybeLocal<String> Get(Isolate* isolate, Local<String> key) const override;
-  Maybe<std::string> Get(const char* key) const override;
+  std::optional<std::string> Get(const char* key) const override;
   void Set(Isolate* isolate, Local<String> key, Local<String> value) override;
   int32_t Query(Isolate* isolate, Local<String> key) const override;
   int32_t Query(const char* key) const override;
@@ -49,7 +51,7 @@ class RealEnvStore final : public KVStore {
 class MapKVStore final : public KVStore {
  public:
   MaybeLocal<String> Get(Isolate* isolate, Local<String> key) const override;
-  Maybe<std::string> Get(const char* key) const override;
+  std::optional<std::string> Get(const char* key) const override;
   void Set(Isolate* isolate, Local<String> key, Local<String> value) override;
   int32_t Query(Isolate* isolate, Local<String> key) const override;
   int32_t Query(const char* key) const override;
@@ -101,7 +103,7 @@ void DateTimeConfigurationChangeNotification(
   }
 }
 
-Maybe<std::string> RealEnvStore::Get(const char* key) const {
+std::optional<std::string> RealEnvStore::Get(const char* key) const {
   Mutex::ScopedLock lock(per_process::env_var_mutex);
 
   size_t init_sz = 256;
@@ -116,19 +118,19 @@ Maybe<std::string> RealEnvStore::Get(const char* key) const {
   }
 
   if (ret >= 0) {  // Env key value fetch success.
-    return Just(std::string(*val, init_sz));
+    return std::string(*val, init_sz);
   }
 
-  return Nothing<std::string>();
+  return std::nullopt;
 }
 
 MaybeLocal<String> RealEnvStore::Get(Isolate* isolate,
                                      Local<String> property) const {
   node::Utf8Value key(isolate, property);
-  Maybe<std::string> value = Get(*key);
+  std::optional<std::string> value = Get(*key);
 
-  if (value.IsJust()) {
-    std::string val = value.FromJust();
+  if (value.has_value()) {
+    std::string val = value.value();
     return String::NewFromUtf8(
         isolate, val.data(), NewStringType::kNormal, val.size());
   }
@@ -229,17 +231,17 @@ std::shared_ptr<KVStore> KVStore::Clone(Isolate* isolate) const {
   return copy;
 }
 
-Maybe<std::string> MapKVStore::Get(const char* key) const {
+std::optional<std::string> MapKVStore::Get(const char* key) const {
   Mutex::ScopedLock lock(mutex_);
   auto it = map_.find(key);
-  return it == map_.end() ? Nothing<std::string>() : Just(it->second);
+  return it == map_.end() ? std::nullopt : std::make_optional(it->second);
 }
 
 MaybeLocal<String> MapKVStore::Get(Isolate* isolate, Local<String> key) const {
   Utf8Value str(isolate, key);
-  Maybe<std::string> value = Get(*str);
-  if (value.IsNothing()) return Local<String>();
-  std::string val = value.FromJust();
+  std::optional<std::string> value = Get(*str);
+  if (!value.has_value()) return MaybeLocal<String>();
+  std::string val = value.value();
   return String::NewFromUtf8(
       isolate, val.data(), NewStringType::kNormal, val.size());
 }
@@ -291,30 +293,29 @@ std::shared_ptr<KVStore> KVStore::CreateMapKVStore() {
   return std::make_shared<MapKVStore>();
 }
 
-Maybe<bool> KVStore::AssignFromObject(Local<Context> context,
+Maybe<void> KVStore::AssignFromObject(Local<Context> context,
                                       Local<Object> entries) {
   Isolate* isolate = context->GetIsolate();
   HandleScope handle_scope(isolate);
   Local<Array> keys;
   if (!entries->GetOwnPropertyNames(context).ToLocal(&keys))
-    return Nothing<bool>();
+    return Nothing<void>();
   uint32_t keys_length = keys->Length();
   for (uint32_t i = 0; i < keys_length; i++) {
     Local<Value> key;
-    if (!keys->Get(context, i).ToLocal(&key))
-      return Nothing<bool>();
+    if (!keys->Get(context, i).ToLocal(&key)) return Nothing<void>();
     if (!key->IsString()) continue;
 
     Local<Value> value;
     Local<String> value_string;
     if (!entries->Get(context, key).ToLocal(&value) ||
         !value->ToString(context).ToLocal(&value_string)) {
-      return Nothing<bool>();
+      return Nothing<void>();
     }
 
     Set(isolate, key.As<String>(), value_string);
   }
-  return Just(true);
+  return JustVoid();
 }
 
 // TODO(bnoordhuis) Not super efficient but called infrequently. Not worth
diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -531,8 +531,12 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
   } else if (args[1]->IsObject()) {
     // User provided env.
     env_vars = KVStore::CreateMapKVStore();
-    env_vars->AssignFromObject(isolate->GetCurrentContext(),
-                               args[1].As<Object>());
+    if (env_vars
+            ->AssignFromObject(isolate->GetCurrentContext(),
+                               args[1].As<Object>())
+            .IsNothing()) {
+      return;
+    }
   } else {
     // Env is shared.
     env_vars = env->env_vars();
@@ -542,21 +546,21 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
     per_isolate_opts.reset(new PerIsolateOptions());
 
     HandleEnvOptions(per_isolate_opts->per_env, [&env_vars](const char* name) {
-      return env_vars->Get(name).FromMaybe("");
+      return env_vars->Get(name).value_or("");
     });
 
 #ifndef NODE_WITHOUT_NODE_OPTIONS
-    std::string node_options;
-    if (env_vars->Get("NODE_OPTIONS").To(&node_options)) {
+    std::optional<std::string> node_options = env_vars->Get("NODE_OPTIONS");
+    if (node_options.has_value()) {
       std::vector<std::string> errors{};
       std::vector<std::string> env_argv =
-          ParseNodeOptionsEnvVar(node_options, &errors);
+          ParseNodeOptionsEnvVar(node_options.value(), &errors);
       // [0] is expected to be the program name, add dummy string.
       env_argv.insert(env_argv.begin(), "");
       std::vector<std::string> invalid_args{};
 
-      std::string parent_node_options;
-      USE(env->env_vars()->Get("NODE_OPTIONS").To(&parent_node_options));
+      std::optional<std::string> parent_node_options =
+          env->env_vars()->Get("NODE_OPTIONS");
 
       // If the worker code passes { env: { ...process.env, ... } } or
       // the NODE_OPTIONS is otherwise character-for-character equal to the
diff --git a/src/util.h b/src/util.h
@@ -40,6 +40,7 @@
 #include <bit>
 #include <limits>
 #include <memory>
+#include <optional>
 #include <set>
 #include <string>
 #include <string_view>
@@ -306,7 +307,7 @@ class KVStore {
 
   virtual v8::MaybeLocal<v8::String> Get(v8::Isolate* isolate,
                                          v8::Local<v8::String> key) const = 0;
-  virtual v8::Maybe<std::string> Get(const char* key) const = 0;
+  virtual std::optional<std::string> Get(const char* key) const = 0;
   virtual void Set(v8::Isolate* isolate,
                    v8::Local<v8::String> key,
                    v8::Local<v8::String> value) = 0;
@@ -317,7 +318,7 @@ class KVStore {
   virtual v8::Local<v8::Array> Enumerate(v8::Isolate* isolate) const = 0;
 
   virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const;
-  virtual v8::Maybe<bool> AssignFromObject(v8::Local<v8::Context> context,
+  virtual v8::Maybe<void> AssignFromObject(v8::Local<v8::Context> context,
                                            v8::Local<v8::Object> entries);
   v8::Maybe<bool> AssignToObject(v8::Isolate* isolate,
                                  v8::Local<v8::Context> context,

Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,10 @@ bool SafeGetenv(const char* key,`
`91`	`91`	`env_vars = per_process::system_environment;`
`92`	`92`	`}`
`93`	`93`
`94`		`- return env_vars->Get(key).To(text);`
	`94`	`+ std::optional<std::string> value = env_vars->Get(key);`
	`95`	`+ if (!value.has_value()) return false;`
	`96`	`+ *text = value.value();`
	`97`	`+ return true;`
`95`	`98`	`}`
`96`	`99`
`97`	`100`	`static void SafeGetenv(const FunctionCallbackInfo<Value>& args) {`