Skip to content

fix: resolve Symbol::For overload ambiguity - #1742

Open
umuoy1 wants to merge 4 commits into
nodejs:mainfrom
umuoy1:fix/symbol-for-overload-ambiguity
Open

fix: resolve Symbol::For overload ambiguity#1742
umuoy1 wants to merge 4 commits into
nodejs:mainfrom
umuoy1:fix/symbol-for-overload-ambiguity

Conversation

@umuoy1

@umuoy1 umuoy1 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Symbol::For an ambiguous for types that can convert to both std::string and std::string_view.

Instead of removing the existing std::string overload, this keep it so string-only wrappers continue to work and adds a constrained forwarding overload for the ambiguous case. Calls that already have a unique best match keep using the existing overloads.

Fixes #1741

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

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.68%. Comparing base (a815332) to head (20ef695).

Files with missing lines Patch % Lines
napi-inl.h 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
- Coverage   63.69%   63.68%   -0.02%     
==========================================
  Files           3        3              
  Lines        2063     2065       +2     
  Branches      730      731       +1     
==========================================
+ Hits         1314     1315       +1     
  Misses        162      162              
- Partials      587      588       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread test/symbol.cc
umuoy1 added 2 commits August 9, 2026 15:51
Signed-off-by: umuoy1 <burningdian@gmail.com>
Signed-off-by: umuoy1 <burningdian@gmail.com>
Comment thread napi.h Outdated
Signed-off-by: umuoy1 <burningdian@gmail.com>
Comment thread napi.h
};

template <typename T, typename = void>
struct has_unambiguous_symbol_for_overload : std::false_type {};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct has_unambiguous_symbol_for_overload : std::false_type {};
struct has_unambiguous_string_convertible_overload : std::false_type {};

Comment thread napi.h
struct has_unambiguous_symbol_for_overload : std::false_type {};

template <typename T>
struct has_unambiguous_symbol_for_overload<

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct has_unambiguous_symbol_for_overload<
struct has_unambiguous_string_convertible_overload<

Comment thread napi.h
// 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 =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using enable_if_ambiguous_symbol_for_t =
using enable_if_ambiguous_string_convertible_t =

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

Ambiguity on Symbol::For overload between std::string and std::string_view convertible

3 participants