From 7340767a7d7e474d75ec84875ce65746a4405ed4 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sat, 12 Oct 2024 09:09:11 +0200 Subject: [PATCH 1/6] chore: Fix generator name in UI tests --- libs/pavex_test_runner/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/libs/pavex_test_runner/src/lib.rs b/libs/pavex_test_runner/src/lib.rs index ed2450e70..e048d128a 100644 --- a/libs/pavex_test_runner/src/lib.rs +++ b/libs/pavex_test_runner/src/lib.rs @@ -602,9 +602,11 @@ impl TestData { [package.metadata.px.generate] generator_type = "cargo_workspace_binary" - generator_name = "app" + generator_name = "dummy" }; cargo_toml["package"]["name"] = format!("application_{}", self.name_hash).into(); + cargo_toml["package"]["metadata"]["px"]["generate"]["generator_name"] = + format!("app_{}", self.name_hash).into(); persist_if_changed( &application_dir.join("Cargo.toml"), toml::to_string(&cargo_toml)?.as_bytes(), From 8bc385b53cd79b373b24371aad7134057006e3bb Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:04:08 +0200 Subject: [PATCH 2/6] chore: Don't compile more crates than the ones strictly necessary. In particular, remove erroneous usage of '--all-targets' --- libs/pavex_test_runner/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/pavex_test_runner/src/lib.rs b/libs/pavex_test_runner/src/lib.rs index e048d128a..2427edb15 100644 --- a/libs/pavex_test_runner/src/lib.rs +++ b/libs/pavex_test_runner/src/lib.rs @@ -203,7 +203,6 @@ fn compile_generated_apps( cmd.arg("-p").arg(name); } let output = cmd - .arg("--all-targets") .current_dir(runtime_directory) .output() .context("Failed to invoke `cargo build` on the generated crates")?; @@ -840,6 +839,10 @@ fn application_code_test(test_name: &str, test: &TestData) -> Trial { fn build_integration_tests(test_dir: &Path, test_name2test_data: &BTreeMap) { let n_integration_tests = test_name2test_data.len(); + if n_integration_tests == 0 { + return; + } + let timer = std::time::Instant::now(); println!("Building {n_integration_tests} integration tests, without running them"); let mut cmd = std::process::Command::new("cargo"); From c0f4e4471fb4f31a3c07eb5397c6e31bfbc7c15c Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Tue, 8 Oct 2024 18:16:29 +0200 Subject: [PATCH 3/6] fix: Perform cross-call-graph analysis to determine if additional .clone() statements are needed before invoking a middleware. --- libs/pavex_test_runner/src/lib.rs | 6 +- .../analyses/processing_pipeline/codegen.rs | 11 +- .../analyses/processing_pipeline/pipeline.rs | 244 +++++++++++++++++- libs/ui_tests/Cargo.toml | 4 + .../Cargo.toml | 17 ++ .../diagnostics.dot | 89 +++++++ .../expectations/app.rs | 238 +++++++++++++++++ .../expectations/diagnostics.dot | 89 +++++++ .../src/lib.rs | 36 +++ .../src/main.rs | 24 ++ .../test_config.toml | 5 + .../Cargo.toml | 17 ++ .../diagnostics.dot | 89 +++++++ .../expectations/diagnostics.dot | 89 +++++++ .../expectations/stderr.txt | 45 ++++ .../src/lib.rs | 36 +++ .../src/main.rs | 24 ++ .../test_config.toml | 5 + 18 files changed, 1062 insertions(+), 6 deletions(-) create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/app.rs create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs create mode 100644 libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml diff --git a/libs/pavex_test_runner/src/lib.rs b/libs/pavex_test_runner/src/lib.rs index 2427edb15..b5c69879c 100644 --- a/libs/pavex_test_runner/src/lib.rs +++ b/libs/pavex_test_runner/src/lib.rs @@ -208,7 +208,11 @@ fn compile_generated_apps( .context("Failed to invoke `cargo build` on the generated crates")?; let build_output = CommandOutput::try_from(&output).context("Failed to parse build output")?; let mut crate_names2error = BTreeMap::<_, String>::new(); - for line in build_output.stderr.lines() { + for line in build_output + .stderr + .lines() + .chain(build_output.stdout.lines()) + { let Ok(cargo_metadata::Message::CompilerMessage(msg)) = serde_json::from_str::(line) else { diff --git a/libs/pavexc/src/compiler/analyses/processing_pipeline/codegen.rs b/libs/pavexc/src/compiler/analyses/processing_pipeline/codegen.rs index c8ceeec69..52803c582 100644 --- a/libs/pavexc/src/compiler/analyses/processing_pipeline/codegen.rs +++ b/libs/pavexc/src/compiler/analyses/processing_pipeline/codegen.rs @@ -86,7 +86,7 @@ impl RequestHandlerPipeline { let invocations = { let mut invocations = vec![]; - for id in &ordered_by_invocation { + for (index, id) in ordered_by_invocation.iter().enumerate() { let fn_ = &id2codegened_fn[id].fn_; if component_db.is_post_processing_middleware(*id) { input_bindings.0.push(Binding { @@ -117,7 +117,14 @@ impl RequestHandlerPipeline { Input bindings: {bindings}", input_type) } - Some(i) => i, + Some(i) => { + let mut output = i.to_token_stream(); + if let Some(cloning_indexes) = stage.type2cloning_indexes.get(input_type) { + if cloning_indexes.contains(&index) { + output = quote! { #i.clone() }; + } } + output + }, } }); let await_ = fn_.sig.asyncness.and_then(|_| Some(quote! { .await })); diff --git a/libs/pavexc/src/compiler/analyses/processing_pipeline/pipeline.rs b/libs/pavexc/src/compiler/analyses/processing_pipeline/pipeline.rs index 0d05d3c08..f57bc1348 100644 --- a/libs/pavexc/src/compiler/analyses/processing_pipeline/pipeline.rs +++ b/libs/pavexc/src/compiler/analyses/processing_pipeline/pipeline.rs @@ -1,9 +1,10 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use ahash::{HashMap, HashMapExt, HashSet}; use guppy::graph::PackageGraph; use guppy::PackageId; use indexmap::{IndexMap, IndexSet}; +use itertools::Itertools; use quote::quote; use pavex_bp_schema::{CloningStrategy, Lifecycle}; @@ -22,6 +23,9 @@ use crate::compiler::analyses::framework_items::FrameworkItemDb; use crate::compiler::app::GENERATED_APP_PACKAGE_ID; use crate::compiler::computation::Computation; use crate::compiler::utils::LifetimeGenerator; +use crate::diagnostic::{ + self, AnnotatedSnippet, CompilerDiagnostic, HelpWithSnippet, LocationExt, OptionalSourceSpanExt, +}; use crate::language::{ Callable, GenericArgument, GenericLifetimeParameter, InvocationStyle, Lifetime, PathType, ResolvedType, TypeReference, @@ -59,6 +63,11 @@ pub struct Stage { pub(crate) post_processing_ids: Vec, /// Pre-processing middlewares to be invoked before `wrapping_id` has completed. pub(crate) pre_processing_ids: Vec, + /// Where to insert `.clone()` invocations for inputs ahead of invoking a + /// middleware within this stage. + /// Middlewares are indexed based on their position in the invocation order for + /// this stage. + pub(crate) type2cloning_indexes: IndexMap>, } #[derive(Debug)] @@ -128,7 +137,7 @@ impl RequestHandlerPipeline { ) -> Result { let error_observer_ids = component_db.error_observers(handler_id).unwrap().to_owned(); - // Step 1: Determine the sequence of middlewares that the request handler is wrapped in. + // Step 1a: Determine the sequence of middlewares that the request handler is wrapped in. let ordered_by_registration = { let mut v = component_db .middleware_chain(handler_id) @@ -138,6 +147,8 @@ impl RequestHandlerPipeline { v }; + // Step 1b: Assign a unique name to each stage, with a suffix based on their + // execution order. let stage_names = { let mut stage_names = vec![]; let mut current_stage_id = 0; @@ -161,6 +172,12 @@ impl RequestHandlerPipeline { stage_names }; + // Step 1c: Group middlewares around in stages. Each stage corresponds to a wrapping + // middleware (or the request handler) alongside the pre- and post- middlewares + // that execute immediately before/after it. + // + // Invariant: all middlewares in stage N are wrapped by the wrapping + // middleware from stage N-1. let pipeline_ids = { let first = component_db .hydrated_component(*ordered_by_registration.first().unwrap(), computation_db); @@ -379,7 +396,7 @@ impl RequestHandlerPipeline { } } - let stages = { + let mut stages = { let mut stages = vec![]; let mut post_processing_ids = vec![]; let mut pre_processing_ids = vec![]; @@ -414,6 +431,8 @@ impl RequestHandlerPipeline { next_state: wrapping_id2next_state.remove(middleware_id), post_processing_ids: std::mem::take(&mut post_processing_ids), pre_processing_ids: std::mem::take(&mut pre_processing_ids), + // This will be populated later on. + type2cloning_indexes: Default::default(), }); } HydratedComponent::PostProcessingMiddleware(_) => { @@ -429,6 +448,156 @@ impl RequestHandlerPipeline { stages }; + // Step 4: at this stage, each call graph *in isolation* satisfies + // the constraints of the borrow checker. + // That's not enough though: different middlewares from the same stage + // might be trying to take the same type value using move semantics. + // We don't know if that's allowed since we haven't done any cross-graph + // analysis up to this stage. + // Now it's the moment! + + // We iterate in reverse order because closer to the request handler + // we are less likely to encounter borrowing issues that relate to some of + // our synthetic types. + 'stage_iter: for stage in stages.iter_mut().rev() { + let ids: Vec<_> = stage + .pre_processing_ids + .iter() + .chain(std::iter::once(&stage.wrapping_id)) + .chain(stage.post_processing_ids.iter()) + .collect(); + + #[derive(Debug)] + struct CloningInfo { + /// The indexes of the middlewares that take the type as input by value. + consumed_by: Vec, + /// The indexes of the middlewares that take a reference (mutable or not) + /// to the type *after* it has been consumed at least once by another + /// middleware. + ref_by: Vec, + } + + #[derive(Clone, Copy, Debug)] + struct ConsumerInfo { + middleware_index: usize, + /// The id of the component for this input type within + /// the context of this middleware. + /// Yes, it can change across middlewares! + component_id: ComponentId, + } + + let mut type2info = HashMap::::new(); + + for (index, &id) in ids.iter().enumerate() { + let call_graph = &id2ordered_call_graphs[id]; + let input_types = + call_graph + .call_graph + .node_weights() + .filter_map(|node| match node { + CallGraphNode::Compute { .. } | CallGraphNode::MatchBranching => None, + CallGraphNode::InputParameter { type_, source } => match source { + InputParameterSource::Component(id) => Some((type_.clone(), *id)), + InputParameterSource::External => None, + }, + }); + + for (ty, component_id) in input_types { + match ty { + ResolvedType::Reference(ref_) => { + // We recurse through multi-references (e.g. &&&T). + let mut inner = ref_.inner.as_ref(); + while let ResolvedType::Reference(ref_) = inner.as_ref() { + inner = ref_.inner.as_ref(); + } + if let Some(info) = type2info.get_mut(inner.as_ref()) { + info.ref_by.push(index); + } + } + ResolvedType::ResolvedPath(_) | + ResolvedType::Tuple(_) => { + let info = type2info.entry(ty.clone()).or_insert( + CloningInfo { consumed_by: Vec::new(), ref_by: Vec::new() } + ); + info.consumed_by.push(ConsumerInfo { middleware_index: index, component_id }); + } + // Scalars are trivially `Copy`, this analysis doesn't concern them. + ResolvedType::ScalarPrimitive(_) => { + continue; + } + // We'd never encounter a raw slice as input type. + ResolvedType::Slice(_) | + // All types are concrete at this stage. + ResolvedType::Generic(_) => unreachable!(), + } + } + } + + let mut type2cloning_indexes = IndexMap::with_capacity(type2info.len()); + for (ty_, cloning_info) in type2info.into_iter() { + let mut indexes = cloning_info.consumed_by; + + // The type is never borrowed after the last move, + // thus we don't need a `.clone()` on the invocation + // for the last consumer + match cloning_info.ref_by.last() { + Some(&last_ref_index) => { + if last_ref_index < indexes.last().unwrap().middleware_index { + indexes.pop(); + } + } + None => { + indexes.pop(); + } + } + + if indexes.is_empty() { + continue; + } + + let issue = indexes.iter().find_position(|info| { + let cannot_clone = component_db.cloning_strategy(info.component_id) + == CloningStrategy::NeverClone; + cannot_clone + }); + if let Some((issue_index, info)) = issue { + let next_ref = cloning_info + .ref_by + .iter() + .find(|&ix| *ix > info.middleware_index); + let next_move = indexes + .get(issue_index + 1) + .map(|info| &info.middleware_index); + let next_index = match (next_ref, next_move) { + (None, None) => unreachable!(), + (None, Some(ix)) | (Some(ix), None) => ix, + (Some(m), Some(r)) => m.min(r), + }; + let moved_by = *ids[info.middleware_index]; + let later_used_by = *ids[*next_index]; + emit_cloning_error( + &ty_, + moved_by, + later_used_by, + info.component_id, + component_db, + computation_db, + package_graph, + diagnostics, + ); + // We emit at most one error to minimise the likelihood of + // duplicates/error cascades that stem from the same underlying + // issue. + break 'stage_iter; + } + + let indexes: BTreeSet<_> = + indexes.into_iter().map(|v| v.middleware_index).collect(); + type2cloning_indexes.insert(ty_, indexes); + } + stage.type2cloning_indexes = type2cloning_indexes; + } + let id2name: IndexMap<_, _> = { let mut wrapping_index = 0u32; let mut pre_processing_index = 0u32; @@ -912,3 +1081,72 @@ fn extract_request_scoped_compute_nodes<'a>( } }) } + +fn emit_cloning_error( + ty_: &ResolvedType, + moved_by: ComponentId, + later_used_by: ComponentId, + component_id: ComponentId, + component_db: &ComponentDb, + computation_db: &ComputationDb, + package_graph: &PackageGraph, + diagnostics: &mut Vec, +) { + let Computation::Callable(moved_by_callable) = component_db + .hydrated_component(moved_by, computation_db) + .computation() + else { + unreachable!() + }; + let moved_by_path = &moved_by_callable.path; + + let Computation::Callable(later_used_by_callable) = component_db + .hydrated_component(later_used_by, computation_db) + .computation() + else { + unreachable!() + }; + let later_used_by_path = &later_used_by_callable.path; + + // TODO(diagnostics): improve the error message pointing at the specific components that require + // the contested type. + let error_msg = format!( + "I can't generate code that will pass the borrow checker *and* match the instructions \ + in your blueprint:\n\ + - One of the components in the call graph for `{moved_by_path}` consumes `{ty_:?}` by value\n\ + - But, later on, the same type is used in the call graph of `{later_used_by_path}`.\n\ + You forbid cloning of `{ty_:?}`, therefore I can't resolve this conflict." + ); + + let mut diagnostic = CompilerDiagnostic::builder(anyhow::anyhow!(error_msg)); + if let Some(user_component_id) = component_db.user_component_id(component_id) { + let help_msg = format!( + "Allow me to clone `{ty_:?}` in order to satisfy the borrow checker.\n\ + You can do so by invoking `.clone_if_necessary()` after having registered your constructor.", + ); + let location = component_db + .user_component_db() + .get_location(user_component_id); + let source = match location.source_file(package_graph) { + Ok(s) => Some(s), + Err(e) => { + diagnostics.push(e.into()); + None + } + }; + let help = match source { + None => HelpWithSnippet::new(help_msg, AnnotatedSnippet::empty()), + Some(source) => { + let labeled_span = diagnostic::get_f_macro_invocation_span(&source, location) + .labeled("The constructor was registered here".into()); + HelpWithSnippet::new( + help_msg, + AnnotatedSnippet::new_optional(source, labeled_span), + ) + } + }; + diagnostic = diagnostic.help_with_snippet(help); + } + + diagnostics.push(diagnostic.build().into()); +} diff --git a/libs/ui_tests/Cargo.toml b/libs/ui_tests/Cargo.toml index 60ce25b14..39bd2f39e 100644 --- a/libs/ui_tests/Cargo.toml +++ b/libs/ui_tests/Cargo.toml @@ -119,6 +119,10 @@ members = [ "blueprint/wrapping_middlewares/next_must_take_a_naked_generic_parameter/generated_app", "blueprint/wrapping_middlewares/wrapping_middlewares_input_parameters_cannot_be_generic", "blueprint/wrapping_middlewares/wrapping_middlewares_input_parameters_cannot_be_generic/generated_app", + "borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post", + "borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/generated_app", + "borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post", + "borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/generated_app", "borrow_checker/cannot_borrow_clonable_request_scoped_as_mut", "borrow_checker/cannot_borrow_clonable_request_scoped_as_mut/generated_app", "borrow_checker/cannot_borrow_singletons_as_mut", diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml new file mode 100644 index 000000000..5264caf89 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "app_ffb908fd" +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/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot new file mode 100644 index 000000000..eb14ce6db --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot @@ -0,0 +1,89 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0(app_ffb908fd::A) -> crate::route_0::Next0"] + 3 [ label = "app_ffb908fd::a() -> app_ffb908fd::A"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 3 -> 2 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app_ffb908fd::wrap(pavex::middleware::Next, app_ffb908fd::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next1) -> pavex::middleware::Next"] + 2 [ label = "app_ffb908fd::A"] + 3 [ label = "crate::route_0::Next1() -> crate::route_0::Next1"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "app_ffb908fd::handler() -> pavex::response::Response"] + 1 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 0 -> 1 [ ] +} + +digraph "GET / - 3" { + 0 [ label = "app_ffb908fd::post(pavex::response::Response, &app_ffb908fd::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app_ffb908fd::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods, app_ffb908fd::A) -> crate::route_1::Next0<'a>"] + 4 [ label = "app_ffb908fd::a() -> app_ffb908fd::A"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 4 -> 2 [ ] + 0 -> 5 [ ] + 6 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "app_ffb908fd::wrap(pavex::middleware::Next>, app_ffb908fd::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "app_ffb908fd::A"] + 3 [ label = "crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 5 [ ] + 6 -> 3 [ ] +} + +digraph "* / - 2" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph "* / - 3" { + 0 [ label = "app_ffb908fd::post(pavex::response::Response, &app_ffb908fd::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app_ffb908fd::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/app.rs b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/app.rs new file mode 100644 index 000000000..6a1cf5095 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/app.rs @@ -0,0 +1,238 @@ +//! 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: pavex_matchit::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 { + let server_state = std::sync::Arc::new(ServerState { + router: build_router(), + application_state, + }); + server_builder.serve(route_request, server_state) +} +fn build_router() -> pavex_matchit::Router { + let mut router = pavex_matchit::Router::new(); + router.insert("/", 0u32).unwrap(); + router +} +async fn route_request( + request: http::Request, + _connection_info: Option, + server_state: std::sync::Arc, +) -> pavex::response::Response { + let (request_head, request_body) = request.into_parts(); + #[allow(unused)] + let request_body = pavex::request::body::RawIncomingBody::from(request_body); + let request_head: pavex::request::RequestHead = request_head.into(); + let matched_route = match server_state.router.at(&request_head.target.path()) { + Ok(m) => m, + Err(_) => { + let allowed_methods: pavex::router::AllowedMethods = pavex::router::MethodAllowList::from_iter( + vec![], + ) + .into(); + return route_1::entrypoint(&allowed_methods).await; + } + }; + let route_id = matched_route.value; + #[allow(unused)] + let url_params: pavex::request::path::RawPathParams<'_, '_> = matched_route + .params + .into(); + match route_id { + 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(s_0: app::A) -> pavex::response::Response { + let response = wrapping_1(s_0.clone()).await; + let response = post_processing_0(response, &s_0).await; + response + } + async fn stage_2() -> pavex::response::Response { + let response = handler().await; + response + } + async fn wrapping_0() -> pavex::response::Response { + let v0 = app::a(); + let v1 = crate::route_0::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: app::A) -> pavex::response::Response { + let v1 = crate::route_0::Next1 { + next: stage_2, + }; + let v2 = pavex::middleware::Next::new(v1); + let v3 = app::wrap(v2, v0); + ::into_response(v3) + } + async fn handler() -> pavex::response::Response { + let v0 = app::handler(); + ::into_response(v0) + } + async fn post_processing_0( + v0: pavex::response::Response, + v1: &app::A, + ) -> pavex::response::Response { + let v2 = app::post(v0, v1); + ::into_response(v2) + } + struct Next0 + where + T: std::future::Future, + { + s_0: app::A, + next: fn(app::A) -> 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)(self.s_0) + } + } + struct Next1 + where + T: std::future::Future, + { + next: fn() -> T, + } + impl std::future::IntoFuture for Next1 + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)() + } + } +} +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, + s_1: app::A, + ) -> pavex::response::Response { + let response = wrapping_1(s_1.clone(), s_0).await; + let response = post_processing_0(response, &s_1).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 = app::a(); + let v2 = crate::route_1::Next0 { + s_0: v0, + s_1: v1, + next: stage_1, + }; + let v3 = pavex::middleware::Next::new(v2); + let v4 = pavex::middleware::wrap_noop(v3).await; + ::into_response(v4) + } + async fn wrapping_1( + v0: app::A, + v1: &pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let v2 = crate::route_1::Next1 { + s_0: v1, + next: stage_2, + }; + let v3 = pavex::middleware::Next::new(v2); + let v4 = app::wrap(v3, v0); + ::into_response(v4) + } + async fn handler(v0: &pavex::router::AllowedMethods) -> pavex::response::Response { + let v1 = pavex::router::default_fallback(v0).await; + ::into_response(v1) + } + async fn post_processing_0( + v0: pavex::response::Response, + v1: &app::A, + ) -> pavex::response::Response { + let v2 = app::post(v0, v1); + ::into_response(v2) + } + struct Next0<'a, T> + where + T: std::future::Future, + { + s_0: &'a pavex::router::AllowedMethods, + s_1: app::A, + next: fn(&'a pavex::router::AllowedMethods, app::A) -> 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, self.s_1) + } + } + 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/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot new file mode 100644 index 000000000..b16e7a952 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot @@ -0,0 +1,89 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0(app::A) -> crate::route_0::Next0"] + 3 [ label = "app::a() -> app::A"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 3 -> 2 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app::wrap(pavex::middleware::Next, app::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next1) -> pavex::middleware::Next"] + 2 [ label = "app::A"] + 3 [ label = "crate::route_0::Next1() -> crate::route_0::Next1"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "app::handler() -> pavex::response::Response"] + 1 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 0 -> 1 [ ] +} + +digraph "GET / - 3" { + 0 [ label = "app::post(pavex::response::Response, &app::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods, app::A) -> crate::route_1::Next0<'a>"] + 4 [ label = "app::a() -> app::A"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 4 -> 2 [ ] + 0 -> 5 [ ] + 6 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "app::wrap(pavex::middleware::Next>, app::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "app::A"] + 3 [ label = "crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 5 [ ] + 6 -> 3 [ ] +} + +digraph "* / - 2" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph "* / - 3" { + 0 [ label = "app::post(pavex::response::Response, &app::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} \ No newline at end of file diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs new file mode 100644 index 000000000..1fc26a8ce --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs @@ -0,0 +1,36 @@ +use pavex::blueprint::{router::GET, Blueprint}; +use pavex::f; +use pavex::middleware::Next; +use pavex::response::Response; +use std::future::IntoFuture; + +#[derive(Clone)] +pub struct A; + +pub fn a() -> A { + todo!() +} + +pub fn wrap(_next: Next, _a: A) -> Response +where + T: IntoFuture, +{ + todo!() +} + +pub fn post(_response: Response, _a: &A) -> Response { + todo!() +} + +pub fn handler() -> Response { + todo!() +} + +pub fn blueprint() -> Blueprint { + let mut bp = Blueprint::new(); + bp.request_scoped(f!(crate::a)).clone_if_necessary(); + bp.post_process(f!(crate::post)); + bp.wrap(f!(crate::wrap)); + bp.route(GET, "/", f!(crate::handler)); + bp +} diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs new file mode 100644 index 000000000..9a906736f --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs @@ -0,0 +1,24 @@ +//! This code is generated by `pavex_test_runner`, +//! Do NOT modify it manually. +use app_ffb908fd::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/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml new file mode 100644 index 000000000..f820e706c --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml @@ -0,0 +1,5 @@ +description = """Pavex correctly clones a type if it is consumed by a wrapping middleware +but it is later needed by a post-processing middleware outside of the wrapping scope""" + +[expectations] +codegen = "pass" diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml new file mode 100644 index 000000000..e8329fe74 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "app_45abbb8d" +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/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot new file mode 100644 index 000000000..ea41b4945 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/diagnostics.dot @@ -0,0 +1,89 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0(app_72158d1c::A) -> crate::route_0::Next0"] + 3 [ label = "app_72158d1c::a() -> app_72158d1c::A"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 3 -> 2 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app_72158d1c::wrap(pavex::middleware::Next, app_72158d1c::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next1) -> pavex::middleware::Next"] + 2 [ label = "app_72158d1c::A"] + 3 [ label = "crate::route_0::Next1() -> crate::route_0::Next1"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "app_72158d1c::handler() -> pavex::response::Response"] + 1 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 0 -> 1 [ ] +} + +digraph "GET / - 3" { + 0 [ label = "app_72158d1c::post(pavex::response::Response, &app_72158d1c::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app_72158d1c::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods, app_72158d1c::A) -> crate::route_1::Next0<'a>"] + 4 [ label = "app_72158d1c::a() -> app_72158d1c::A"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 4 -> 2 [ ] + 0 -> 5 [ ] + 6 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "app_72158d1c::wrap(pavex::middleware::Next>, app_72158d1c::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "app_72158d1c::A"] + 3 [ label = "crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 5 [ ] + 6 -> 3 [ ] +} + +digraph "* / - 2" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph "* / - 3" { + 0 [ label = "app_72158d1c::post(pavex::response::Response, &app_72158d1c::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app_72158d1c::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot new file mode 100644 index 000000000..b16e7a952 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/diagnostics.dot @@ -0,0 +1,89 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0(app::A) -> crate::route_0::Next0"] + 3 [ label = "app::a() -> app::A"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 3 -> 2 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app::wrap(pavex::middleware::Next, app::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next1) -> pavex::middleware::Next"] + 2 [ label = "app::A"] + 3 [ label = "crate::route_0::Next1() -> crate::route_0::Next1"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 4 [ ] +} + +digraph "GET / - 2" { + 0 [ label = "app::handler() -> pavex::response::Response"] + 1 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 0 -> 1 [ ] +} + +digraph "GET / - 3" { + 0 [ label = "app::post(pavex::response::Response, &app::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods, app::A) -> crate::route_1::Next0<'a>"] + 4 [ label = "app::a() -> app::A"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 4 -> 2 [ ] + 0 -> 5 [ ] + 6 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "app::wrap(pavex::middleware::Next>, app::A) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next1<'a>) -> pavex::middleware::Next>"] + 2 [ label = "app::A"] + 3 [ label = "crate::route_1::Next1(&'a pavex::router::AllowedMethods) -> crate::route_1::Next1<'a>"] + 5 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 6 [ label = "&pavex::router::AllowedMethods"] + 2 -> 0 [ ] + 1 -> 0 [ ] + 3 -> 1 [ ] + 0 -> 5 [ ] + 6 -> 3 [ ] +} + +digraph "* / - 2" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph "* / - 3" { + 0 [ label = "app::post(pavex::response::Response, &app::A) -> pavex::response::Response"] + 1 [ label = "pavex::response::Response"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 4 [ label = "&app::A"] + 1 -> 0 [ ] + 0 -> 3 [ ] + 4 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} \ No newline at end of file diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt new file mode 100644 index 000000000..7eb25087d --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt @@ -0,0 +1,45 @@ +ERROR: + ร— I can't generate code that will pass the borrow checker *and* match the + โ”‚ instructions in your blueprint: + โ”‚ - One of the components in the call graph for `app::wrap` + โ”‚ consumes `app::A` by value + โ”‚ - But, later on, the same type is used in the call graph of + โ”‚ `app::post`. + โ”‚ You forbid cloning of `app::A`, therefore I can't resolve this + โ”‚ conflict. + โ”‚ + โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow + โ”‚ checker. + โ”‚ You can do so by invoking `.clone_if_necessary()` after having + โ”‚ registered your constructor. + โ”‚ โ˜ž + โ”‚ โ•ญโ”€[borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs:30:1] + โ”‚ 30 โ”‚ let mut bp = Blueprint::new(); + โ”‚ 31 โ”‚ bp.request_scoped(f!(crate::a)).never_clone(); + โ”‚ ยท  โ”€โ”€โ”€โ”€โ”€โ”€โ”ฌโ”€โ”€โ”€โ”€โ”€ + โ”‚ ยท โ•ฐโ”€โ”€ The constructor was registered here + โ”‚ 32 โ”‚ bp.post_process(f!(crate::post)); + โ”‚ โ•ฐโ”€โ”€โ”€โ”€ + +ERROR: + ร— I can't generate code that will pass the borrow checker *and* match the + โ”‚ instructions in your blueprint: + โ”‚ - One of the components in the call graph for `app::wrap` + โ”‚ consumes `app::A` by value + โ”‚ - But, later on, the same type is used in the call graph of + โ”‚ `app::post`. + โ”‚ You forbid cloning of `app::A`, therefore I can't resolve this + โ”‚ conflict. + โ”‚ + โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow + โ”‚ checker. + โ”‚ You can do so by invoking `.clone_if_necessary()` after having + โ”‚ registered your constructor. + โ”‚ โ˜ž + โ”‚ โ•ญโ”€[borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs:30:1] + โ”‚ 30 โ”‚ let mut bp = Blueprint::new(); + โ”‚ 31 โ”‚ bp.request_scoped(f!(crate::a)).never_clone(); + โ”‚ ยท  โ”€โ”€โ”€โ”€โ”€โ”€โ”ฌโ”€โ”€โ”€โ”€โ”€ + โ”‚ ยท โ•ฐโ”€โ”€ The constructor was registered here + โ”‚ 32 โ”‚ bp.post_process(f!(crate::post)); + โ”‚ โ•ฐโ”€โ”€โ”€โ”€ \ No newline at end of file diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs new file mode 100644 index 000000000..f716f642f --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs @@ -0,0 +1,36 @@ +use pavex::blueprint::{router::GET, Blueprint}; +use pavex::f; +use pavex::middleware::Next; +use pavex::response::Response; +use std::future::IntoFuture; + +#[derive(Clone)] +pub struct A; + +pub fn a() -> A { + todo!() +} + +pub fn wrap(_next: Next, _a: A) -> Response +where + T: IntoFuture, +{ + todo!() +} + +pub fn post(_response: Response, _a: &A) -> Response { + todo!() +} + +pub fn handler() -> Response { + todo!() +} + +pub fn blueprint() -> Blueprint { + let mut bp = Blueprint::new(); + bp.request_scoped(f!(crate::a)).never_clone(); + bp.post_process(f!(crate::post)); + bp.wrap(f!(crate::wrap)); + bp.route(GET, "/", f!(crate::handler)); + bp +} diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs new file mode 100644 index 000000000..8e1a9e254 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/main.rs @@ -0,0 +1,24 @@ +//! This code is generated by `pavex_test_runner`, +//! Do NOT modify it manually. +use app_45abbb8d::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/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml new file mode 100644 index 000000000..30c262ce6 --- /dev/null +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/test_config.toml @@ -0,0 +1,5 @@ +description = """Pavex refuses to clones a type if it was marked as `NeverClone`, +even if cloning is required to fix a borrow checker across middlewares""" + +[expectations] +codegen = "fail" From f635124709862b3d1a15d00f3a6acf5fdbcab6f1 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:04:15 +0200 Subject: [PATCH 4/6] chore: Formatting --- libs/pavex_cli/build.rs | 4 +- libs/pavexc/build.rs | 10 ++- .../borrow_checker/move_while_borrowed.rs | 2 +- .../analyses/user_components/processed_db.rs | 78 +++++++++++-------- libs/pavexc/src/rustdoc/compute/cache.rs | 59 ++++++++------ libs/pavexc_cli/build.rs | 4 +- 6 files changed, 90 insertions(+), 67 deletions(-) diff --git a/libs/pavex_cli/build.rs b/libs/pavex_cli/build.rs index 69f6f224e..ee009cf1c 100644 --- a/libs/pavex_cli/build.rs +++ b/libs/pavex_cli/build.rs @@ -1,7 +1,5 @@ use anyhow::Result; -use vergen_gitcl::{ - Emitter, GitclBuilder, -}; +use vergen_gitcl::{Emitter, GitclBuilder}; pub fn main() -> Result<()> { Emitter::default() diff --git a/libs/pavexc/build.rs b/libs/pavexc/build.rs index 13fa6bd22..9974f2f5e 100644 --- a/libs/pavexc/build.rs +++ b/libs/pavexc/build.rs @@ -1,11 +1,13 @@ use anyhow::Result; -use vergen_gitcl::{ - Emitter, GitclBuilder, -}; +use vergen_gitcl::{Emitter, GitclBuilder}; pub fn main() -> Result<()> { Emitter::default() - .add_instructions(&GitclBuilder::default().describe(true, false, None).build()?)? + .add_instructions( + &GitclBuilder::default() + .describe(true, false, None) + .build()?, + )? .emit()?; Ok(()) } diff --git a/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/move_while_borrowed.rs b/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/move_while_borrowed.rs index 80fdf13e6..d0566b399 100644 --- a/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/move_while_borrowed.rs +++ b/libs/pavexc/src/compiler/analyses/call_graph/borrow_checker/move_while_borrowed.rs @@ -439,7 +439,7 @@ fn emit_ancestor_descendant_borrow_error( if let Some(user_component_id) = component_db.user_component_id(component_id) { let help_msg = format!( "Allow me to clone `{contended_type:?}` in order to satisfy the borrow checker.\n\ - You can do so by invoking `.cloning(CloningStrategy::CloneIfNecessary)` on the type returned by `.constructor`.", + You can do so by invoking `.clone_if_necessary()` after having registered your constructor.", ); let location = component_db .user_component_db() diff --git a/libs/pavexc/src/compiler/analyses/user_components/processed_db.rs b/libs/pavexc/src/compiler/analyses/user_components/processed_db.rs index 81f54af3f..b4c19bed3 100644 --- a/libs/pavexc/src/compiler/analyses/user_components/processed_db.rs +++ b/libs/pavexc/src/compiler/analyses/user_components/processed_db.rs @@ -18,7 +18,7 @@ use crate::compiler::component::{PrebuiltType, PrebuiltTypeValidationError}; use crate::compiler::interner::Interner; use crate::compiler::resolvers::{resolve_type_path, CallableResolutionError, TypeResolutionError}; use crate::diagnostic::{ - AnnotatedSnippet, CallableDefinition, CompilerDiagnostic, OptionalSourceSpanExt, SourceSpanExt + AnnotatedSnippet, CallableDefinition, CompilerDiagnostic, OptionalSourceSpanExt, SourceSpanExt, }; use crate::language::ResolvedPath; use crate::rustdoc::CrateCollection; @@ -235,7 +235,7 @@ impl UserComponentDb { } /// Return the cloning strategy of the component with the given id. - /// This is going to be `Some(..)` for constructor and prebuilt type components, + /// This is going to be `Some(..)` for constructor and prebuilt type components, /// and `None` for all other components. pub fn get_cloning_strategy(&self, id: UserComponentId) -> Option<&CloningStrategy> { self.id2cloning_strategy.get(&id) @@ -361,7 +361,7 @@ impl UserComponentDb { diagnostics: &mut Vec, ) { use std::fmt::Write as _; - + let location = component_db.get_location(component_id); let source = try_source!(location, package_graph, diagnostics); let label = source @@ -377,24 +377,30 @@ impl UserComponentDb { PrebuiltTypeValidationError::CannotHaveLifetimeParameters { ty } => { if ty.has_implicit_lifetime_parameters() { writeln!( - &mut error_msg, + &mut error_msg, "\n`{resolved_path}` has elided lifetime parameters, which might be non-'static." ).unwrap(); } else { let named_lifetimes = ty.named_lifetime_parameters(); if named_lifetimes.len() == 1 { write!( - &mut error_msg, + &mut error_msg, "\n`{resolved_path}` has a named lifetime parameter, `'{}`, that you haven't constrained to be 'static.", named_lifetimes[0] ).unwrap(); } else { write!( - &mut error_msg, + &mut error_msg, "\n`{resolved_path}` has {} named lifetime parameters that you haven't constrained to be 'static: ", named_lifetimes.len(), ).unwrap(); - comma_separated_list(&mut error_msg, named_lifetimes.iter(), |s| format!("`'{s}`"), "and").unwrap(); + comma_separated_list( + &mut error_msg, + named_lifetimes.iter(), + |s| format!("`'{s}`"), + "and", + ) + .unwrap(); write!(&mut error_msg, ".").unwrap(); } }; @@ -404,21 +410,27 @@ impl UserComponentDb { let generic_type_parameters = ty.unassigned_generic_type_parameters(); if generic_type_parameters.len() == 1 { write!( - &mut error_msg, + &mut error_msg, "\n`{resolved_path}` has a generic type parameter, `{}`, that you haven't assigned a concrete type to.", generic_type_parameters[0] ).unwrap(); } else { write!( - &mut error_msg, + &mut error_msg, "\n`{resolved_path}` has {} generic type parameters that you haven't assigned concrete types to: ", generic_type_parameters.len(), ).unwrap(); - comma_separated_list(&mut error_msg, generic_type_parameters.iter(), |s| format!("`{s}`"), "and").unwrap(); + comma_separated_list( + &mut error_msg, + generic_type_parameters.iter(), + |s| format!("`{s}`"), + "and", + ) + .unwrap(); write!(&mut error_msg, ".").unwrap(); } help = format!("Set the generic parameters to concrete types when registering the type as prebuilt. E.g. `bp.prebuilt(f!(crate::MyType))` for `struct MyType(T)`.") - }, + } } let e = anyhow::anyhow!(e).context(error_msg); let diagnostic = CompilerDiagnostic::builder(e) @@ -480,22 +492,22 @@ impl UserComponentDb { diagnostics.push(diagnostic.into()); } CallableResolutionError::InputParameterResolutionError(ref inner_error) => { - let definition_snippet = - if let Some(def) = CallableDefinition::compute_from_item(&inner_error.callable_item, package_graph) { - let mut inputs = def.sig.inputs.iter(); - let input = inputs.nth(inner_error.parameter_index).cloned().unwrap(); - let local_span = match input { - syn::FnArg::Typed(typed) => typed.ty.span(), - syn::FnArg::Receiver(r) => r.span(), - }; - let label = def.convert_local_span(local_span).labeled("I don't know how handle this parameter".into()); - Some(AnnotatedSnippet::new( - def.named_source(), - label, - )) - } else { - None + let definition_snippet = if let Some(def) = + CallableDefinition::compute_from_item(&inner_error.callable_item, package_graph) + { + let mut inputs = def.sig.inputs.iter(); + let input = inputs.nth(inner_error.parameter_index).cloned().unwrap(); + let local_span = match input { + syn::FnArg::Typed(typed) => typed.ty.span(), + syn::FnArg::Receiver(r) => r.span(), }; + let label = def + .convert_local_span(local_span) + .labeled("I don't know how handle this parameter".into()); + Some(AnnotatedSnippet::new(def.named_source(), label)) + } else { + None + }; let label = source .as_ref() .map(|source| { @@ -530,17 +542,19 @@ impl UserComponentDb { } CallableResolutionError::OutputTypeResolutionError(ref inner_error) => { let annotated_snippet = { - if let Some(def) = CallableDefinition::compute_from_item(&inner_error.callable_item, package_graph) { + if let Some(def) = CallableDefinition::compute_from_item( + &inner_error.callable_item, + package_graph, + ) { match &def.sig.output { syn::ReturnType::Default => None, syn::ReturnType::Type(_, type_) => Some(type_.span()), } .map(|s| { - let label = def.convert_local_span(s).labeled("The output type that I can't handle".into()); - AnnotatedSnippet::new( - def.named_source(), - label, - ) + let label = def + .convert_local_span(s) + .labeled("The output type that I can't handle".into()); + AnnotatedSnippet::new(def.named_source(), label) }) } else { None diff --git a/libs/pavexc/src/rustdoc/compute/cache.rs b/libs/pavexc/src/rustdoc/compute/cache.rs index 56a0b611c..174b525bb 100644 --- a/libs/pavexc/src/rustdoc/compute/cache.rs +++ b/libs/pavexc/src/rustdoc/compute/cache.rs @@ -68,10 +68,12 @@ impl RustdocGlobalFsCache { ) -> Result, anyhow::Error> { let connection = self.connection_pool.get()?; match cache_key { - RustdocCacheKey::ThirdPartyCrate(metadata) => { - self.third_party_cache - .get(metadata, &self.cargo_fingerprint, &connection, package_graph) - } + RustdocCacheKey::ThirdPartyCrate(metadata) => self.third_party_cache.get( + metadata, + &self.cargo_fingerprint, + &connection, + package_graph, + ), RustdocCacheKey::ToolchainCrate(name) => { self.toolchain_cache .get(name, &self.cargo_fingerprint, &connection) @@ -84,14 +86,17 @@ impl RustdocGlobalFsCache { &self, cache_key: &RustdocCacheKey, krate: &crate::rustdoc::Crate, - package_graph: &PackageGraph + package_graph: &PackageGraph, ) -> Result<(), anyhow::Error> { let connection = self.connection_pool.get()?; match cache_key { - RustdocCacheKey::ThirdPartyCrate(metadata) => { - self.third_party_cache - .insert(metadata, krate, &self.cargo_fingerprint, &connection, package_graph) - } + RustdocCacheKey::ThirdPartyCrate(metadata) => self.third_party_cache.insert( + metadata, + krate, + &self.cargo_fingerprint, + &connection, + package_graph, + ), RustdocCacheKey::ToolchainCrate(name) => { self.toolchain_cache .insert(name, krate, &self.cargo_fingerprint, &connection) @@ -372,7 +377,7 @@ impl ThirdPartyCrateCache { tracing::Span::current().record("cache_key", tracing::field::debug(&cache_key)); // Retrieve from rustdoc's output from cache, if available. let mut stmt = connection.prepare_cached( - "SELECT + "SELECT root_item_id, external_crates, paths, @@ -446,7 +451,12 @@ impl ThirdPartyCrateCache { Ok(Some(krate)) } - let outcome = _get(package_metadata, cargo_fingerprint, connection, package_graph); + let outcome = _get( + package_metadata, + cargo_fingerprint, + connection, + package_graph, + ); match &outcome { Ok(Some(_)) => { tracing::Span::current().record("hit", true); @@ -462,8 +472,8 @@ impl ThirdPartyCrateCache { /// Store the JSON documentation generated by `rustdoc` in the cache. #[instrument( name = "Cache third-party crate docs to disk", - skip_all, - level=tracing::Level::DEBUG, + skip_all, + level=tracing::Level::DEBUG, fields(crate.id = %package_metadata.id(), cache_key = tracing::field::Empty)) ] fn insert( @@ -472,9 +482,10 @@ impl ThirdPartyCrateCache { krate: &crate::rustdoc::Crate, cargo_fingerprint: &str, connection: &rusqlite::Connection, - package_graph: &PackageGraph + package_graph: &PackageGraph, ) -> Result<(), anyhow::Error> { - let Some(cache_key) = ThirdPartyCrateCacheKey::build(package_graph, package_metadata, cargo_fingerprint) + let Some(cache_key) = + ThirdPartyCrateCacheKey::build(package_graph, package_metadata, cargo_fingerprint) else { return Ok(()); }; @@ -720,15 +731,15 @@ impl<'a> ThirdPartyCrateCacheKey<'a> { let hash = match checksum_crate(&package_path) { Ok(hash) => hash, Err(e) => { - tracing::warn!( - error.message = %e, - error.details = ?e, - "Failed to compute the hash of the package at {}. - I won't cache its JSON documentation to avoid serving stale data.", - package_metadata.id().repr() - ); - return None; - } + tracing::warn!( + error.message = %e, + error.details = ?e, + "Failed to compute the hash of the package at {}. + I won't cache its JSON documentation to avoid serving stale data.", + package_metadata.id().repr() + ); + return None; + } }; Some(hash.to_string()) } else { diff --git a/libs/pavexc_cli/build.rs b/libs/pavexc_cli/build.rs index 8958b1c0b..851aef8ee 100644 --- a/libs/pavexc_cli/build.rs +++ b/libs/pavexc_cli/build.rs @@ -1,7 +1,5 @@ use anyhow::Result; -use vergen_gitcl::{ - Emitter, GitclBuilder, -}; +use vergen_gitcl::{Emitter, GitclBuilder}; pub fn main() -> Result<()> { Emitter::default() From 79ba45fd00ecfa4cb0e1bb19db1f8febc637c5bd Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sat, 12 Oct 2024 11:08:41 +0200 Subject: [PATCH 5/6] chore: Update test expectations with improved error message --- .../borrow_checker/transitive_borrows/expectations/stderr.txt | 4 ++-- .../expectations/stderr.txt | 4 ++-- .../expectations/stderr.txt | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/ui_tests/borrow_checker/transitive_borrows/expectations/stderr.txt b/libs/ui_tests/borrow_checker/transitive_borrows/expectations/stderr.txt index f2cb42d78..8a70942eb 100644 --- a/libs/ui_tests/borrow_checker/transitive_borrows/expectations/stderr.txt +++ b/libs/ui_tests/borrow_checker/transitive_borrows/expectations/stderr.txt @@ -10,8 +10,8 @@ โ”‚ โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow โ”‚ checker. - โ”‚ You can do so by invoking `.cloning(CloningStrategy::CloneIfNecessary)` - โ”‚ on the type returned by `.constructor`. + โ”‚ You can do so by invoking `.clone_if_necessary()` after having + โ”‚ registered your constructor. โ”‚ โ˜ž โ”‚ โ•ญโ”€[borrow_checker/transitive_borrows/src/lib.rs:47:1] โ”‚ 47 โ”‚ let mut bp = Blueprint::new(); diff --git a/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_input_type_is_not_clonable/expectations/stderr.txt b/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_input_type_is_not_clonable/expectations/stderr.txt index 971443ce6..322875321 100644 --- a/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_input_type_is_not_clonable/expectations/stderr.txt +++ b/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_input_type_is_not_clonable/expectations/stderr.txt @@ -9,8 +9,8 @@ โ”‚ โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow โ”‚ checker. - โ”‚ You can do so by invoking `.cloning(CloningStrategy::CloneIfNecessary)` - โ”‚ on the type returned by `.constructor`. + โ”‚ You can do so by invoking `.clone_if_necessary()` after having + โ”‚ registered your constructor. โ”‚ โ˜ž โ”‚ โ•ญโ”€[borrow_checker/triangle/triangle_cannot_be_solved_if_input_type_is_not_clonable/src/lib.rs:37:1] โ”‚ 37 โ”‚ // A is a singleton, so it will be an input parameter of the dependency closure for `handler` diff --git a/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_type_is_not_clonable/expectations/stderr.txt b/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_type_is_not_clonable/expectations/stderr.txt index 92e52a9b4..3065ee2ca 100644 --- a/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_type_is_not_clonable/expectations/stderr.txt +++ b/libs/ui_tests/borrow_checker/triangle/triangle_cannot_be_solved_if_type_is_not_clonable/expectations/stderr.txt @@ -9,8 +9,8 @@ โ”‚ โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow โ”‚ checker. - โ”‚ You can do so by invoking `.cloning(CloningStrategy::CloneIfNecessary)` - โ”‚ on the type returned by `.constructor`. + โ”‚ You can do so by invoking `.clone_if_necessary()` after having + โ”‚ registered your constructor. โ”‚ โ˜ž โ”‚ โ•ญโ”€[borrow_checker/triangle/triangle_cannot_be_solved_if_type_is_not_clonable/src/lib.rs:36:1] โ”‚ 36 โ”‚ let mut bp = Blueprint::new(); From 7ef822a665f9a30578b6ddea193ec7ff87844961 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sat, 12 Oct 2024 12:52:03 +0200 Subject: [PATCH 6/6] feat: Pavex deduplicates diagnostics, thus reducing visual noise when code generation fails --- libs/pavexc_cli/src/main.rs | 85 ++++++++++++------- .../expectations/stderr.txt | 23 ----- 2 files changed, 54 insertions(+), 54 deletions(-) diff --git a/libs/pavexc_cli/src/main.rs b/libs/pavexc_cli/src/main.rs index 749f89c0b..d6a8aa349 100644 --- a/libs/pavexc_cli/src/main.rs +++ b/libs/pavexc_cli/src/main.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashMap}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::{Display, Formatter}; use std::path::{Path, PathBuf}; use std::process::ExitCode; @@ -320,22 +320,25 @@ fn generate( let file = fs_err::OpenOptions::new().read(true).open(blueprint)?; ron::de::from_reader(&file)? }; + let mut reporter = DiagnosticReporter::new(color_profile); // We use the path to the generated application crate as a fingerprint for the project. let project_fingerprint = output.to_string_lossy().into_owned(); - let app = match App::build(blueprint, project_fingerprint, docs_toolchain) { - Ok((a, warnings)) => { - for e in warnings { + let (app, issues) = match App::build(blueprint, project_fingerprint, docs_toolchain) { + Ok((a, issues)) => { + for e in &issues { assert_eq!(e.severity(), Some(Severity::Warning)); - print_report(&e, color_profile); } - a - } - Err(issues) => { - for e in issues { - print_report(&e, color_profile); - } - return Ok(ExitCode::FAILURE); + (Some(a), issues) } + Err(issues) => (None, issues), + }; + + for e in issues { + reporter.print_report(&e); + } + + let Some(app) = app else { + return Ok(ExitCode::FAILURE); }; if let Some(diagnostic_path) = diagnostics { app.diagnostic_representation() @@ -350,33 +353,53 @@ fn generate( generated_app.persist(&output, &mut writer)?; if let Err(errors) = writer.verify() { for e in errors { - print_report(&e, color_profile); + reporter.print_report(&e); } return Ok(ExitCode::FAILURE); } Ok(ExitCode::SUCCESS) } -fn print_report(e: &miette::Report, color_profile: Color) { - let use_color = use_color_on_stderr(color_profile); - match e.severity() { - None | Some(Severity::Error) => { - if use_color { - eprintln!("{}: {e:?}", "ERROR".bold().red()); - } else { - eprintln!("ERROR: {e:?}"); - } - } - Some(Severity::Warning) => { - if use_color { - eprintln!("{}: {e:?}", "WARNING".bold().yellow()); - } else { - eprintln!("WARNING: {e:?}"); - } +/// The compiler may emit the same diagnostic more than once +/// (for a variety of reasons). We use this helper to dedup them. +struct DiagnosticReporter { + already_emitted: HashSet, + use_color: bool, +} + +impl DiagnosticReporter { + fn new(color_profile: Color) -> Self { + let use_color = use_color_on_stderr(color_profile); + Self { + already_emitted: Default::default(), + use_color, } - _ => { - unreachable!() + } + fn print_report(&mut self, e: &miette::Report) { + let formatted = format!("{e:?}"); + if self.already_emitted.contains(&formatted) { + // Avoid printing the same diagnostic multiple times. + return; } + let prefix = match e.severity() { + None | Some(Severity::Error) => { + let mut p = "ERROR".to_string(); + if self.use_color { + p = p.bold().red().to_string(); + } + p + } + Some(Severity::Warning) => { + let mut p = "WARNING".to_string(); + if self.use_color { + p = p.bold().yellow().to_string(); + } + p + } + _ => unreachable!(), + }; + eprintln!("{prefix}: {formatted}"); + self.already_emitted.insert(formatted); } } diff --git a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt index 7eb25087d..3f12b658b 100644 --- a/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt +++ b/libs/ui_tests/borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/expectations/stderr.txt @@ -1,26 +1,3 @@ -ERROR: - ร— I can't generate code that will pass the borrow checker *and* match the - โ”‚ instructions in your blueprint: - โ”‚ - One of the components in the call graph for `app::wrap` - โ”‚ consumes `app::A` by value - โ”‚ - But, later on, the same type is used in the call graph of - โ”‚ `app::post`. - โ”‚ You forbid cloning of `app::A`, therefore I can't resolve this - โ”‚ conflict. - โ”‚ - โ”‚ help: Allow me to clone `app::A` in order to satisfy the borrow - โ”‚ checker. - โ”‚ You can do so by invoking `.clone_if_necessary()` after having - โ”‚ registered your constructor. - โ”‚ โ˜ž - โ”‚ โ•ญโ”€[borrow_checker/across_middlewares/type_is_not_cloned_if_consumed_by_wrap_but_needed_by_post/src/lib.rs:30:1] - โ”‚ 30 โ”‚ let mut bp = Blueprint::new(); - โ”‚ 31 โ”‚ bp.request_scoped(f!(crate::a)).never_clone(); - โ”‚ ยท  โ”€โ”€โ”€โ”€โ”€โ”€โ”ฌโ”€โ”€โ”€โ”€โ”€ - โ”‚ ยท โ•ฐโ”€โ”€ The constructor was registered here - โ”‚ 32 โ”‚ bp.post_process(f!(crate::post)); - โ”‚ โ•ฐโ”€โ”€โ”€โ”€ - ERROR: ร— I can't generate code that will pass the borrow checker *and* match the โ”‚ instructions in your blueprint: