fix: refine Symbol::For overload resolution

Signed-off-by: umuoy1 <burningdian@gmail.com>

Author: umuoy1

doc/symbol.md

@@ -49,6 +49,7 @@ Returns a `Napi::Symbol` representing a well-known `Symbol` from the
 
 ### For
 ```cpp
+static Napi::Symbol Napi::Symbol::For(napi_env env, const std::string& description);
 static Napi::Symbol Napi::Symbol::For(napi_env env, std::string_view description);
 static Napi::Symbol Napi::Symbol::For(napi_env env, const char* description);
 static Napi::Symbol Napi::Symbol::For(napi_env env, String description);
@@ -58,12 +59,16 @@ static Napi::Symbol Napi::Symbol::For(napi_env env, napi_value description);
 - `[in] env`: The `napi_env` environment in which to construct the `Napi::Symbol` object.
 - `[in] description`: The C++ string representing the `Napi::Symbol` in the global registry to retrieve.
   `description` may be any of:
-  - `std::string_view` - represents a UTF-8 string view. `std::string` values
-    are implicitly convertible to `std::string_view`.
+  - `const std::string&` - represents a UTF-8 string.
+  - `std::string_view` - represents a UTF-8 string view.
   - `const char*` - represents a UTF8 string description.
   - `String` - Node addon API String description.
   - `napi_value` - Node-API `napi_value` description.
 
+String-like arguments implicitly convertible to both `const std::string&` and
+`std::string_view` that do not have a unique best match among the non-template
+overloads are resolved through `std::string_view`.
+
 Searches in the global registry for existing symbol with the given name. If the symbol already exist it will be returned, otherwise a new symbol will be created in the registry. It's equivalent to Symbol.for() called from JavaScript.
 
 [`Napi::Name`]: ./name.md

napi-inl.h

@@ -1416,12 +1416,24 @@ inline MaybeOrValue<Symbol> Symbol::WellKnown(napi_env env,
 #endif
 }
 
+inline MaybeOrValue<Symbol> Symbol::For(napi_env env,
+                                        const std::string& description) {
+  napi_value descriptionValue = String::New(env, description);
+  return Symbol::For(env, descriptionValue);
+}
+
 inline MaybeOrValue<Symbol> Symbol::For(napi_env env,
                                         std::string_view description) {
   napi_value descriptionValue = String::New(env, description);
   return Symbol::For(env, descriptionValue);
 }
 
+template <typename T, details::enable_if_ambiguous_symbol_for_t<T&&>>
+inline MaybeOrValue<Symbol> Symbol::For(napi_env env, T&& description) {
+  std::string_view descriptionView = std::forward<T>(description);
+  return Symbol::For(env, descriptionView);
+}
+
 inline MaybeOrValue<Symbol> Symbol::For(napi_env env, const char* description) {
   napi_value descriptionValue = String::New(env, description);
   return Symbol::For(env, descriptionValue);

napi.h

@@ -20,6 +20,8 @@
 #include <chrono>
 #include <string>
 #include <string_view>
+#include <type_traits>
+#include <utility>
 #include <vector>
 
 // VS2015 RTM has bugs with constexpr, so require min of VS2015 Update 3 (known
@@ -786,6 +788,41 @@ class String : public Name {
       const;  ///< Converts a String value to a UTF-16 encoded C++ string.
 };
 
+namespace details {
+
+// This overload set must mirror the non-template Symbol::For overloads.
+struct symbol_for_overload_probe {
+  static void select(const std::string&);
+  static void select(std::string_view);
+  static void select(const char*);
+  static void select(String);
+  static void select(napi_value);
+};
+
+template <typename T, typename = void>
+struct has_unambiguous_symbol_for_overload : std::false_type {};
+
+template <typename T>
+struct has_unambiguous_symbol_for_overload<
+    T,
+    std::void_t<decltype(symbol_for_overload_probe::select(std::declval<T>()))>>
+    : std::true_type {};
+
+// Enable the template overload only for string-like arguments that have no
+// unique best match among the non-template Symbol::For overloads.
+//
+// Exclude nullptr because it matches the pointer overloads equally well and
+// cannot safely initialize a std::string_view.
+template <typename T>
+using enable_if_ambiguous_symbol_for_t =
+    std::enable_if_t<!std::is_null_pointer_v<std::decay_t<T>> &&
+                         std::is_convertible_v<T, const std::string&> &&
+                         std::is_convertible_v<T, std::string_view> &&
+                         !has_unambiguous_symbol_for_overload<T>::value,
+                     int>;
+
+}  // namespace details
+
 /// A JavaScript symbol value.
 class Symbol : public Name {
  public:
@@ -825,9 +862,17 @@ class Symbol : public Name {
   /// Get a public Symbol (e.g. Symbol.iterator).
   static MaybeOrValue<Symbol> WellKnown(napi_env, const std::string& name);
 
+  // Create a symbol in the global registry, UTF-8 Encoded cpp string
+  static MaybeOrValue<Symbol> For(napi_env env, const std::string& description);
+
   // Create a symbol in the global registry, UTF-8 encoded cpp string view
   static MaybeOrValue<Symbol> For(napi_env env, std::string_view description);
 
+  // Resolve otherwise ambiguous string-like arguments through the
+  // std::string_view overload
+  template <typename T, details::enable_if_ambiguous_symbol_for_t<T&&> = 0>
+  static MaybeOrValue<Symbol> For(napi_env env, T&& description);
+
   // Create a symbol in the global registry, C style string (null terminated)
   static MaybeOrValue<Symbol> For(napi_env env, const char* description);
 

test/symbol.cc

@@ -1,21 +1,61 @@
 #include <napi.h>
 
 #include <string_view>
+#include <utility>
 
 #include "test_helper.h"
 using namespace Napi;
 
 namespace {
 
-class StringLike {
- public:
-  explicit StringLike(const std::string& value) : _value(value) {}
+struct StringLike {
+  operator std::string() const { return "unexpected-string-key"; }
+  operator std::string_view() const { return value; }
 
-  operator std::string() const { return _value; }
-  operator std::string_view() const { return _value; }
+  std::string value;
+};
+
+struct RvalueStringLike {
+  operator std::string() && { return "unexpected-rvalue-string-key"; }
+  operator std::string_view() && { return value; }
+
+  std::string value;
+};
+
+struct StringOnlyLike {
+  operator std::string() const { return value; }
+
+  std::string value;
+};
+
+struct BothBases : std::string, std::string_view {};
+
+struct ViewAndNapiString : std::string_view, Napi::String {};
+
+struct StringReferenceLike {
+  operator std::string&() const { return stringValue; }
+  operator std::string&&() const { return std::move(stringValue); }
+  operator std::string_view() const { return viewValue; }
 
- private:
-  std::string _value;
+  mutable std::string stringValue;
+  std::string_view viewValue;
+};
+
+struct ImplicitStringViewLike {
+  operator std::string_view() const { return value; }
+
+  std::string_view value;
+};
+
+struct ExplicitStringViewLike {
+  explicit operator std::string_view() const { return value; }
+
+  std::string_view value;
+};
+
+struct ImplicitAndExplicitStringViewLike : ImplicitStringViewLike,
+                                           ExplicitStringViewLike {
+  operator std::string() const { return "unexpected-string-key"; }
 };
 
 }  // namespace
@@ -64,10 +104,59 @@ Symbol FetchSymbolFromGlobalRegistryWithStringViewKey(
 
 Symbol FetchSymbolFromGlobalRegistryWithStringLikeKey(
     const Napi::CallbackInfo& info) {
-  StringLike key(info[0].As<String>().Utf8Value());
+  StringLike key{info[0].As<String>().Utf8Value()};
   return MaybeUnwrap(Napi::Symbol::For(info.Env(), key));
 }
 
+Symbol FetchSymbolFromGlobalRegistryWithRvalueStringLikeKey(
+    const Napi::CallbackInfo& info) {
+  return MaybeUnwrap(Napi::Symbol::For(
+      info.Env(), RvalueStringLike{info[0].As<String>().Utf8Value()}));
+}
+
+Symbol FetchSymbolFromGlobalRegistryWithStringOnlyLikeKey(
+    const Napi::CallbackInfo& info) {
+  StringOnlyLike key{info[0].As<String>().Utf8Value()};
+  return MaybeUnwrap(Napi::Symbol::For(info.Env(), key));
+}
+
+Symbol FetchSymbolFromGlobalRegistryWithBothBasesKey(
+    const Napi::CallbackInfo& info) {
+  std::string value = info[0].As<String>().Utf8Value();
+  BothBases key;
+  static_cast<std::string&>(key) = "unexpected-string-key";
+  static_cast<std::string_view&>(key) = value;
+  return MaybeUnwrap(Symbol::For(info.Env(), key));
+}
+
+Symbol FetchSymbolFromGlobalRegistryWithViewAndNapiStringKey(
+    const Napi::CallbackInfo& info) {
+  Env env = info.Env();
+  std::string value = info[0].As<String>().Utf8Value();
+  ViewAndNapiString key;
+  static_cast<std::string_view&>(key) = value;
+  static_cast<Napi::String&>(key) =
+      Napi::String::New(env, "unexpected-napi-string-key");
+  return MaybeUnwrap(Symbol::For(env, key));
+}
+
+Symbol FetchSymbolFromGlobalRegistryWithStringReferenceKey(
+    const Napi::CallbackInfo& info) {
+  std::string value = info[0].As<String>().Utf8Value();
+  StringReferenceLike key{"unexpected-string-reference-key", value};
+  return MaybeUnwrap(Symbol::For(info.Env(), key));
+}
+
+Symbol FetchSymbolFromGlobalRegistryWithImplicitViewKey(
+    const Napi::CallbackInfo& info) {
+  std::string value = info[0].As<String>().Utf8Value();
+  ImplicitAndExplicitStringViewLike key;
+  static_cast<ImplicitStringViewLike&>(key).value = value;
+  static_cast<ExplicitStringViewLike&>(key).value =
+      "unexpected-explicit-string-view-key";
+  return MaybeUnwrap(Symbol::For(info.Env(), key));
+}
+
 Symbol FetchSymbolFromGlobalRegistryWithCKey(const Napi::CallbackInfo& info) {
   String cppStringKey = info[0].As<String>();
   return MaybeUnwrap(
@@ -106,6 +195,18 @@ Object InitSymbol(Env env) {
       Function::New(env, FetchSymbolFromGlobalRegistryWithStringViewKey);
   exports["getSymbolFromGlobalRegistryWithStringLikeKey"] =
       Function::New(env, FetchSymbolFromGlobalRegistryWithStringLikeKey);
+  exports["getSymbolFromGlobalRegistryWithRvalueStringLikeKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithRvalueStringLikeKey);
+  exports["getSymbolFromGlobalRegistryWithStringOnlyLikeKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithStringOnlyLikeKey);
+  exports["getSymbolFromGlobalRegistryWithBothBasesKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithBothBasesKey);
+  exports["getSymbolFromGlobalRegistryWithViewAndNapiStringKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithViewAndNapiStringKey);
+  exports["getSymbolFromGlobalRegistryWithStringReferenceKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithStringReferenceKey);
+  exports["getSymbolFromGlobalRegistryWithImplicitViewKey"] =
+      Function::New(env, FetchSymbolFromGlobalRegistryWithImplicitViewKey);
   exports["testUndefinedSymbolCanBeCreated"] =
       Function::New(env, TestUndefinedSymbolsCanBeCreated);
   exports["testNullSymbolCanBeCreated"] =

test/symbol.js

@@ -42,6 +42,7 @@ function test (binding) {
     const symbTwo = fetchFunction(symbol);
     assert(symbOne && symbTwo);
     assert(symbOne === symbTwo);
+    assert(symbOne === Symbol.for(symbol));
   }
 
   assertCanCreateSymbol('testing');
@@ -55,7 +56,27 @@ function test (binding) {
   assertCanCreateOrFetchGlobalSymbols('data', binding.symbol.getSymbolFromGlobalRegistry);
   assertCanCreateOrFetchGlobalSymbols('CppKey', binding.symbol.getSymbolFromGlobalRegistryWithCppKey);
   assertCanCreateOrFetchGlobalSymbols('StringViewKey', binding.symbol.getSymbolFromGlobalRegistryWithStringViewKey);
-  assertCanCreateOrFetchGlobalSymbols('StringLikeKey', binding.symbol.getSymbolFromGlobalRegistryWithStringLikeKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'StringLikeKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithStringLikeKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'RvalueStringLikeKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithRvalueStringLikeKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'StringOnlyLikeKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithStringOnlyLikeKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'BothBasesKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithBothBasesKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'ViewAndNapiStringKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithViewAndNapiStringKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'StringReferenceKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithStringReferenceKey);
+  assertCanCreateOrFetchGlobalSymbols(
+    'ImplicitViewKey',
+    binding.symbol.getSymbolFromGlobalRegistryWithImplicitViewKey);
   assertCanCreateOrFetchGlobalSymbols('CKey', binding.symbol.getSymbolFromGlobalRegistryWithCKey);
 
   assert(binding.symbol.createNewSymbolWithNoArgs() === undefined);