From 837cbfe5a2cbfb820a90dd8e5bca9f17e9dc3b2d Mon Sep 17 00:00:00 2001 From: David Herman Date: Sun, 2 Feb 2025 10:00:50 -0800 Subject: [PATCH] unit tests and logic cleanup for RefCell borrowing errors --- .../neon/src/types_impl/extract/container.rs | 4 +- crates/neon/src/types_impl/extract/error.rs | 4 +- test/napi/lib/container.js | 38 +++++++++++++++++-- test/napi/src/js/container.rs | 36 +++++++++++++++++- 4 files changed, 74 insertions(+), 8 deletions(-) diff --git a/crates/neon/src/types_impl/extract/container.rs b/crates/neon/src/types_impl/extract/container.rs index bfcb2c75e..b0059f5fd 100644 --- a/crates/neon/src/types_impl/extract/container.rs +++ b/crates/neon/src/types_impl/extract/container.rs @@ -62,7 +62,7 @@ impl<'cx, T: 'static> TryFromJs<'cx> for Ref<'cx, T> { match v.downcast::>, _>(cx) { Ok(v) => { let cell = JsBox::deref(&v); - Ok(cell.try_borrow().map_err(|_| RefCellError::Borrowed)) + Ok(cell.try_borrow().map_err(|_| RefCellError::MutablyBorrowed)) } Err(_) => Ok(Err(RefCellError::WrongType)), } @@ -81,7 +81,7 @@ impl<'cx, T: 'static> TryFromJs<'cx> for RefMut<'cx, T> { let cell = JsBox::deref(&v); Ok(cell .try_borrow_mut() - .map_err(|_| RefCellError::MutablyBorrowed)) + .map_err(|_| RefCellError::Borrowed)) } Err(_) => Ok(Err(RefCellError::WrongType)), } diff --git a/crates/neon/src/types_impl/extract/error.rs b/crates/neon/src/types_impl/extract/error.rs index eb9b53d0f..982a007da 100644 --- a/crates/neon/src/types_impl/extract/error.rs +++ b/crates/neon/src/types_impl/extract/error.rs @@ -91,8 +91,8 @@ impl fmt::Display for RefCellError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { RefCellError::WrongType => write!(f, "expected {}", RefCell::<()>::container_name()), - RefCellError::MutablyBorrowed => write!(f, "std::cell::RefCell is mutably borrowed"), - RefCellError::Borrowed => write!(f, "std::cell::RefCell is borrowed"), + RefCellError::MutablyBorrowed => write!(f, "RefCell is mutably borrowed"), + RefCellError::Borrowed => write!(f, "RefCell is borrowed"), } } } diff --git a/test/napi/lib/container.js b/test/napi/lib/container.js index cf23117f4..d421ab5a2 100644 --- a/test/napi/lib/container.js +++ b/test/napi/lib/container.js @@ -2,20 +2,26 @@ const addon = require(".."); const { expect } = require("chai"); const assert = require("chai").assert; -describe("container", function () { +describe("Container type extractors", function () { it("can produce and consume a RefCell", function () { const cell = addon.createStringRefCell("my sekret mesij"); const s = addon.readStringRefCell(cell); assert.strictEqual(s, "my sekret mesij"); }); - it("concatenates a RefCell with a String", function () { + it("can produce and modify a RefCell", function () { + const cell = addon.createStringRefCell("new"); + addon.writeStringRefCell(cell, "modified"); + assert.strictEqual(addon.readStringRefCell(cell), "modified"); + }) + + it("can concatenate a RefCell with a String", function () { const cell = addon.createStringRefCell("hello"); const s = addon.stringRefCellConcat(cell, " world"); assert.strictEqual(s, "hello world"); }); - it("fails with a type error when not given a RefCell", function () { + it("fail with a type error when not given a RefCell", function () { try { addon.stringRefCellConcat("hello", " world"); assert.fail("should have thrown"); @@ -24,4 +30,30 @@ describe("container", function () { assert.strictEqual(err.message, "expected std::cell::RefCell"); } }); + + it("dynamically fail when borrowing a mutably borrowed RefCell", function () { + const cell = addon.createStringRefCell("hello"); + try { + addon.borrowMutAndThen(cell, () => { + addon.stringRefConcat(cell, " world"); + }); + assert.fail("should have thrown"); + } catch (err) { + assert.instanceOf(err, Error); + assert.strictEqual(err.message, "RefCell is mutably borrowed"); + } + }); + + it("dynamically fail when modifying a borrowed RefCell", function () { + const cell = addon.createStringRefCell("hello"); + try { + addon.borrowAndThen(cell, () => { + addon.writeStringRef(cell, "world"); + }); + assert.fail("should have thrown"); + } catch (err) { + assert.instanceOf(err, Error); + assert.strictEqual(err.message, "RefCell is borrowed"); + } + }); }); diff --git a/test/napi/src/js/container.rs b/test/napi/src/js/container.rs index addb083f7..7ba6f34a6 100644 --- a/test/napi/src/js/container.rs +++ b/test/napi/src/js/container.rs @@ -1,4 +1,6 @@ -use std::cell::RefCell; +use neon::{prelude::*, types::extract::Error}; + +use std::cell::{Ref, RefCell, RefMut}; #[neon::export] fn create_string_ref_cell(s: String) -> RefCell { @@ -10,7 +12,39 @@ fn read_string_ref_cell(s: &RefCell) -> String { s.borrow().clone() } +#[neon::export] +fn write_string_ref_cell(s: &RefCell, value: String) { + *s.borrow_mut() = value; +} + #[neon::export] fn string_ref_cell_concat(lhs: &RefCell, rhs: String) -> String { lhs.borrow().clone() + &rhs } + +#[neon::export] +fn string_ref_concat(lhs: Ref, rhs: String) -> String { + lhs.clone() + &rhs +} + +#[neon::export] +fn write_string_ref(mut s: RefMut, value: String) { + *s = value; +} + +#[neon::export] +fn borrow_and_then<'cx>(cx: &mut Cx<'cx>, cell: &RefCell, f: Handle) +-> JsResult<'cx, JsString> { + let s = cell.borrow(); + f.bind(cx).exec()?; + Ok(cx.string(s.clone())) +} + +#[neon::export] +fn borrow_mut_and_then<'cx>(cx: &mut Cx<'cx>, cell: &RefCell, f: Handle) +-> JsResult<'cx, JsString> { + let mut s = cell.borrow_mut(); + f.bind(cx).exec()?; + *s = "overwritten".to_string(); + Ok(cx.string(s.clone())) +}