diff --git a/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/multiple_consumers.rs b/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/multiple_consumers.rs index 5a624509e..1e2e6b873 100644 --- a/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/multiple_consumers.rs +++ b/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/multiple_consumers.rs @@ -19,6 +19,7 @@ use crate::compiler::computation::Computation; use crate::diagnostic::{ AnnotatedSnippet, CompilerDiagnostic, HelpWithSnippet, LocationExt, OptionalSourceSpanExt, }; +use crate::language::ResolvedType; use crate::rustdoc::CrateCollection; use crate::{diagnostic, try_source}; @@ -59,8 +60,13 @@ pub(super) fn multiple_consumers( }) .collect(); if consumer_ids.len() > 1 { - if copy_checker.is_copy(&call_graph, node_id, component_db, computation_db) { - // You can't have a "used after moved" error for a Copy type. + // You can't have a "used after moved" error for a Copy type. + if copy_checker.is_copy(&call_graph, node_id, component_db, computation_db) + // Even though `&mut` references are not `Copy`, "used after move" is not + // the right error message for them. + // They belong to the `move_while_borrowed` category. So we skip them here. + || is_ref(node_id, &call_graph, component_db, computation_db) + { continue; } @@ -165,6 +171,29 @@ pub(super) fn multiple_consumers( } } +fn is_ref( + node_id: NodeIndex, + call_graph: &RawCallGraph, + component_db: &ComponentDb, + computation_db: &ComputationDb, +) -> bool { + let _is_ref = |ty_| matches!(ty_, &ResolvedType::Reference(..)); + match &call_graph[node_id] { + CallGraphNode::Compute { component_id, .. } => { + let component = component_db.hydrated_component(*component_id, computation_db); + let Some(output_type) = component.output_type() else { + // `()` is not a reference. + return false; + }; + _is_ref(&output_type) + } + CallGraphNode::MatchBranching => { + return false; + } + CallGraphNode::InputParameter { type_, .. } => _is_ref(type_), + } +} + fn emit_multiple_consumers_error( consumed_node_id: NodeIndex, consuming_node_ids: BTreeSet, diff --git a/libs/pavexc/src/compiler/codegen_utils.rs b/libs/pavexc/src/compiler/codegen_utils.rs index d8951162e..39637c6e1 100644 --- a/libs/pavexc/src/compiler/codegen_utils.rs +++ b/libs/pavexc/src/compiler/codegen_utils.rs @@ -114,7 +114,28 @@ where } } }; + + // We also register the reference as a shared reference + // since we rely on the compiler's deref coercion to convert + // `&mut T` to `&T` when needed. + if let ResolvedType::Reference(TypeReference { + is_mutable: true, + lifetime, + inner, + }) = &type_ + { + dependency_bindings.insert( + ResolvedType::Reference(TypeReference { + is_mutable: false, + lifetime: lifetime.to_owned(), + inner: inner.to_owned(), + }), + tokens.clone(), + ); + } + dependency_bindings.insert(type_, tokens); + if to_be_removed { // It won't be needed in the future blocks.remove(&dependency_index); diff --git a/libs/ui_tests/Cargo.toml b/libs/ui_tests/Cargo.toml index 5aa31d0b3..38ea61a0e 100644 --- a/libs/ui_tests/Cargo.toml +++ b/libs/ui_tests/Cargo.toml @@ -148,6 +148,8 @@ members = [ "borrow_checker/diamond/diamond_can_be_solved_if_we_can_clone/generated_app", "borrow_checker/diamond/diamond_cannot_be_solved_if_we_cannot_clone", "borrow_checker/diamond/diamond_cannot_be_solved_if_we_cannot_clone/generated_app", + "borrow_checker/mixed_mutability", + "borrow_checker/mixed_mutability/generated_app", "borrow_checker/multiple_consumers/a_clonable_framework_type_can_be_moved_twice", "borrow_checker/multiple_consumers/a_clonable_framework_type_can_be_moved_twice/generated_app", "borrow_checker/multiple_consumers/a_clonable_input_type_can_be_moved_twice", diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/Cargo.toml b/libs/ui_tests/borrow_checker/mixed_mutability/Cargo.toml new file mode 100644 index 000000000..2b9addc4d --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "app_7c2a5f14" +version = "0.1.0" +edition = "2021" + +[lints.rust.unexpected_cfgs] +level = "allow" +check-cfg = ["cfg(pavex_ide_hint)"] + +[dependencies.pavex] +workspace = true + +[dependencies.pavex_cli_client] +workspace = true + +[dependencies.workspace_hack] +workspace = true diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/diagnostics.dot b/libs/ui_tests/borrow_checker/mixed_mutability/diagnostics.dot new file mode 100644 index 000000000..78ad445ea --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/diagnostics.dot @@ -0,0 +1,75 @@ +digraph "GET / - 0" { + 0 [ label = "2| pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "1| pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "0| crate::route_0::Next0() -> crate::route_0::Next0"] + 3 [ label = "3| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 3 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "4| app_7c2a5f14::wrapper(pavex::middleware::Next>, app_7c2a5f14::C) -> pavex::response::Response"] + 1 [ label = "3| pavex::middleware::Next::new(crate::route_0::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| app_7c2a5f14::c(&app_7c2a5f14::A) -> app_7c2a5f14::C"] + 3 [ label = "0| app_7c2a5f14::a() -> app_7c2a5f14::A"] + 4 [ label = "2| crate::route_0::Next1(&'a mut app_7c2a5f14::A) -> crate::route_0::Next1<'a>"] + 5 [ label = "5| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 3 -> 2 [ label = "&"] + 1 -> 0 [ ] + 4 -> 1 [ ] + 3 -> 4 [ label = "&mut "] + 0 -> 5 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "2| app_7c2a5f14::handler(app_7c2a5f14::B, &mut app_7c2a5f14::A) -> pavex::response::Response"] + 1 [ label = "1| app_7c2a5f14::b(&app_7c2a5f14::A) -> app_7c2a5f14::B"] + 3 [ label = "3| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "0| &mut app_7c2a5f14::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 1 [ ] + 4 -> 0 [ ] +} + +digraph "* * - 0" { + 0 [ label = "3| pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "2| pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"] + 4 [ label = "4| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 5 [ label = "0| &pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 4 [ ] + 5 -> 2 [ ] +} + +digraph "* * - 1" { + 0 [ label = "5| app_7c2a5f14::wrapper(pavex::middleware::Next>, app_7c2a5f14::C) -> pavex::response::Response"] + 1 [ label = "4| pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| app_7c2a5f14::c(&app_7c2a5f14::A) -> app_7c2a5f14::C"] + 3 [ label = "0| app_7c2a5f14::a() -> app_7c2a5f14::A"] + 4 [ label = "3| crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 6 [ label = "6| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 7 [ label = "2| &pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 3 -> 2 [ label = "&"] + 1 -> 0 [ ] + 4 -> 1 [ ] + 0 -> 6 [ ] + 7 -> 4 [ ] +} + +digraph "* * - 2" { + 0 [ label = "1| pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "2| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "0| &pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "0| crate::ApplicationState() -> crate::ApplicationState"] +} diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/expectations/app.rs b/libs/ui_tests/borrow_checker/mixed_mutability/expectations/app.rs new file mode 100644 index 000000000..e40029817 --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/expectations/app.rs @@ -0,0 +1,233 @@ +//! Do NOT edit this code. +//! It was automatically generated by Pavex. +//! All manual edits will be lost next time the code is generated. +extern crate alloc; +struct ServerState { + router: Router, + #[allow(dead_code)] + application_state: ApplicationState, +} +pub struct ApplicationState {} +pub async fn build_application_state() -> crate::ApplicationState { + crate::ApplicationState {} +} +pub fn run( + server_builder: pavex::server::Server, + application_state: ApplicationState, +) -> pavex::server::ServerHandle { + async fn handler( + request: http::Request, + connection_info: Option, + server_state: std::sync::Arc, + ) -> pavex::response::Response { + let (router, state) = (&server_state.router, &server_state.application_state); + router.route(request, connection_info, state).await + } + let router = Router::new(); + let server_state = std::sync::Arc::new(ServerState { + router, + application_state, + }); + server_builder.serve(handler, server_state) +} +struct Router { + router: matchit::Router, +} +impl Router { + /// Create a new router instance. + /// + /// This method is invoked once, when the server starts. + pub fn new() -> Self { + Self { router: Self::router() } + } + fn router() -> matchit::Router { + let mut router = matchit::Router::new(); + router.insert("/", 0u32).unwrap(); + router + } + pub async fn route( + &self, + request: http::Request, + _connection_info: Option, + #[allow(unused)] + state: &ApplicationState, + ) -> pavex::response::Response { + let (request_head, _) = request.into_parts(); + let request_head: pavex::request::RequestHead = request_head.into(); + let Ok(matched_route) = self.router.at(&request_head.target.path()) else { + let allowed_methods: pavex::router::AllowedMethods = pavex::router::MethodAllowList::from_iter( + vec![], + ) + .into(); + return route_1::entrypoint(&allowed_methods).await; + }; + match matched_route.value { + 0u32 => { + match &request_head.method { + &pavex::http::Method::GET => route_0::entrypoint().await, + _ => { + let allowed_methods: pavex::router::AllowedMethods = pavex::router::MethodAllowList::from_iter([ + pavex::http::Method::GET, + ]) + .into(); + route_1::entrypoint(&allowed_methods).await + } + } + } + i => unreachable!("Unknown route id: {}", i), + } + } +} +pub mod route_0 { + pub async fn entrypoint() -> pavex::response::Response { + let response = wrapping_0().await; + response + } + async fn stage_1() -> pavex::response::Response { + let response = wrapping_1().await; + response + } + async fn stage_2<'a>(s_0: &'a mut app::A) -> pavex::response::Response { + let response = handler(s_0).await; + response + } + async fn wrapping_0() -> pavex::response::Response { + let v0 = crate::route_0::Next0 { + next: stage_1, + }; + let v1 = pavex::middleware::Next::new(v0); + let v2 = pavex::middleware::wrap_noop(v1).await; + ::into_response(v2) + } + async fn wrapping_1() -> pavex::response::Response { + let mut v0 = app::a(); + let v1 = app::c(&v0); + let v2 = crate::route_0::Next1 { + s_0: &mut v0, + next: stage_2, + }; + let v3 = pavex::middleware::Next::new(v2); + let v4 = app::wrapper(v3, v1); + ::into_response(v4) + } + async fn handler(v0: &mut app::A) -> pavex::response::Response { + let v1 = app::b(v0); + let v2 = app::handler(v1, v0); + ::into_response(v2) + } + struct Next0 + where + T: std::future::Future, + { + next: fn() -> T, + } + impl std::future::IntoFuture for Next0 + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)() + } + } + struct Next1<'a, T> + where + T: std::future::Future, + { + s_0: &'a mut app::A, + next: fn(&'a mut app::A) -> T, + } + impl<'a, T> std::future::IntoFuture for Next1<'a, T> + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)(self.s_0) + } + } +} +pub mod route_1 { + pub async fn entrypoint<'a>( + s_0: &'a pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let response = wrapping_0(s_0).await; + response + } + async fn stage_1<'a>( + s_0: &'a pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let response = wrapping_1(s_0).await; + response + } + async fn stage_2<'a>( + s_0: &'a pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let response = handler(s_0).await; + response + } + async fn wrapping_0( + v0: &pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let v1 = crate::route_1::Next0 { + s_0: v0, + next: stage_1, + }; + let v2 = pavex::middleware::Next::new(v1); + let v3 = pavex::middleware::wrap_noop(v2).await; + ::into_response(v3) + } + async fn wrapping_1( + v0: &pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let v1 = app::a(); + let v2 = app::c(&v1); + let v3 = crate::route_1::Next1 { + s_0: v0, + next: stage_2, + }; + let v4 = pavex::middleware::Next::new(v3); + let v5 = app::wrapper(v4, v2); + ::into_response(v5) + } + async fn handler(v0: &pavex::router::AllowedMethods) -> pavex::response::Response { + let v1 = pavex::router::default_fallback(v0).await; + ::into_response(v1) + } + struct Next0<'a, T> + where + T: std::future::Future, + { + s_0: &'a pavex::router::AllowedMethods, + next: fn(&'a pavex::router::AllowedMethods) -> T, + } + impl<'a, T> std::future::IntoFuture for Next0<'a, T> + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)(self.s_0) + } + } + struct Next1<'a, T> + where + T: std::future::Future, + { + s_0: &'a pavex::router::AllowedMethods, + next: fn(&'a pavex::router::AllowedMethods) -> T, + } + impl<'a, T> std::future::IntoFuture for Next1<'a, T> + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)(self.s_0) + } + } +} \ No newline at end of file diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/expectations/diagnostics.dot b/libs/ui_tests/borrow_checker/mixed_mutability/expectations/diagnostics.dot new file mode 100644 index 000000000..39b5c2fad --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/expectations/diagnostics.dot @@ -0,0 +1,75 @@ +digraph "GET / - 0" { + 0 [ label = "2| pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "1| pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "0| crate::route_0::Next0() -> crate::route_0::Next0"] + 3 [ label = "3| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 3 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "4| app::wrapper(pavex::middleware::Next>, app::C) -> pavex::response::Response"] + 1 [ label = "3| pavex::middleware::Next::new(crate::route_0::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| app::c(&app::A) -> app::C"] + 3 [ label = "0| app::a() -> app::A"] + 4 [ label = "2| crate::route_0::Next1(&'a mut app::A) -> crate::route_0::Next1<'a>"] + 5 [ label = "5| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 3 -> 2 [ label = "&"] + 1 -> 0 [ ] + 4 -> 1 [ ] + 3 -> 4 [ label = "&mut "] + 0 -> 5 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "2| app::handler(app::B, &mut app::A) -> pavex::response::Response"] + 1 [ label = "1| app::b(&app::A) -> app::B"] + 3 [ label = "3| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "0| &mut app::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 1 [ ] + 4 -> 0 [ ] +} + +digraph "* * - 0" { + 0 [ label = "3| pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "2| pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"] + 4 [ label = "4| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 5 [ label = "0| &pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 4 [ ] + 5 -> 2 [ ] +} + +digraph "* * - 1" { + 0 [ label = "5| app::wrapper(pavex::middleware::Next>, app::C) -> pavex::response::Response"] + 1 [ label = "4| pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "1| app::c(&app::A) -> app::C"] + 3 [ label = "0| app::a() -> app::A"] + 4 [ label = "3| crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 6 [ label = "6| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 7 [ label = "2| &pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 3 -> 2 [ label = "&"] + 1 -> 0 [ ] + 4 -> 1 [ ] + 0 -> 6 [ ] + 7 -> 4 [ ] +} + +digraph "* * - 2" { + 0 [ label = "1| pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "2| ::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "0| &pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "0| crate::ApplicationState() -> crate::ApplicationState"] +} \ No newline at end of file diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/src/lib.rs b/libs/ui_tests/borrow_checker/mixed_mutability/src/lib.rs new file mode 100644 index 000000000..3c6e63393 --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/src/lib.rs @@ -0,0 +1,56 @@ +use pavex::blueprint::{router::GET, Blueprint}; +use pavex::f; +use pavex::middleware::Next; +use pavex::response::Response; +use std::future::IntoFuture; + +// The call graph for the handler looks like this: +// +// &mut A +// / \ +// | | +// | B +// | | +// | | +// handler +// +// Pavex should correctly determine that this is not an issue, +// because `B` only borrows `A` immutably. +// Furthermore, Pavex should correctly pass `&mut A` to `B`'s constructor, +// relying on the compiler's deref coercion to do its magic. + +pub struct A; + +pub struct B; + +pub struct C; + +pub fn wrapper>(_next: Next, _c: C) -> Response { + todo!() +} + +pub fn a() -> A { + todo!() +} + +pub fn c(_a: &A) -> C { + todo!() +} + +pub fn b(_a: &A) -> B { + todo!() +} + +pub fn handler(_b: B, _a: &mut A) -> Response { + todo!() +} + +pub fn blueprint() -> Blueprint { + let mut bp = Blueprint::new(); + bp.request_scoped(f!(crate::a)); + bp.request_scoped(f!(crate::b)); + bp.request_scoped(f!(crate::c)); + bp.wrap(f!(crate::wrapper)); + bp.route(GET, "/", f!(crate::handler)); + bp +} diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/src/main.rs b/libs/ui_tests/borrow_checker/mixed_mutability/src/main.rs new file mode 100644 index 000000000..51deb0401 --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/src/main.rs @@ -0,0 +1,24 @@ +//! This code is generated by `pavex_test_runner`, +//! Do NOT modify it manually. +use app_7c2a5f14::blueprint; +use pavex_cli_client::{Client, config::Color}; +use pavex_cli_client::commands::generate::GenerateError; + +fn main() -> Result<(), Box> { + let ui_test_dir: std::path::PathBuf = std::env::var("UI_TEST_DIR").unwrap().into(); + let outcome = Client::new() + .color(Color::Always) + .pavex_cli_path(std::env::var("PAVEX_TEST_CLI_PATH").unwrap().into()) + .generate(blueprint(), ui_test_dir.join("generated_app")) + .diagnostics_path("diagnostics.dot".into()) + .execute(); + match outcome { + Ok(_) => {}, + Err(GenerateError::NonZeroExitCode(_)) => { std::process::exit(1); } + Err(e) => { + eprintln!("Failed to invoke `pavex generate`.\n{:?}", e); + std::process::exit(1); + } + } + Ok(()) +} diff --git a/libs/ui_tests/borrow_checker/mixed_mutability/test_config.toml b/libs/ui_tests/borrow_checker/mixed_mutability/test_config.toml new file mode 100644 index 000000000..1b6cc403b --- /dev/null +++ b/libs/ui_tests/borrow_checker/mixed_mutability/test_config.toml @@ -0,0 +1,5 @@ +description = """Paves should correctly handle the interaction between mutable +and immutable reference, relying on deref coercion where appropriate""" + +[expectations] +codegen = "pass"