Skip to content

Commit

Permalink
fix: Pavex no longer returns a borrow checker error when a &mut refer…
Browse files Browse the repository at this point in the history
…ence must be coerced to a shared reference (#401)
  • Loading branch information
LukeMathWalker authored Dec 30, 2024
1 parent ab5e395 commit 65656bb
Show file tree
Hide file tree
Showing 10 changed files with 539 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<NodeIndex>,
Expand Down
21 changes: 21 additions & 0 deletions libs/pavexc/src/compiler/codegen_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions libs/ui_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
17 changes: 17 additions & 0 deletions libs/ui_tests/borrow_checker/mixed_mutability/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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
75 changes: 75 additions & 0 deletions libs/ui_tests/borrow_checker/mixed_mutability/diagnostics.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
digraph "GET / - 0" {
0 [ label = "2| pavex::middleware::wrap_noop(pavex::middleware::Next<crate::route_0::Next0>) -> pavex::response::Response"]
1 [ label = "1| pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next<crate::route_0::Next0>"]
2 [ label = "0| crate::route_0::Next0() -> crate::route_0::Next0"]
3 [ label = "3| <pavex::response::Response as pavex::response::IntoResponse>::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<crate::route_0::Next1<'a>>, app_7c2a5f14::C) -> pavex::response::Response"]
1 [ label = "3| pavex::middleware::Next::new(crate::route_0::Next1<'a>) -> pavex::middleware::Next<crate::route_0::Next1<'a>>"]
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| <pavex::response::Response as pavex::response::IntoResponse>::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| <pavex::response::Response as pavex::response::IntoResponse>::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<crate::route_1::Next0<'a>>) -> pavex::response::Response"]
1 [ label = "2| pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next<crate::route_1::Next0<'a>>"]
2 [ label = "1| crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"]
4 [ label = "4| <pavex::response::Response as pavex::response::IntoResponse>::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<crate::route_1::Next1<'a>>, app_7c2a5f14::C) -> pavex::response::Response"]
1 [ label = "4| pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next<crate::route_1::Next1<'a>>"]
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| <pavex::response::Response as pavex::response::IntoResponse>::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| <pavex::response::Response as pavex::response::IntoResponse>::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"]
}
Loading

0 comments on commit 65656bb

Please sign in to comment.