JsSymbol primitive

[chrisbajorin]

Addresses #502 and JsSymbol RFC.

A few thoughts from playing with the implementation:

In JS, the Symbol() function spec allows for non-string values that can be coerced to string, e.g.
Symbol(null).description === 'null'.

Should JsSymbol::with_description() accept some trait that allows it to accept both Handle and &str (along the lines of PropertyKey)? I don't know how other folks utilize the description property. When using them in JS, I personally only use string literals since I use the description to convey meaning to other developers in the source code. I don't think I've ever used the description itself as a value, but I could see someone else using it as such.

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)?

[kjvalencik]

@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.

[chrisbajorin]

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.

[kjvalencik]

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.

[kjvalencik]

Thanks so much for working on this feature! I've left some comments, but if you want to wait for the design to solidify before making changes, that's totally reasonable!

Cheers!

[kjvalencik]

This method needs to validate napi_status.

Suggested change

	pub unsafe fn symbol(out: &mut Local, env: Env, desc: Local) {
	napi::create_symbol(env, desc, out as *mut Local);
	}
	pub unsafe fn symbol(env: Env, desc: Local) -> Local {
	let local = MaybeUninit::uninit();
	assert_eq!(napi::create_symbol(env, desc, local.as_mut_ptr()), napi::Status::Ok);
	local.assume_init();
	}

[chrisbajorin]

For this part I was definitely playing the follow-the-pattern game. Is there a reason why the other create primitives aren't validated? I can understand null/undefined/boolean as they are effectively just getters for global values, but number doesn't have any validation.

[kjvalencik]

JsNumber should be validating it since it's effectively infallible due to protections on the Rust side.

Thinking about this one a bit more, it should probably return something to indicate an error since there are valid cases where it could throw.

Maybe check if it's throwing and return None and then assert that it's ok.

[chrisbajorin]

I'm not following. I understand this part:

JsNumber should be validating it since it's effectively infallible due to protections on the Rust side.

In neon_runtime::primitive::number(), the value passed in has to be an f64, so it's inherently validated by the Rust type system.

While in neon_runtime::primitive::symbol(), desc has to be either a null pointer or a Local representing a ValueType::String. Adding the status check makes sense to me given that someone could pass in a different Local type, but I don't understand this part:

Thinking about this one a bit more, it should probably return something to indicate an error since there are valid cases where it could throw.

What does this have to do with throw? If we're in a throwing state, and this function returns Option<Local> We'd have to expect/unwrap it in JsSymbol::new_internal() in order to return Handle<JsSymbol>. If we then unwrap the None, that panic would override the current throw state which seems to defeat the point.

If it's the case that you're mistaking this function for the description getter, that one currently returns None when in a throwing state:

    let sym = cx.symbol("description");
    let opt1 = sym.description(&mut cx);
    assert!(opt1.is_some());
    cx.throw_type_error::<_, ()>("entering throw state with TypeError").unwrap_err();
    let opt2 = sym.description(&mut cx);
    assert!(opt2.is_none());

Although this is a side effect of napi::get_property() failing in neon_runtime::object::get_string() rather than explicitly checking for the throw state in JsSymbol::description().

[kjvalencik]

You're right! I was thinking this method accepts a str as a description and converting to JsString could fail, but it's already a napi_value here. 🤦

I agree, checking the status shouldn't be necessary. It might not hurt to check in a debug_assert.

[kjvalencik]

This would probably be a little simpler if we added napi_get_named_property to neon-runtime. Usually strings are user provided so we can't be guaranteed they don't contain null, but this one could be a constant.

const DESCRIPTION_KEY: &[u8] = b"description\0";

[kjvalencik]

I'd prefer if this were built into Node-API, but looks like it isn't. The best I can tell reading the ECMAScript spec, this property is guaranteed to exist and it's impossible to overwrite. @dherman is that correct?

[chrisbajorin]

FYI, when I first wrote up the RFC, node@10 was still in maintenance LTS, and symbol.prototype.description was unsupported. With this implementation, if I downgrade to 10.24, the description tests fail both the node and rust getter with:

  1) JsSymbol
       should return a JsSymbol with a description built in Rust:
     AssertionError: expected undefined to equal 'neon:description'
      at Context.<anonymous> (lib/symbols.js:8:16)

  2) JsSymbol
       should read the description property in Rust:
     AssertionError: expected undefined to equal 'neon:description'
      at Context.<anonymous> (lib/symbols.js:18:16)

neon_runtime::tag::is_string(env, local) returns false in the description function, so it returns None.

[kjvalencik]

The good news is we no longer support Node 10 and can make that simplification.

[dherman]

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.

[kjvalencik]

@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.

[chrisbajorin]

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>
}

[kjvalencik]

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.

[chrisbajorin]

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().

[kjvalencik]

@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.

[kjvalencik]

@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.

[chrisbajorin]

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.

[kjvalencik]

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!