Boa: Calling a function that mutates itself causes borrow panic

[HalidOdat]

This is caused when we call a function we borrow the function object and we do not release the hold until function ends. see GcObject::call / GcObject::construct

To Reproduce
Steps to reproduce the issue, or JavaScript code that causes this failure.

function x() {
    x.y = 3;
}
x();

Expected behavior
Not panic and x to have a property y with value 3

Actual behavior
Panics:

thread 'main' panicked at 'Object already borrowed: BorrowMutError', boa/src/builtins/object/gcobject.rs:41:9

[dvtkrlbs]

i can try to fix this issue

[HalidOdat]

i can try to fix this issue

Sure! Go ahead :)

[dvtkrlbs]

hmm i understand why this issue happens since there is another borrow to the GcObject but i dont understand where the other borrow is and cant think of a way to fix it. I will take an another look at it but could use some on help what to do 😞

[HalidOdat]

hmm i understand why this issue happens since there is another borrow to the GcObject but i dont understand where the other borrow is

The other borrow happens in assignment expression execution we call .set_field() with borrows the object.
The second borrow is not that important where it happens as long as it happens somewhere.

cant think of a way to fix it.

The problem is that we borrow mutably the object and we don't release, why do we so this? It's because we take a reference of StatementList to call .run() on it when so we don't have to clone all function body AST every time we do a call and for built in functions we don't have to worry about copying since it only a function pointer (8-bytes) cheap to copy.

So a way to fix this is to store the body of the function (StatementList) as a Rc<StatementList> (so we can cheaply clone it), then we borrow the object clone the body (for ordinary and builtin) we drop the borrow and we execute the body.

what do you think?

[dvtkrlbs]

hmm that makes perfect sense but now the problem is we can't derive Trace trait since it is not implemented for Rc<Statementlist>

[dvtkrlbs]

My plan is to create a RcStatementlist and just use the empty trace macro for now to see if it works
but i have no idea on how trace system works
Edit: So this approach makes everything worse

[HalidOdat]

My plan is to create a RcStatementlist and just use the empty trace macro for now to see if it works
but i have no idea on how trace system works
Edit: So this approach makes everything worse

This is what I would have suggested (we do the same with RcString, RcSymbol, RcBigInt), how does this make everything worse?

[dvtkrlbs]

Than i cant even call run method (i thought Rc was just a wrapper type) it cant find most of the methods of the StatementList. I dont know i might just missing something i am kinda tired.

[HalidOdat]

Than i cant even call run method (i thought Rc was just a wrapper type) it cant find most of the methods of the StatementList. I dont know i might just missing something i am kinda tired.

we need to implement Deref trait on the RcStatementlist:

use std::ops::Deref;
impl Deref for RcStatementlist {
    type Target = Statementlist;
    fn deref(&self) -> &Self::Target {
        self.0
    }
}

The std::rc::Rc implements Deref, but our wrapper type (RcStatementlist) does not. Check RcString/RcSymbol/RcBigInt for more information/examples since its very similar.

[dvtkrlbs]

Wait so Rc does not allow mutation and we need mutation to use set_field ? Dont we need a refcell or something like that too ? Also do we need this to return a RcStatementList too. I am kinda disconnected sorry it was a long day 😢

[HalidOdat]

Wait so Rc does not allow mutation and we need mutation to use set_field ?

we can use .set_field() since in object in Value is stored as a GcObject which allows interior mutability (Rc is not appropriate since we can have cyclic reference which the gc handles). And it has nothing to do with how we store the StatementList ast. it is completly sepatate from .set_filed()

Dont we need a refcell or something like that too ?

GcObject is defied as Gc<GcCell<Object>> the GcCell allows interior mutability.

Also do we need this to return a RcStatementList too.

It does not have to return it. it should be a only used in Function enum.

[dvtkrlbs]

Got it thank you for the help. I probably should look at it tomorrow.

[dvtkrlbs]

Ok so i took an another look and i think we need to clone the body here but just cloning the body does not fix the issue. I just called body.clone().run().

[HalidOdat]

Ok so i took an another look and i think we need to clone the body here but just cloning the body does not fix the issue. I just called body.clone().run().

That is only one half of the problem the next is to release the borrow a nicer way of doing this is to create a enum

enum FunctionBody {
    BuiltIn(NativeFunction),
    Ordinary(RcStatememtList),
}

and return it in the if expression https://github.com/dvtkrlbs/boa/blob/0fc8052f4ee8bc79ee3df265b855f66b421f641e/boa/src/builtins/object/gcobject.rs#L68 then match on it outside of the borrow.

[dvtkrlbs]

Ok so i did as you said here. But i still get a panic. (I wish runtime borrows were easier to debug 😞 )

[HalidOdat]

Ok so i did as you said here. But i still get a panic. (I wish runtime borrows were easier to debug disappointed )

Well the borrow is still active until the borrow guard goes out of scope (GcCellRef), borrow guard is here and goes out of scope at the end of function scope. A way to fix this is with drop manually with drop function other and nicer way is to restrict it to the if let expression scope:

from:

let object = self.borrow(); // this goes out of scope at the end of function
if let Some(function) = object.as_function() {
// ...

to:

if let Some(function) = self.borrow().as_function() { // borrow goes out of scope at the end of if let expression
// ...

[dvtkrlbs]

Ok now it works 🥳. I thought with NLL it would get dropped before the function returns

[HalidOdat]

I thought with NLL it would get dropped before the function returns

NLL does not work with dynamic borrows, only static ones