Skip to content

Commit

Permalink
Working with mutable references only makes sense if the type can't be…
Browse files Browse the repository at this point in the history
… cloned (LukeMathWalker#226)
  • Loading branch information
LukeMathWalker authored Mar 30, 2024
1 parent 3a67a93 commit a8bf063
Show file tree
Hide file tree
Showing 4 changed files with 139 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
ERROR:
× You can't inject `&mut app::A` as an input parameter to `app::handler`,
│ since `&mut app::A` has been marked `CloneIfNecessary`.
│ Reasoning about mutations becomes impossible if Pavex can't guarantee that
│ all mutations will affect *the same* instance of `app::A`.
│
│ ╭─[src/lib.rs:19:1]
│ 19 │ .cloning(CloningStrategy::CloneIfNecessary);
│ 20 │ bp.route(GET, "/", f!(self::handler));
│ ·  ────────┬────────
│ · ╰── The request handler was registered here
│ 21 │ bp
│ ╰────
│ ╭─[src/lib.rs:11:1]
│ 11 │
│ 12 │ pub fn handler(_a: &mut A) -> Response {
│ ·  ─────┬────
│ · ╰── The &mut reference
│ 13 │ todo!()
│ ╰────
│  help: Change `app::A`'s cloning strategy to `NeverClone`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
use pavex::blueprint::{constructor::CloningStrategy, router::GET, Blueprint};
use pavex::f;
use pavex::response::Response;

#[derive(Clone)]
pub struct A;

pub fn build() -> A {
A
}

pub fn handler(_a: &mut A) -> Response {
todo!()
}

pub fn blueprint() -> Blueprint {
let mut bp = Blueprint::new();
bp.request_scoped(f!(self::build))
.cloning(CloningStrategy::CloneIfNecessary);
bp.route(GET, "/", f!(self::handler));
bp
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
description = "You can't borrow a mutable reference to a clonable request-scoped component"

[expectations]
codegen = "fail"
94 changes: 92 additions & 2 deletions libs/pavexc/src/compiler/analyses/constructibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use indexmap::{IndexMap, IndexSet};
use itertools::Itertools;
use syn::spanned::Spanned;

use pavex_bp_schema::Lifecycle;
use pavex_bp_schema::{CloningStrategy, Lifecycle};

use crate::compiler::analyses::components::{ComponentDb, ComponentId};
use crate::compiler::analyses::components::{ConsumptionMode, HydratedComponent};
Expand Down Expand Up @@ -194,7 +194,32 @@ impl ConstructibleDb {
);
};
}
Lifecycle::RequestScoped => {}
Lifecycle::RequestScoped => {
let cloning_strategy =
component_db.cloning_strategy(input_component_id);
if cloning_strategy == CloningStrategy::CloneIfNecessary {
if let Some(user_component_id) =
component_db.user_component_id(component_id)
{
Self::mut_ref_to_clonable_request_scoped(
user_component_id,
component_db.user_component_db(),
input,
input_index,
package_graph,
krate_collection,
computation_db,
diagnostics,
)
} else {
tracing::warn!(
"&mut clonable request-scoped input ({:?}) for component {:?}, but the component is not a user component.",
input,
component_id
);
};
}
}
Lifecycle::Transient => {
if let Some(user_component_id) =
component_db.user_component_id(component_id)
Expand Down Expand Up @@ -762,6 +787,71 @@ impl ConstructibleDb {
diagnostics.push(diagnostic.into());
}

fn mut_ref_to_clonable_request_scoped(
user_component_id: UserComponentId,
user_component_db: &UserComponentDb,
scoped_input_type: &ResolvedType,
scoped_input_index: usize,
package_graph: &PackageGraph,
krate_collection: &CrateCollection,
computation_db: &ComputationDb,
diagnostics: &mut Vec<miette::Error>,
) {
fn get_snippet(
callable: &Callable,
krate_collection: &CrateCollection,
package_graph: &PackageGraph,
mut_ref_input_index: usize,
) -> Option<AnnotatedSnippet> {
let def = CallableDefinition::compute(callable, krate_collection, package_graph)?;
let input = &def.sig.inputs[mut_ref_input_index];
let label = def
.convert_local_span(input.span())
.labeled("The &mut reference".into());
Some(AnnotatedSnippet::new(def.named_source(), label))
}

let component_kind = user_component_db[user_component_id].callable_type();
let callable = &computation_db[user_component_id];
let location = user_component_db.get_location(user_component_id);
let source = try_source!(location, package_graph, diagnostics);
let label = source
.as_ref()
.map(|source| {
diagnostic::get_f_macro_invocation_span(&source, location)
.labeled(format!("The {component_kind} was registered here"))
})
.flatten();

let definition_snippet = get_snippet(
&computation_db[user_component_id],
krate_collection,
package_graph,
scoped_input_index,
);
let ResolvedType::Reference(ref_) = scoped_input_type else {
unreachable!()
};
let error = anyhow::anyhow!(
"You can't inject `{scoped_input_type:?}` as an input parameter to `{}`, \
since `{scoped_input_type:?}` has been marked `CloneIfNecessary`.\n\
Reasoning about mutations becomes impossible if Pavex can't guarantee that all mutations \
will affect *the same* instance of `{:?}`.",
callable.path,
ref_.inner
);
let diagnostic = CompilerDiagnostic::builder(error)
.optional_source(source)
.optional_label(label)
.optional_additional_annotated_snippet(definition_snippet)
.help(format!(
"Change `{:?}`'s cloning strategy to `NeverClone`.",
ref_.inner
))
.build();
diagnostics.push(diagnostic.into());
}

fn singleton_must_depend_on_singletons(
singleton_id: ComponentId,
dependency_id: ComponentId,
Expand Down

0 comments on commit a8bf063

Please sign in to comment.