@chrisbajorin I might not have a chance to review the code right away, but I wanted to answer some of your questions with my thoughts quickly:

Should JsSymbol::with_description() accept some trait that allows it to accept both Handle and &str (along the lines of PropertyKey)?

This is a good idea. It would be nice if a user could directly use either a AsRef<str> or a JsString. If we only pick one, I would probably favor JsString since it's slightly more flexible. The downside of this is that because of ownership on Context it would require two lines of code instead of a single line and that's not very ergonomic.

// Compiler error because `Context` is still borrowed by `cx.string` when calling `with_description`
JsSymbol::with_description(&mut cx, cx.string("MySymbol"));

let description = cx.string("MySymbol");
JsSymbol::with_description(&mut cx, description);

But, since I can't think of a use case for frequently creating symbols or using very large descriptions, the overhead of converting to &str and back, might be fine. What do you think?

If we do have an API that can take a handle, I do think it should be restricted to just JsString and not all JsValue for a couple of reasons:

• Users can always manually perform the coercion: https://docs.rs/neon/0.8.3/neon/types/trait.Value.html#method.to_string
• Coercion may throw an exception and that would require making this API failable. This would be awkward ergonomics since in most cases it will never fail.

Along that same line of thought, if people do use the description as a value, should JsSymbol::description() return an Option<Handle<'a, JsString>> instead of Option<String> (or even Handle<'a, JsValue> that could be either JsUndefined or JsString)?

I like returning Option<Handle<'a, JsString>>. It's lower overhead if the conversion isn't necessary and can still be chained with .map(|v| v.value(&mut cx)) to get an Option<String>. I think it's valuable to perform the undefined check upfront to give a more constrained type than JsValue that needs to be downcast.

It would be nice if a user could directly use either a AsRef<str> or a JsString.

One solution is to have JsSymbol::with_description() accept Handle<JsString>, but have cx.symbol() take AsRef<str>. The two-liner could then just be the implementation for cx.symbol() itself:

    fn symbol<S: AsRef<str>>(&mut self, desc: S) -> Handle<'a, JsSymbol> {
        let description = self.string(desc);
        JsSymbol::with_description(self, description)
    }

However, this would be a deviation from the rest of the context helper methods as direct wrappers around the underlying type constructor signature. That being said, because of the optional nature of the description property, it's already a deviation anyway.

Alternatively, have both functions accept something that implements SymbolDescription:

pub trait SymbolDescription {
    fn to_description_local(self, env: Env) -> Option<raw::Local>;
}

impl <'a>SymbolDescription for &'a str {
    fn to_description_local(self, env: Env) -> Option<raw::Local> {
        JsString::new_internal(env, self).map(|s| s.to_raw())
    }
}

impl <'a>SymbolDescription for Handle<'a, JsString> {
    fn to_description_local(self, _: Env) -> Option<raw::Local> {
        Some(self.to_raw())
    }
}

Although after writing it up, I'm not sure if it's needless complexity just to avoid writing that extra line of code (and this naive implementation would silently ignore a StringOverflow by converting it to an undefined description).

Glancing through my own JS code, I can't find a single instance where I'm dynamically generating unique symbols. Given that symbols can't be converted to NapiRef (although this may change), I suspect that I'll likely still be declaring my library symbols in index.js, and passing them into a neon function whenever they are needed. Within that context, my primary motivation for the PR is definitely for the typeguard, so I can't say I have strong opinions on the constructor interface.

If we do have an API that can take a handle, I do think it should be restricted to just JsString and not all JsValue

That makes sense.

I like returning Option<Handle<'a, JsString>>

As do I. It would also allow me to revert the my edit to JsString, making the feature purely additive.

I really like your suggestion of having some asymmetry between cx.symbol(..) and JsSymbol. In general, the Context methods are meant to be the most common constructors and not necessarily one-to-one with new.

That seems like a really good balance between flexibility and convenience.

Not being able to put a symbol in a persistent reference is unfortunate. That does limit the usability quite a bit but, I can't think of any way around it without it changing upstream.

Maybe a workaround (at least until/unless they fix NapiRef to work with symbols) would be for impl<T: JsSymbol> Root<T> to have specialized logic to wrap the symbol in a wrapper object before putting it in the NapiRef? It's maybe a little tacky since it just duck-types the methods of impl<T: Object> Root<T> as a kind of ad-hoc specialization.

@dherman I really like that idea and it should work. We can always optimize it later without making any breaking changes to the Neon API.

The very thing that keeps napi_ref from working (Symbol isn't an object), also allows this from being an overlapping impl in Rust.

In order to keep this syntax consistent:

let root1 = cx.argument::<JsObject>(0)?.root(&mut cx);
let root2 = cx.argument::<JsSymbol>(1)?.root(&mut cx);

the Object/PropertyKey API is fallible, so the Root::new() function would have to panic if set_from fails on the object wrapper:

impl Root<JsSymbol> {

    const PROPERTY_KEY: &'static str = "internal";
    
    pub fn new<'a, C: Context<'a>>(cx: &mut C, symbol: &JsSymbol) -> Self {
        let value = JsObject::new_internal(cx.env());
        unsafe {
            let mut result = false;
            if !Root::PROPERTY_KEY.set_from(cx, &mut result, value.to_raw(), symbol.to_raw()) {
                // panic?
            }
        }

        let env = cx.env().to_raw();
        let internal = unsafe { reference::new(env, value.to_raw()) };

        Self {
            internal: Some(NapiRef(internal as *mut _)),
            #[cfg(feature = "napi-6")]
            drop_queue: InstanceData::drop_queue(cx),
            _phantom: PhantomData,
        }
    }
}

Alternatively, we could make the JsSymbol root implementations all fallible, but this expands the API, and I suspect you'd have to point folks to this discussion just to make sense of it:

impl JsSymbol {
    // ...
    pub fn try_root<'a, C: Context<'a>>(&self, cx: &mut C) -> NeonResult<Root<Self>>
}

impl Root<JsSymbol> {

    pub fn try_new<'a, C: Context<'a>>(cx: &mut C, symbol: &JsSymbol) -> NeonResult<Self>
  
    pub fn clone<'a, C: Context<'a>>(&self, cx: &mut C) -> Self
    
    pub fn drop<'a, C: Context<'a>>(self, cx: &mut C)

    pub fn try_into_inner<'a, C: Context<'a>>(self, cx: &mut C) -> JsResult<'a, JsSymbol>

    pub fn try_to_inner<'a, C: Context<'a>>(&self, cx: &mut C) -> JsResult<'a, JsSymbol>
}

It should be safe to panic there. The only way these API can fail is if the key can't be allocated because it exceeds the max length (we statically know the length and this won't happen) or if there is a getter defined that throws. We also know that can't happen.

Relating to our discussion about throwing, It seems like napi::create_reference() succeeds while in a throw state, whereas the object property setters return Status::PendingException. So if your code is:

    let obj: Handle<JsObject> = cx.argument(0)?;
    cx.throw_type_error::<_, ()>("entering a throwing state with TypeError").unwrap_err();
    let root = obj.root(&mut cx);

this would throw a TypeError, but if you replace that JsObject with JsSymbol, then the TypeError would be overridden by whatever panic is in that property setter in Root::<JsSymbol>::new().

@chrisbajorin This is because most things will panic if in a throwing state and the unwind gets caught and converted back to an exception. It's an unfortunate compromise in the design due to limitations in Rust.

Ideally, it would be statically impossible to call methods that can't be called while in a throwing state. However, to do that gets really unergonomic because Context would need to be threaded through the Ok results:

let (cx, obj) = cx.argument::<JsObject>(0)?;

Instead, Neon simply suggests that users are careful never to try to use Context when in a throwing state. Another option would be to pre-emptively return Err(Throw) whenever we are already throwing, but then that makes nearly every single API return Result when most are infallible.

In retrospect, it may have been better for all APIs to return Result, but that's not an API change we would take likely.

tl;dr -- What you shared is expected.

@chrisbajorin Sorry for letting this sit for so long! It had slipped my mind. Are you still interested in finishing this? I think it's pretty close.

I'm terribly sorry about that, I was moving last year and my computer sat in a box for a few months. This completely slipped my mind. I'll take a look this weekend and see if I can get it finished up.

Let me know if you have any questions about the conflicts. A bunch of files moved and were changed when I deleted the legacy backend. Cheers!