Just for reference:
29494f4

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.87%. Comparing base (67cb6f1) to head (5f6ac80).
Report is 2 commits behind head on master.

@@           Coverage Diff           @@
##           master    #1387   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files          58       58           
  Lines       10765    10765           
=======================================
  Hits         9244     9244           
  Misses       1521     1521           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

All relevant FnMut are updated in the latest commit.

Is there any safety issue ?
In general, FnMut can be recursive, no ? (https://users.rust-lang.org/t/mutability-and-recursion/35080)

Is there any safety issue ? In general, FnMut can be recursive, no ? (https://users.rust-lang.org/t/mutability-and-recursion/35080)

@gwenn

A recursive &mut self method can be considered as a free fn with a &mut self paramater passed around, which is not violating the uniqueness rule because only one stack frame owns the &mut self at a time.

FnMut is not the same as it can choose to NOT pass mut references to the recursive call. That's why recursions are not safe for them.

This example might demostrate the unsafeness (multiple mutable borrows) more clearly:

let conn = Connection::open_in_memory().unwrap();
let mut num = 0;
conn.create_scalar_function("recursive_fn", 1, FunctionFlags::default(), move |ctx| {
    let num = &mut num;
    println!("num is mutably borrowed");
    let param = ctx.get::<i64>(0)?;
    let conn_ref = unsafe { ctx.get_connection()? };
    if param >= 1 {
        conn_ref.query_row("SELECT recursive_fn(?)", params![ param - 1 ], |_| Ok(()))?;
    }
    drop(num);
    println!("num's mutable reference is dropped");
    Ok(0)
}).unwrap();
conn.execute_batch("SELECT recursive_fn(3)").unwrap();

output:

num is mutably borrowed
num is mutably borrowed
num is mutably borrowed
num is mutably borrowed
num's mutable reference is dropped
num's mutable reference is dropped
num's mutable reference is dropped
num's mutable reference is dropped

@branchseer

Your example relies on an unsafe call to ctx.get_connection() and could be considered unsound.

Currently, there is no safe way to obtain a connection reference within the callback function. This is because Connection is Send but not Sync, and the callback must be Send.

However, the documentation for get_connection() doesn’t outline specific requirements for safely using the returned ConnectionRef; it merely references a vague statement on an issue.

So, strictly speaking, your example isn't unsound... ;)

@gwenn

I propose two alternative solutions:

1. Remove the critical get_connection() method without replacement.
2. Document the safety requirements explicitly, providing a clear list of constraints that must be upheld when using the unsafe get_connection().

The second option is non-trivial: the constraints are complex. For practical reasons, broad restrictions should be included to cover all cases comprehensively. As I understand, the restrictions concern thread safety and prohibit any handle usage that could trigger callbacks to Rust code from SQLite.

In practice, I suspect that most crate users won’t fully understand these safety requirements, leading to a high risk of misuse. Consequently, software depending on rusqlite could exhibit undefined behavior with potentially serious side effects.

@mrothNET

Currently, there is no safe way to obtain a connection reference within the callback function.

Well, we can get the connection in the callback using thread-local storage without any unsafe:

thread_local! {
    static CONN: RefCell<Option<Connection>> = RefCell::new(None);
}

CONN.with_borrow_mut(|conn_mut| *conn_mut = Some(Connection::open_in_memory().unwrap()));
CONN.with_borrow(|conn| {
    let conn = conn.as_ref().unwrap();
    let mut num = 0;

    conn.create_scalar_function("recursive_fn", 1, FunctionFlags::default(), move |ctx| {
        let num = &mut num;
        println!("num is mutably borrowed");
        let param = ctx.get::<i64>(0)?;
        CONN.with_borrow(|conn| -> rusqlite::Result<()> {
            let conn = conn.as_ref().unwrap();
            if param >= 1 {
                conn.query_row("SELECT recursive_fn(?)", params![ param - 1 ], |_| Ok(()))?;
            }
            Ok(())
        })?;
        drop(num);
        println!("num's mutable reference is dropped");
        Ok(0)
    }).unwrap();

    conn.execute_batch("SELECT recursive_fn(3)").unwrap();
})

My point is, custom functions are by design reentrant. I believe the developers of SQLite have never felt the need to add any check or lock to prevent them from being re-entered, and that makes them Fn.

@branchseer

Well, we can get the connection in the callback using thread-local storage without any unsafe:

Ah, thanks! I hadn’t thought of that...

My point is, custom functions are by design reentrant. I believe the developers of SQLite have never felt the need to add any check or lock to prevent them from being re-entered, and that makes them Fn.

Yes, you're right. The only solution is to make the callback Fn.

Thanks for the insight!