From 0372dfc18a37485bf179147fe67f59def3e656a4 Mon Sep 17 00:00:00 2001 From: Chris Bajorin <7016796+chrisbajorin@users.noreply.github.com> Date: Tue, 8 Dec 2020 18:56:15 -0500 Subject: [PATCH 1/3] Symbol primitive --- text/0000-symbol-primitive.md | 120 ++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 text/0000-symbol-primitive.md diff --git a/text/0000-symbol-primitive.md b/text/0000-symbol-primitive.md new file mode 100644 index 0000000..960f503 --- /dev/null +++ b/text/0000-symbol-primitive.md @@ -0,0 +1,120 @@ +- Feature Name: symbol_primitive +- Start Date: 2020-12-08 +- RFC PR: +- Neon Issue: [#502](https://github.com/neon-bindings/neon/issues/502) + +# Summary +[summary]: #summary + +This RFC proposes an implementation for the `symbol` primitive: https://www.ecma-international.org/ecma-262/11.0/index.html#sec-ecmascript-language-types-symbol-type + +# Motivation +[motivation]: #motivation + +Currently, the `Symbol` function and primitive object wrapper API is available [using a workaround](https://github.com/neon-bindings/neon/issues/502#issuecomment-602568598) with `cx.global()`. However, typechecking a symbol passed in as an argument is not. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Constructing a `JsSymbol`: +```rust +// symbol with a description +let symbol = JsSymbol::new(&mut cx, Some("foo")); + +// symbol without a description +let symbol = JsSymbol::new(&mut cx, None::<&str>); + +// convenience construction method on Context +let symbol = cx.symbol(Some("foo")); +``` + +As with other primitives, we can type check it: +```rust +fn is_symbol(mut cx: FunctionContext) -> JsResult { + let arg = cx.argument(0)?; + let result = arg.is_a::(&mut cx); + Ok(cx.boolean(result)) +} +``` + +Unlike many other primitives, `JsSymbol` doesn't have a `value` method to convert to a Rust type. However, it has a `description` method that returns the underlying `Symbol.prototype.description` instance property: +```rust +let symbol = JsSymbol::new(&mut cx, Some("foo")); +let description = symbol.description(&mut cx); +assert_eq!(description, Some("foo".to_owned())); + +let symbol_without_description = JsSymbol::new(&mut cx, None::<&str>); +let description = symbol_without_description.description(&mut cx); +assert_eq!(description, None); +``` + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +#### JsSymbol +- `new` would be implemented using [napi_create_symbol](https://nodejs.org/docs/latest-v14.x/api/n-api.html#n_api_napi_create_symbolhttps://nodejs.org/docs/latest-v14.x/api/n-api.html#n_api_napi_create_symbol) (as well as the v8 equivalent for the legacy-api) +- `description` would use the [existing implementation](https://github.com/neon-bindings/neon/blob/5f6481a53203580c3a301be02567a502de53e871/crates/neon-runtime/src/napi/object.rs#L110-L114) for napi_get_property +- `is_typeof` would leverage the [existing implementation](https://github.com/neon-bindings/neon/blob/main/crates/neon-runtime/src/napi/tag.rs#L5-L9) for typechecking, with a `napi::ValueType::Symbol` +```rust +#[repr(C)] +#[derive(Copy, Clone)] +pub struct JsSymbol(raw::Local); + +impl JsSymbol { + pub fn new<'a, C: Context<'a>, S: AsRef>(cx: &mut C, description: Option) -> Handle<'a, JsSymbol>; + + pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option; +} + +impl Value for JsSymbol {} + +impl Managed for JsSymbol { + fn to_raw(self) -> raw::Local { self.0 } + + fn from_raw(_: Env, h: raw::Local) -> Self { JsSymbol(h) } +} + +impl ValueInternal for JsSymbol { + fn name() -> String { "symbol".to_string() } + + fn is_typeof(env: Env, other: Other) -> bool { + unsafe { neon_runtime::tag::is_symbol(env.to_raw(), other.to_raw())} + } +} +``` + +`Context` would be modified to add a convenience method for constructing a `JsSymbol`: +```rust +trait Context<'a>: ContextInternal<'a> { + fn symbol>(&mut self, s: Option) -> Handle<'a, JsSymbol> { + JsSymbol::new(self, s) + } +} +``` + +NOTE: Node 10.23 [does not support](https://node.green/#ES2019-misc-Symbol-prototype-description) `Symbol.prototype.description`, so I believe this would need to be feature-flagged. + +# Rationale and alternatives +[alternatives]: #alternatives + +The proposed `new` function has the caveat that you need to add a type annotation to the `None` variant. I'm unsure of how commonplace it is to construct symbols without descriptions. It might be worth considering two constructors, allowing users to skip wrapping the description in `Option`. +```rust +impl JsSymbol { + pub fn new<'a, C: Context<'a>, S: AsRef>(cx: &mut C, s: S) -> Handle<'a, JsSymbol>; + pub fn new_without_description<'a, C: Context<'a>>(cx: &mut C) -> Handle<'a, JsSymbol>; + pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option; +} + +trait Context<'a>: ContextInternal<'a> { + fn symbol>(&mut self, s: S) -> Handle<'a, JsSymbol> { + JsSymbol::new(self, s) + } +} +``` + + +# Unresolved questions +[unresolved]: #unresolved-questions + +This RFC does not currently propose anything for the `Symbol` object wrapper API, which includes Well-Known Symbols and the Symbol global registry (`Symbol.for()`/`Symbol.keyFor()`). +These are available using the workaround linked above. While this proposal could be extended to include those, the primary goal for this RFC was to work towards completeness in typechecking. From c20ffed4f5b351448d23d9accf66427f70d7d678 Mon Sep 17 00:00:00 2001 From: Chris Bajorin <7016796+chrisbajorin@users.noreply.github.com> Date: Wed, 16 Dec 2020 20:14:45 -0500 Subject: [PATCH 2/3] update to use alternative based on feedback --- text/0000-symbol-primitive.md | 45 ++++++++++------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/text/0000-symbol-primitive.md b/text/0000-symbol-primitive.md index 960f503..2f9e94d 100644 --- a/text/0000-symbol-primitive.md +++ b/text/0000-symbol-primitive.md @@ -19,13 +19,13 @@ Currently, the `Symbol` function and primitive object wrapper API is available [ Constructing a `JsSymbol`: ```rust // symbol with a description -let symbol = JsSymbol::new(&mut cx, Some("foo")); +let symbol = JsSymbol::with_description(&mut cx, "foo"); // symbol without a description -let symbol = JsSymbol::new(&mut cx, None::<&str>); +let symbol = JsSymbol::new(&mut cx); -// convenience construction method on Context -let symbol = cx.symbol(Some("foo")); +// convenience construction method on Context for the common case of `with_description` +let symbol = cx.symbol("foo"); ``` As with other primitives, we can type check it: @@ -39,11 +39,11 @@ fn is_symbol(mut cx: FunctionContext) -> JsResult { Unlike many other primitives, `JsSymbol` doesn't have a `value` method to convert to a Rust type. However, it has a `description` method that returns the underlying `Symbol.prototype.description` instance property: ```rust -let symbol = JsSymbol::new(&mut cx, Some("foo")); +let symbol = JsSymbol::with_description(&mut cx, "foo"); let description = symbol.description(&mut cx); assert_eq!(description, Some("foo".to_owned())); -let symbol_without_description = JsSymbol::new(&mut cx, None::<&str>); +let symbol_without_description = JsSymbol::new(&mut cx); let description = symbol_without_description.description(&mut cx); assert_eq!(description, None); ``` @@ -52,7 +52,7 @@ assert_eq!(description, None); [reference-level-explanation]: #reference-level-explanation #### JsSymbol -- `new` would be implemented using [napi_create_symbol](https://nodejs.org/docs/latest-v14.x/api/n-api.html#n_api_napi_create_symbolhttps://nodejs.org/docs/latest-v14.x/api/n-api.html#n_api_napi_create_symbol) (as well as the v8 equivalent for the legacy-api) +- `new` would be implemented using [napi_create_symbol](https://nodejs.org/docs/latest-v14.x/api/n-api.html#n_api_napi_create_symbol) - `description` would use the [existing implementation](https://github.com/neon-bindings/neon/blob/5f6481a53203580c3a301be02567a502de53e871/crates/neon-runtime/src/napi/object.rs#L110-L114) for napi_get_property - `is_typeof` would leverage the [existing implementation](https://github.com/neon-bindings/neon/blob/main/crates/neon-runtime/src/napi/tag.rs#L5-L9) for typechecking, with a `napi::ValueType::Symbol` ```rust @@ -61,8 +61,8 @@ assert_eq!(description, None); pub struct JsSymbol(raw::Local); impl JsSymbol { - pub fn new<'a, C: Context<'a>, S: AsRef>(cx: &mut C, description: Option) -> Handle<'a, JsSymbol>; - + pub fn new<'a, C: Context<'a>>(cx: &mut C) -> Handle<'a, JsSymbol>; + pub fn with_description<'a, C: Context<'a>, S: AsRef>(cx: &mut C, s: S) -> Handle<'a, JsSymbol>; pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option; } @@ -83,38 +83,19 @@ impl ValueInternal for JsSymbol { } ``` -`Context` would be modified to add a convenience method for constructing a `JsSymbol`: +`Context` would be modified to add a convenience method for constructing a `JsSymbol` with a description: ```rust -trait Context<'a>: ContextInternal<'a> { - fn symbol>(&mut self, s: Option) -> Handle<'a, JsSymbol> { - JsSymbol::new(self, s) - } -} -``` - -NOTE: Node 10.23 [does not support](https://node.green/#ES2019-misc-Symbol-prototype-description) `Symbol.prototype.description`, so I believe this would need to be feature-flagged. - -# Rationale and alternatives -[alternatives]: #alternatives - -The proposed `new` function has the caveat that you need to add a type annotation to the `None` variant. I'm unsure of how commonplace it is to construct symbols without descriptions. It might be worth considering two constructors, allowing users to skip wrapping the description in `Option`. -```rust -impl JsSymbol { - pub fn new<'a, C: Context<'a>, S: AsRef>(cx: &mut C, s: S) -> Handle<'a, JsSymbol>; - pub fn new_without_description<'a, C: Context<'a>>(cx: &mut C) -> Handle<'a, JsSymbol>; - pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option; -} - trait Context<'a>: ContextInternal<'a> { fn symbol>(&mut self, s: S) -> Handle<'a, JsSymbol> { - JsSymbol::new(self, s) + JsSymbol::with_description(self, s) } } ``` +NOTE: Node 10.23 [does not support](https://node.green/#ES2019-misc-Symbol-prototype-description) `Symbol.prototype.description`, so I believe this would need to be feature-flagged. # Unresolved questions [unresolved]: #unresolved-questions This RFC does not currently propose anything for the `Symbol` object wrapper API, which includes Well-Known Symbols and the Symbol global registry (`Symbol.for()`/`Symbol.keyFor()`). -These are available using the workaround linked above. While this proposal could be extended to include those, the primary goal for this RFC was to work towards completeness in typechecking. +These are available using the workaround linked above. From 754088d7be6bcd79d23a5dc6d831cce0f4be6f17 Mon Sep 17 00:00:00 2001 From: Chris Bajorin Date: Sat, 26 Jun 2021 20:09:20 -0400 Subject: [PATCH 3/3] model symbol description as a JsString Handle --- text/0000-symbol-primitive.md | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/text/0000-symbol-primitive.md b/text/0000-symbol-primitive.md index 2f9e94d..afedc54 100644 --- a/text/0000-symbol-primitive.md +++ b/text/0000-symbol-primitive.md @@ -18,13 +18,14 @@ Currently, the `Symbol` function and primitive object wrapper API is available [ Constructing a `JsSymbol`: ```rust -// symbol with a description -let symbol = JsSymbol::with_description(&mut cx, "foo"); +// symbol with a JsString Handle description +let description = cx.string("foo"); +let symbol = JsSymbol::with_description(&mut cx, description); // symbol without a description let symbol = JsSymbol::new(&mut cx); -// convenience construction method on Context for the common case of `with_description` +// convenience construction method on Context for the common case with a string. let symbol = cx.symbol("foo"); ``` @@ -39,9 +40,10 @@ fn is_symbol(mut cx: FunctionContext) -> JsResult { Unlike many other primitives, `JsSymbol` doesn't have a `value` method to convert to a Rust type. However, it has a `description` method that returns the underlying `Symbol.prototype.description` instance property: ```rust -let symbol = JsSymbol::with_description(&mut cx, "foo"); -let description = symbol.description(&mut cx); -assert_eq!(description, Some("foo".to_owned())); +let description_handle = cx.string("foo"); +let symbol = JsSymbol::with_description(&mut cx, description_handle); +let description_string = symbol.description(&mut cx).unwrap().value(&mut cx); +assert_eq!(description_string, "foo".to_owned()); let symbol_without_description = JsSymbol::new(&mut cx); let description = symbol_without_description.description(&mut cx); @@ -62,8 +64,8 @@ pub struct JsSymbol(raw::Local); impl JsSymbol { pub fn new<'a, C: Context<'a>>(cx: &mut C) -> Handle<'a, JsSymbol>; - pub fn with_description<'a, C: Context<'a>, S: AsRef>(cx: &mut C, s: S) -> Handle<'a, JsSymbol>; - pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option; + pub fn with_description<'a, C: Context<'a>>(cx: &mut C, d: Handle<'a, JsString>) -> Handle<'a, JsSymbol>; + pub fn description<'a, C: Context<'a>>(self, cx: &mut C) -> Option>; } impl Value for JsSymbol {} @@ -87,13 +89,12 @@ impl ValueInternal for JsSymbol { ```rust trait Context<'a>: ContextInternal<'a> { fn symbol>(&mut self, s: S) -> Handle<'a, JsSymbol> { - JsSymbol::with_description(self, s) + let desc = self.string(s); + JsSymbol::with_description(self, desc) } } ``` -NOTE: Node 10.23 [does not support](https://node.green/#ES2019-misc-Symbol-prototype-description) `Symbol.prototype.description`, so I believe this would need to be feature-flagged. - # Unresolved questions [unresolved]: #unresolved-questions