From ac11fcdd37310a3f2fff8525f4261c18f4301ffe Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Fri, 2 Sep 2022 17:06:58 +0200 Subject: [PATCH 01/17] Remove hack to fix the import path of hyper::body::Body. First step in fixing a problem is admitting you have one! --- libs/pavex/src/web/app.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/pavex/src/web/app.rs b/libs/pavex/src/web/app.rs index 28aeb1edf..66ee84e48 100644 --- a/libs/pavex/src/web/app.rs +++ b/libs/pavex/src/web/app.rs @@ -63,8 +63,7 @@ impl GeneratedApp { Self::inject_app_into_workspace_members(&workspace, &directory)?; - let lib_rs = prettyplease::unparse(&syn::parse2(self.lib_rs)?) - .replace("hyper::body::body::Body", "hyper::body::Body"); + let lib_rs = prettyplease::unparse(&syn::parse2(self.lib_rs)?); if let Some(dependencies) = &mut self.cargo_toml.dependencies { for dependency in dependencies.values_mut() { From 39d27a3d80862753714eaecc28d5a4e9a353d8f5 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Fri, 2 Sep 2022 17:31:01 +0200 Subject: [PATCH 02/17] Improve test output on failure - it should always include stdout/stderr. This allows more effective usage of dbg! to debug tests where the source generation command succeeded but other expectations are broken. --- libs/pavex_test_runner/src/lib.rs | 130 +++++++++++++++++++++--------- 1 file changed, 90 insertions(+), 40 deletions(-) diff --git a/libs/pavex_test_runner/src/lib.rs b/libs/pavex_test_runner/src/lib.rs index 22be77033..df01d2545 100644 --- a/libs/pavex_test_runner/src/lib.rs +++ b/libs/pavex_test_runner/src/lib.rs @@ -1,12 +1,15 @@ -use crate::snapshot::SnapshotTest; +use std::collections::HashMap; +use std::path::PathBuf; +use std::process::Output; + use anyhow::Context; use console::style; use libtest_mimic::Conclusion; use libtest_mimic::Outcome; -use std::collections::HashMap; -use std::path::PathBuf; use toml::toml; +use crate::snapshot::SnapshotTest; + mod snapshot; /// Create a test case for each folder in `definition_directory`. @@ -249,13 +252,24 @@ fn run_test(test: &libtest_mimic::Test) -> Outcome { match test.data.load_configuration() { // Ensure that the test description is always injected on top of the failure message Ok(c) => match _run_test(&c, test) { - Ok(Outcome::Failed { msg }) => Outcome::Failed { - msg: msg.map(|msg| enrich_failure_message(&c, msg)), + Ok(TestOutcome { + outcome: Outcome::Failed { msg }, + source_generation_output, + }) => Outcome::Failed { + msg: msg.map(|msg| { + enrich_failure_message( + &c, + format!( + "{msg}\n\n--- STDOUT:\n{}\n--- STDERR:\n{}", + source_generation_output.stdout, source_generation_output.stderr + ), + ) + }), }, Err(e) => Outcome::Failed { msg: Some(enrich_failure_message(&c, unexpected_failure_message(&e))), }, - Ok(o) => o, + Ok(o) => o.outcome, }, // We do not have the test description if we fail to load the test configuration, so... Err(e) => Outcome::Failed { @@ -267,7 +281,7 @@ fn run_test(test: &libtest_mimic::Test) -> Outcome { fn _run_test( test_config: &TestConfig, test: &libtest_mimic::Test, -) -> Result { +) -> Result { test.data .seed_test_filesystem(test_config) .context("Failed to seed the filesystem for the test runtime folder")?; @@ -280,44 +294,44 @@ fn _run_test( .current_dir(&test.data.runtime_directory) .output() .unwrap(); + let source_generation_output: CommandOutput = (&output).try_into()?; let expectations_directory = test.data.definition_directory.join("expectations"); if !output.status.success() { - let stdout = std::str::from_utf8(&output.stdout) - .context("The application printed invalid UTF8 data to stdout")?; - let stderr = std::str::from_utf8(&output.stderr) - .context("The application printed invalid UTF8 data to stderr")?; return match test_config.expectations.codegen { - ExpectedOutcome::Pass => { - Ok(Outcome::Failed { - msg: Some(format!( - "We failed to generate the application code.\n\n--- STDOUT:\n{}\n--- STDERR:\n{}", - stdout, stderr - )), - }) - } + ExpectedOutcome::Pass => Ok(TestOutcome { + outcome: Outcome::Failed { + msg: Some(format!("We failed to generate the application code.",)), + }, + source_generation_output, + }), ExpectedOutcome::Fail => { let stderr_snapshot = SnapshotTest::new(expectations_directory.join("stderr.txt")); - if stderr_snapshot.verify(stderr).is_err() { - return Ok(Outcome::Failed { - msg: Some( - "The failure message returned by code generation does not match what we expected".into()), + if stderr_snapshot + .verify(&source_generation_output.stderr) + .is_err() + { + return Ok(TestOutcome { + outcome: Outcome::Failed { + msg: Some( + "The failure message returned by code generation does not match what we expected".into()) + }, + source_generation_output, }); } - Ok(Outcome::Passed) + Ok(TestOutcome { + outcome: Outcome::Passed, + source_generation_output, + }) } }; } else if ExpectedOutcome::Fail == test_config.expectations.codegen { - let stdout = std::str::from_utf8(&output.stdout) - .context("The application printed invalid UTF8 data to stdout")?; - let stderr = std::str::from_utf8(&output.stderr) - .context("The application printed invalid UTF8 data to stderr")?; - return Ok(Outcome::Failed { - msg: Some(format!( - "We expected code generation to fail, but it succeeded!\n\n--- STDOUT:\n{}\n--- STDERR:\n{}", - stdout, stderr - )), + return Ok(TestOutcome { + outcome: Outcome::Failed { + msg: Some("We expected code generation to fail, but it succeeded!".into()), + }, + source_generation_output, }); }; @@ -325,10 +339,14 @@ fn _run_test( let actual_diagnostics = fs_err::read_to_string(test.data.runtime_directory.join("diagnostics.dot"))?; if diagnostics_snapshot.verify(&actual_diagnostics).is_err() { - return Ok(Outcome::Failed { - msg: Some( - "Diagnostics for the generated application do not match what we expected".into(), - ), + return Ok(TestOutcome { + outcome: Outcome::Failed { + msg: Some( + "Diagnostics for the generated application do not match what we expected" + .into(), + ), + }, + source_generation_output, }); } @@ -342,12 +360,44 @@ fn _run_test( ) .unwrap(); if app_code_snapshot.verify(&actual_app_code).is_err() { - return Ok(Outcome::Failed { - msg: Some("The generated application code does not match what we expected".into()), + return Ok(TestOutcome { + outcome: Outcome::Failed { + msg: Some("The generated application code does not match what we expected".into()), + }, + source_generation_output, }); } - Ok(Outcome::Passed) + Ok(TestOutcome { + outcome: Outcome::Passed, + source_generation_output, + }) +} + +struct TestOutcome { + outcome: Outcome, + source_generation_output: CommandOutput, +} + +/// A refined `std::process::Output` that assumes that both stderr and stdout are valid UTF8. +struct CommandOutput { + stdout: String, + stderr: String, +} + +impl TryFrom<&Output> for CommandOutput { + type Error = anyhow::Error; + + fn try_from(o: &Output) -> Result { + let stdout = std::str::from_utf8(&o.stdout) + .context("The application printed invalid UTF8 data to stdout")?; + let stderr = std::str::from_utf8(&o.stderr) + .context("The application printed invalid UTF8 data to stderr")?; + Ok(Self { + stdout: stdout.to_string(), + stderr: stderr.to_string(), + }) + } } fn unexpected_failure_message(e: &anyhow::Error) -> String { From 2fdb4025ca74dfcd4254bb1694cc31a5aedeeb9c Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 11:20:36 +0200 Subject: [PATCH 03/17] Make sure that the path index only contains paths that are publicly reachable. --- libs/pavex/src/rustdoc.rs | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/libs/pavex/src/rustdoc.rs b/libs/pavex/src/rustdoc.rs index 54b5d830b..145eaded5 100644 --- a/libs/pavex/src/rustdoc.rs +++ b/libs/pavex/src/rustdoc.rs @@ -6,7 +6,7 @@ use std::path::Path; use anyhow::{anyhow, Context}; use guppy::graph::{PackageGraph, PackageMetadata, PackageSource}; use guppy::{PackageId, Version}; -use rustdoc_types::ItemEnum; +use rustdoc_types::{ItemEnum, Visibility}; use crate::language::ImportPath; @@ -138,7 +138,7 @@ impl PackageIdSpecification { } } -impl std::fmt::Display for PackageIdSpecification { +impl Display for PackageIdSpecification { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { if let Some(source) = &self.source { write!(f, "{}#", source)?; @@ -169,9 +169,19 @@ impl Crate { let mut path_index: HashMap<_, _> = krate .paths .iter() - .map(|(id, summary)| (summary.path.clone(), id.to_owned())) + .filter_map(|(id, summary)| { + if let Some(item) = krate.index.get(&id) { + // Local items are manually index later via `index_local_items` + // This gives us precise paths for types that were re-exported from third-party + // crates or private modules. + if item.visibility == Visibility::Public && item.crate_id != 0 { + return Some((summary.path.clone(), id.to_owned())); + } + } + None + }) .collect(); - index_re_exports(&krate, vec![], &mut path_index, &krate.root); + index_local_items(&krate, vec![], &mut path_index, &krate.root); Self { package_id, krate, @@ -192,7 +202,7 @@ impl Crate { url.trim_end_matches('/') .split('/') .last() - .map(guppy::Version::parse) + .map(Version::parse) .and_then(|x| x.ok()) } else { None @@ -247,16 +257,23 @@ impl Crate { } } -fn index_re_exports<'a>( +fn index_local_items<'a>( krate: &'a rustdoc_types::Crate, mut current_path: Vec<&'a str>, path_index: &mut HashMap, rustdoc_types::Id>, current_item_id: &rustdoc_types::Id, ) { - // TODO: handle visibility // TODO: the way we handle `current_path` is extremely wasteful, // we can likely reuse the same buffer throughout. let current_item = &krate.index[current_item_id]; + + // We do not want to index private items. + if let Visibility::Default | Visibility::Crate | Visibility::Restricted { .. } = + current_item.visibility + { + return; + } + match ¤t_item.inner { ItemEnum::Module(m) => { let current_path_segment = current_item @@ -265,7 +282,7 @@ fn index_re_exports<'a>( .expect("All 'module' items have a 'name' property"); current_path.push(current_path_segment); for item_id in &m.items { - index_re_exports(krate, current_path.clone(), path_index, item_id); + index_local_items(krate, current_path.clone(), path_index, item_id); } } ItemEnum::Import(i) => { @@ -282,7 +299,7 @@ fn index_re_exports<'a>( if let ItemEnum::Module(_) = imported_item.inner { current_path.push(&i.name); } - index_re_exports(krate, current_path.clone(), path_index, imported_id); + index_local_items(krate, current_path.clone(), path_index, imported_id); } } } From 5cb941491ca61cf2e3fb48a18e5f8f5d84016ee8 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 12:16:39 +0200 Subject: [PATCH 04/17] Build a mechanism to find the publicly importable path for a type. --- libs/pavex/src/rustdoc.rs | 81 ++++++++++++++++++++++++++------------- libs/pavex/src/web/app.rs | 54 ++++++++++++++++++-------- 2 files changed, 94 insertions(+), 41 deletions(-) diff --git a/libs/pavex/src/rustdoc.rs b/libs/pavex/src/rustdoc.rs index 145eaded5..bcafa449e 100644 --- a/libs/pavex/src/rustdoc.rs +++ b/libs/pavex/src/rustdoc.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::default::Default; use std::fmt::{Display, Formatter}; use std::path::Path; @@ -162,6 +162,7 @@ pub struct Crate { package_id: PackageId, krate: rustdoc_types::Crate, path_index: HashMap, rustdoc_types::Id>, + public_local_path_index: HashMap>>, } impl Crate { @@ -169,23 +170,26 @@ impl Crate { let mut path_index: HashMap<_, _> = krate .paths .iter() - .filter_map(|(id, summary)| { - if let Some(item) = krate.index.get(&id) { - // Local items are manually index later via `index_local_items` - // This gives us precise paths for types that were re-exported from third-party - // crates or private modules. - if item.visibility == Visibility::Public && item.crate_id != 0 { - return Some((summary.path.clone(), id.to_owned())); - } - } - None - }) + .map(|(id, summary)| (summary.path.clone(), id.to_owned())) .collect(); - index_local_items(&krate, vec![], &mut path_index, &krate.root); + + let mut public_local_path_index = HashMap::new(); + index_local_items(&krate, vec![], &mut public_local_path_index, &krate.root); + + path_index.reserve(public_local_path_index.len()); + for (id, public_paths) in &public_local_path_index { + for public_path in public_paths { + if path_index.get(public_path).is_none() { + path_index.insert(public_path.to_owned(), id.to_owned()); + } + } + } + Self { package_id, krate, path_index, + public_local_path_index, } } @@ -255,12 +259,34 @@ impl Crate { let id = self.get_id_by_path(path)?; Ok(self.get_type_by_id(id)) } + + pub fn get_importable_path(&self, id: &rustdoc_types::Id) -> &[String] { + if let Some(path) = self.public_local_path_index.get(id) { + return path.iter().next().unwrap(); + } + + let item = self.get_type_by_id(id); + if item.crate_id != 0 { + let external_crate = &self.krate.external_crates[&item.crate_id]; + panic!( + "You can only retrieve a path that is guaranteed to be public for local types. \ + `{}` is not local. That id belongs to {} (crate_id={}).", + &item.id.0, &external_crate.name, item.crate_id + ) + } + + panic!( + "Failed to find a publicly importable path for the type id `{}`. \ + This is likely to be a bug in our handling of rustdoc's JSON output.", + id.0 + ) + } } fn index_local_items<'a>( krate: &'a rustdoc_types::Crate, mut current_path: Vec<&'a str>, - path_index: &mut HashMap, rustdoc_types::Id>, + path_index: &mut HashMap>>, current_item_id: &rustdoc_types::Id, ) { // TODO: the way we handle `current_path` is extremely wasteful, @@ -310,10 +336,11 @@ fn index_local_items<'a>( .as_deref() .expect("All 'struct' items have a 'name' property"); current_path.push(struct_name); - path_index.insert( - current_path.into_iter().map(|s| s.to_string()).collect(), - current_item_id.to_owned(), - ); + let path = current_path.into_iter().map(|s| s.to_string()).collect(); + path_index + .entry(current_item_id.to_owned()) + .or_default() + .insert(path); } ItemEnum::Enum(_) => { let enum_name = current_item @@ -321,10 +348,11 @@ fn index_local_items<'a>( .as_deref() .expect("All 'enum' items have a 'name' property"); current_path.push(enum_name); - path_index.insert( - current_path.into_iter().map(|s| s.to_string()).collect(), - current_item_id.to_owned(), - ); + let path = current_path.into_iter().map(|s| s.to_string()).collect(); + path_index + .entry(current_item_id.to_owned()) + .or_default() + .insert(path); } ItemEnum::Function(_) => { let function_name = current_item @@ -332,10 +360,11 @@ fn index_local_items<'a>( .as_deref() .expect("All 'function' items have a 'name' property"); current_path.push(function_name); - path_index.insert( - current_path.into_iter().map(|s| s.to_string()).collect(), - current_item_id.to_owned(), - ); + let path = current_path.into_iter().map(|s| s.to_string()).collect(); + path_index + .entry(current_item_id.to_owned()) + .or_default() + .insert(path); } _ => {} } diff --git a/libs/pavex/src/web/app.rs b/libs/pavex/src/web/app.rs index 66ee84e48..0131e7d0e 100644 --- a/libs/pavex/src/web/app.rs +++ b/libs/pavex/src/web/app.rs @@ -1,5 +1,4 @@ -use std::collections::BTreeMap; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::Debug; use std::io::{BufWriter, Write}; use std::path::{Path, PathBuf}; @@ -21,7 +20,7 @@ use pavex_builder::{AppBlueprint, RawCallableIdentifiers}; use crate::language::{Callable, ParseError, ResolvedType}; use crate::language::{EncodedResolvedPath, ResolvedPath, UnknownPath}; -use crate::rustdoc::{CannotGetCrateData, Crate, CrateCollection}; +use crate::rustdoc::{CannotGetCrateData, CrateCollection}; use crate::web::application_state_call_graph::ApplicationStateCallGraph; use crate::web::dependency_graph::CallableDependencyGraph; use crate::web::diagnostic::{ @@ -689,8 +688,11 @@ fn process_handlers( fn process_type( type_: &rustdoc_types::Type, - krate: &Crate, - package_graph: &guppy::graph::PackageGraph, + // The package id where the type we are trying to process has been referenced (e.g. as an + // input/output parameter). + used_by_package_id: &PackageId, + package_graph: &PackageGraph, + krate_collection: &mut CrateCollection, ) -> Result { match type_ { Type::ResolvedPath(rustdoc_types::Path { id, args, .. }) => { @@ -708,8 +710,9 @@ fn process_type( GenericArg::Type(generic_type) => { generics.push(process_type( generic_type, - krate, + used_by_package_id, package_graph, + krate_collection, )?); } GenericArg::Const(_) => { @@ -728,18 +731,19 @@ fn process_type( } } } - let type_summary = krate.get_summary_by_id(id)?; + let used_by_krate = krate_collection.get_or_compute_by_id(used_by_package_id)?; + let type_summary = used_by_krate.get_summary_by_id(id)?; let type_package_id = if type_summary.crate_id == 0 { - krate.package_id().to_owned() + used_by_krate.package_id().to_owned() } else { - let (owning_crate, owning_crate_version) = krate + let (owning_crate, owning_crate_version) = used_by_krate .get_external_crate_name(type_summary.crate_id) .unwrap(); if owning_crate.name == "std" { PackageId::new(STD_PACKAGE_ID) } else { let transitive_dependencies = package_graph - .query_forward([krate.package_id()]) + .query_forward([used_by_krate.package_id()]) .unwrap() .resolve(); let mut iterator = @@ -764,9 +768,19 @@ fn process_type( .to_owned() } }; + let referenced_base_type_path = type_summary.path.clone(); + let base_type = if type_summary.crate_id == 0 { + used_by_krate.get_importable_path(id) + } else { + // The crate where the type is actually defined. + let source_crate = krate_collection.get_or_compute_by_id(&type_package_id)?; + let type_definition_id = source_crate.get_id_by_path(&referenced_base_type_path)?; + source_crate.get_importable_path(type_definition_id) + } + .to_owned(); Ok(ResolvedType { package_id: type_package_id, - base_type: type_summary.path.clone(), + base_type, generic_arguments: generics, }) } @@ -780,10 +794,10 @@ fn process_type( fn process_callable( krate_collection: &mut CrateCollection, callable_path: &ResolvedPath, - package_graph: &guppy::graph::PackageGraph, + package_graph: &PackageGraph, ) -> Result { let type_ = callable_path.find_type(krate_collection)?; - let krate = krate_collection.get_or_compute_by_id(&callable_path.package_id)?; + let used_by_package_id = &callable_path.package_id; let decl = match &type_.inner { ItemEnum::Function(f) => &f.decl, ItemEnum::Method(m) => &m.decl, @@ -800,7 +814,12 @@ fn process_callable( let mut parameter_paths = Vec::with_capacity(decl.inputs.len()); for (parameter_index, (_, parameter_type)) in decl.inputs.iter().enumerate() { - match process_type(parameter_type, krate, package_graph) { + match process_type( + parameter_type, + used_by_package_id, + package_graph, + krate_collection, + ) { Ok(p) => parameter_paths.push(p), Err(e) => { return Err(ParameterResolutionError { @@ -823,7 +842,12 @@ fn process_callable( }, Some(output_type) => { // TODO: distinguish between output and parameters - match process_type(output_type, krate, package_graph) { + match process_type( + output_type, + used_by_package_id, + package_graph, + krate_collection, + ) { Ok(p) => p, Err(e) => { return Err(OutputTypeResolutionError { From a576a895d608e8ff6cc578e2ad90fdbff39aae6d Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 12:21:43 +0200 Subject: [PATCH 05/17] Update snapshots for one test. --- .../expectations/app.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/app.rs index 154d2ff83..038ce160a 100644 --- a/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/app.rs @@ -58,8 +58,8 @@ fn route_request( _ => panic!("This is a bug, no route registered for a route id"), } } -pub fn route_handler_0() -> http_1::response::Response { - let v1 = app::header1(); +pub fn route_handler_0() -> http_1::Response { let v0 = app::header2(); + let v1 = app::header1(); app::stream_file(v1, v0) } \ No newline at end of file From f19f883950e8d6ea0d0ef3213e4aefa16dfb5d83 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 12:21:48 +0200 Subject: [PATCH 06/17] Update snapshots for one test. --- .../expectations/diagnostics.dot | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/diagnostics.dot index b82bad20e..9b399e991 100644 --- a/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/multiple_versions_for_the_same_crate_are_supported/expectations/diagnostics.dot @@ -1,7 +1,7 @@ digraph "/home" { - 0 [ label = "app::stream_file(http_0::header::name::HeaderName, http_1::header::name::HeaderName) -> http_1::response::Response"] - 1 [ label = "http_1::header::name::HeaderName"] - 2 [ label = "http_0::header::name::HeaderName"] + 0 [ label = "app::stream_file(http_0::header::HeaderName, http_1::header::HeaderName) -> http_1::Response"] + 1 [ label = "http_1::header::HeaderName"] + 2 [ label = "http_0::header::HeaderName"] 1 -> 0 [ ] 2 -> 0 [ ] } From 96bbb1cba871f9ff244c2396b336844b28225e7c Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 12:37:36 +0200 Subject: [PATCH 07/17] Fix flaky test. --- libs/pavex/src/web/codegen_utils.rs | 8 +++++--- libs/pavex/src/web/handler_call_graph.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/libs/pavex/src/web/codegen_utils.rs b/libs/pavex/src/web/codegen_utils.rs index e22eb04d8..3e157e806 100644 --- a/libs/pavex/src/web/codegen_utils.rs +++ b/libs/pavex/src/web/codegen_utils.rs @@ -1,13 +1,15 @@ -use crate::language::{Callable, ResolvedType}; -use crate::web::dependency_graph::DependencyGraphNode; +use std::collections::HashMap; + use bimap::BiHashMap; use petgraph::stable_graph::{NodeIndex, StableDiGraph}; use petgraph::Direction; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; -use std::collections::HashMap; use syn::parse_quote; +use crate::language::{Callable, ResolvedType}; +use crate::web::dependency_graph::DependencyGraphNode; + pub(crate) enum Fragment { VariableReference(syn::Ident), Statement(Box), diff --git a/libs/pavex/src/web/handler_call_graph.rs b/libs/pavex/src/web/handler_call_graph.rs index 47ea4c35a..428f043ec 100644 --- a/libs/pavex/src/web/handler_call_graph.rs +++ b/libs/pavex/src/web/handler_call_graph.rs @@ -191,7 +191,7 @@ pub(crate) fn codegen<'a>( let mut parameter_bindings = HashMap::new(); let mut variable_generator = VariableNameGenerator::default(); - let mut scoped_constructors = HashMap::::new(); + let mut scoped_constructors = IndexMap::::new(); let mut blocks = HashMap::::new(); while let Some(node_index) = dfs.next(Reversed(call_graph)) { From bda1177aacec8ea1bee94422bed84d2e3a3414b7 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Sat, 3 Sep 2022 12:43:35 +0200 Subject: [PATCH 08/17] Align example with current usage pattern. The example is broken (we can't render `std`). --- examples/app_blueprint/Cargo.toml | 10 +-- .../{blueprint.json => blueprint.ron} | 28 ++++---- examples/app_blueprint/src/app.rs | 65 ------------------- examples/app_blueprint/src/bin.rs | 5 +- examples/app_blueprint/src/lib.rs | 7 +- 5 files changed, 22 insertions(+), 93 deletions(-) rename examples/app_blueprint/{blueprint.json => blueprint.ron} (84%) delete mode 100644 examples/app_blueprint/src/app.rs diff --git a/examples/app_blueprint/Cargo.toml b/examples/app_blueprint/Cargo.toml index ae455de96..861665cec 100644 --- a/examples/app_blueprint/Cargo.toml +++ b/examples/app_blueprint/Cargo.toml @@ -7,14 +7,6 @@ edition = "2021" path = "src/bin.rs" name = "bp" -[[bin]] -path = "src/app.rs" -name = "app" - [dependencies] pavex_builder = { path = "../../libs/pavex_builder" } -http = "0.2" -hyper = { version = "0.14", features = ["server", "http1", "http2", "runtime"] } -matchit = "0.6.0" -tokio = { version = "1"} -anyhow = { version = "<=1.0.59"} +pavex_runtime = { path = "../../libs/pavex_runtime" } \ No newline at end of file diff --git a/examples/app_blueprint/blueprint.json b/examples/app_blueprint/blueprint.ron similarity index 84% rename from examples/app_blueprint/blueprint.json rename to examples/app_blueprint/blueprint.ron index 4ba900525..2a8835f08 100644 --- a/examples/app_blueprint/blueprint.json +++ b/examples/app_blueprint/blueprint.ron @@ -20,14 +20,14 @@ ), ], component_lifecycles: { - ( - registered_at: "app_blueprint", - import_path: "crate :: extract_path", - ): RequestScoped, ( registered_at: "app_blueprint", import_path: "crate :: http_client", ): Singleton, + ( + registered_at: "app_blueprint", + import_path: "crate :: extract_path", + ): RequestScoped, ( registered_at: "app_blueprint", import_path: "crate :: logger", @@ -45,7 +45,7 @@ import_path: "crate :: stream_file", ): [ ( - line: 40, + line: 41, column: 10, file: "examples/app_blueprint/src/lib.rs", ), @@ -56,25 +56,25 @@ registered_at: "app_blueprint", import_path: "crate :: extract_path", ): ( - line: 38, - column: 10, - file: "examples/app_blueprint/src/lib.rs", + line: 58, + column: 32, + file: "libs/pavex_builder/src/app.rs", ), ( registered_at: "app_blueprint", import_path: "crate :: logger", ): ( - line: 39, - column: 10, - file: "examples/app_blueprint/src/lib.rs", + line: 58, + column: 32, + file: "libs/pavex_builder/src/app.rs", ), ( registered_at: "app_blueprint", import_path: "crate :: http_client", ): ( - line: 37, - column: 10, - file: "examples/app_blueprint/src/lib.rs", + line: 58, + column: 32, + file: "libs/pavex_builder/src/app.rs", ), }, ) \ No newline at end of file diff --git a/examples/app_blueprint/src/app.rs b/examples/app_blueprint/src/app.rs deleted file mode 100644 index 0b5adb77b..000000000 --- a/examples/app_blueprint/src/app.rs +++ /dev/null @@ -1,65 +0,0 @@ -struct ServerState { - router: matchit::Router, - application_state: ApplicationState, -} -pub struct ApplicationState { - s0: app_blueprint::HttpClient, -} -pub fn build_application_state(v0: app_blueprint::Config) -> ApplicationState { - let v1 = app_blueprint::http_client(v0); - ApplicationState { s0: v1 } -} -pub async fn run( - server_builder: hyper::server::Builder, - application_state: ApplicationState, -) -> Result<(), anyhow::Error> { - let server_state = std::sync::Arc::new(ServerState { - router: build_router()?, - application_state, - }); - let make_service = hyper::service::make_service_fn(move |_| { - let server_state = server_state.clone(); - async move { - Ok::< - _, - hyper::Error, - >( - hyper::service::service_fn(move |request| { - let server_state = server_state.clone(); - async move { - Ok::<_, hyper::Error>(route_request(request, server_state)) - } - }), - ) - } - }); - server_builder.serve(make_service).await.map_err(Into::into) -} -fn build_router() -> Result, matchit::InsertError> { - let mut router = matchit::Router::new(); - router.insert("/home", 0u32)?; - Ok(router) -} -fn route_request( - request: http::Request, - server_state: std::sync::Arc, -) -> http::Response { - let route_id = server_state - .router - .at(request.uri().path()) - .expect("Failed to match incoming request path"); - match route_id.value { - 0u32 => route_handler_0(server_state.application_state.s0.clone(), request), - _ => panic!("This is a bug, no route registered for a route id"), - } -} -pub fn route_handler_0( - v0: app_blueprint::HttpClient, - v1: http::request::Request, -) -> http::response::Response { - let v2 = app_blueprint::extract_path(v1); - let v3 = app_blueprint::logger(); - app_blueprint::stream_file(v2, v3, v0) -} - -fn main () {} \ No newline at end of file diff --git a/examples/app_blueprint/src/bin.rs b/examples/app_blueprint/src/bin.rs index d15c4b14e..bcfe4333e 100644 --- a/examples/app_blueprint/src/bin.rs +++ b/examples/app_blueprint/src/bin.rs @@ -1,10 +1,11 @@ -use app_blueprint::app_blueprint; use std::error::Error; use std::path::PathBuf; use std::str::FromStr; +use app_blueprint::app_blueprint; + fn main() -> Result<(), Box> { - let path = PathBuf::from_str("blueprint.json")?; + let path = PathBuf::from_str("blueprint.ron")?; app_blueprint().persist(&path)?; std::process::Command::new("../../target/debug/pavex_cli") diff --git a/examples/app_blueprint/src/lib.rs b/examples/app_blueprint/src/lib.rs index 402e9a587..47a06843c 100644 --- a/examples/app_blueprint/src/lib.rs +++ b/examples/app_blueprint/src/lib.rs @@ -1,9 +1,10 @@ -use pavex_builder::{f, AppBlueprint, Lifecycle}; use std::path::PathBuf; +use pavex_builder::{f, AppBlueprint, Lifecycle}; + pub struct Logger; -pub fn extract_path(_inner: http::Request) -> PathBuf { +pub fn extract_path(_inner: pavex_runtime::http::Request) -> PathBuf { todo!() } @@ -15,7 +16,7 @@ pub fn stream_file( _inner: PathBuf, _logger: Logger, _http_client: HttpClient, -) -> http::Response { +) -> pavex_runtime::http::Response { todo!() } From 1c732ec142d0be8aead33d2382a17ee02a0fc0d2 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Tue, 6 Sep 2022 14:51:10 +0200 Subject: [PATCH 09/17] Hack together a flow to resolve std's types. --- libs/pavex/src/rustdoc.rs | 130 ++++++++++++++++++++++++++++++++++++-- libs/pavex/src/web/app.rs | 61 +++--------------- 2 files changed, 134 insertions(+), 57 deletions(-) diff --git a/libs/pavex/src/rustdoc.rs b/libs/pavex/src/rustdoc.rs index bcafa449e..5fc6ac63e 100644 --- a/libs/pavex/src/rustdoc.rs +++ b/libs/pavex/src/rustdoc.rs @@ -1,7 +1,8 @@ use std::collections::{BTreeSet, HashMap}; use std::default::Default; use std::fmt::{Display, Formatter}; -use std::path::Path; +use std::path::{Path, PathBuf}; +use std::str::FromStr; use anyhow::{anyhow, Context}; use guppy::graph::{PackageGraph, PackageMetadata, PackageSource}; @@ -18,16 +19,54 @@ pub struct CannotGetCrateData { pub source: anyhow::Error, } +const TOOLCHAIN_CRATES: [&str; 3] = ["std", "core", "alloc"]; + pub fn get_crate_data( root_folder: &Path, package_id_spec: &PackageIdSpecification, ) -> Result { - _get_crate_data(root_folder, package_id_spec).map_err(|e| CannotGetCrateData { + // Some crates are not compiled as part of the dependency tree of the current workspace. + // They are instead bundled as part of Rust's toolchain and automatically available for import + // and usage in your crate: the standard library (`std`), `core` (a smaller subset of `std` + // that does not require an allocator), `alloc` (a smaller subset of `std` that assumes you + // can allocate). + // Since those crates are pre-compiled (and somewhat special), we can't generate their + // documentation on the fly. We assume that their JSON docs have been pre-computed and are + // available for us to look at. + if TOOLCHAIN_CRATES.contains(&package_id_spec.name.as_str()) { + get_toolchain_crate_data(package_id_spec) + } else { + _get_crate_data(root_folder, package_id_spec) + } + .map_err(|e| CannotGetCrateData { package_spec: package_id_spec.to_string(), source: e, }) } +fn get_toolchain_crate_data( + package_id_spec: &PackageIdSpecification, +) -> Result { + // TODO: determine the correct path to the files using `rustup show home`, + // `rustup show active-toolchain` and `rustup component list --installed --toolchain <...>`. + let root_folder = PathBuf::from_str("/Users/luca/code/pavex/").unwrap(); + let json_path = root_folder.join(format!("{}.json", package_id_spec.name)); + let json = fs_err::read_to_string(json_path).with_context(|| { + format!( + "Failed to retrieve the JSON docs for {}", + package_id_spec.name + ) + })?; + serde_json::from_str::(&json) + .with_context(|| { + format!( + "Failed to deserialize the JSON docs for {}", + package_id_spec.name + ) + }) + .map_err(Into::into) +} + fn _get_crate_data( target_directory: &Path, package_id_spec: &PackageIdSpecification, @@ -74,8 +113,16 @@ impl CrateCollection { &mut self, package_id: &PackageId, ) -> Result<&Crate, CannotGetCrateData> { - let package_metadata = self.1.metadata(package_id).expect("Unknown package ID"); - let package_spec = PackageIdSpecification::new(&package_metadata); + let package_spec = if TOOLCHAIN_CRATES.contains(&package_id.repr()) { + PackageIdSpecification { + source: None, + name: package_id.repr().to_string(), + version: None, + } + } else { + let package_metadata = self.1.metadata(package_id).expect("Unknown package ID"); + PackageIdSpecification::new(&package_metadata) + }; if self.0.get(&package_spec.to_string()).is_none() { let krate = get_crate_data( self.1.workspace().target_directory().as_std_path(), @@ -86,6 +133,78 @@ impl CrateCollection { } Ok(&self.0[&package_spec.to_string()]) } + + /// Retrieve the package id where a certain item was originally defined. + pub fn get_defining_package_id_for_item( + &mut self, + used_by_package_id: &PackageId, + item_id: &rustdoc_types::Id, + ) -> Result { + let package_graph = self.1.clone(); + let used_by_krate = self.get_or_compute_by_id(used_by_package_id)?; + let type_summary = used_by_krate.get_summary_by_id(item_id)?; + + let type_package_id = if type_summary.crate_id == 0 { + used_by_krate.package_id().to_owned() + } else { + let (owning_crate, owning_crate_version) = used_by_krate + .get_external_crate_name(type_summary.crate_id) + .unwrap(); + if TOOLCHAIN_CRATES.contains(&owning_crate.name.as_str()) { + PackageId::new(owning_crate.name.clone()) + } else { + let transitive_dependencies = package_graph + .query_forward([used_by_package_id]) + .unwrap() + .resolve(); + let mut iterator = + transitive_dependencies.links(guppy::graph::DependencyDirection::Forward); + iterator + .find(|link| { + link.to().name() == owning_crate.name + && owning_crate_version + .as_ref() + .map(|v| link.to().version() == v) + .unwrap_or(true) + }) + .ok_or_else(|| { + anyhow!( + "I could not find the package id for the crate where `{}` is defined", + type_summary.path.join("::") + ) + }) + .unwrap() + .to() + .id() + .to_owned() + } + }; + Ok(type_package_id) + } + + pub fn get_canonical_import_path( + &mut self, + used_by_package_id: &PackageId, + item_id: &rustdoc_types::Id, + ) -> Result, anyhow::Error> { + let definition_package_id = + self.get_defining_package_id_for_item(used_by_package_id, item_id)?; + + let used_by_krate = self.get_or_compute_by_id(used_by_package_id)?; + let type_summary = used_by_krate.get_summary_by_id(item_id)?; + let referenced_base_type_path = type_summary.path.clone(); + let base_type = if type_summary.crate_id == 0 { + self.get_or_compute_by_id(used_by_package_id)? + .get_importable_path(item_id) + } else { + // The crate where the type is actually defined. + let source_crate = self.get_or_compute_by_id(&definition_package_id)?; + let type_definition_id = source_crate.get_id_by_path(&referenced_base_type_path)?; + source_crate.get_importable_path(type_definition_id) + } + .to_owned(); + Ok(base_type) + } } /// A selector that follows the [package ID specification](https://doc.rust-lang.org/cargo/reference/pkgid-spec.html). @@ -318,7 +437,8 @@ fn index_local_items<'a>( if let Some(imported_summary) = krate.paths.get(imported_id) { debug_assert!(imported_summary.crate_id != 0); } else { - panic!("The imported id is not listed in the index nor in the path section of rustdoc's JSON output") + // TODO: this is firing for std's JSON docs. File a bug report. + // panic!("The imported id ({}) is not listed in the index nor in the path section of rustdoc's JSON output", imported_id.0) } } Some(imported_item) => { diff --git a/libs/pavex/src/web/app.rs b/libs/pavex/src/web/app.rs index 0131e7d0e..08ac78a29 100644 --- a/libs/pavex/src/web/app.rs +++ b/libs/pavex/src/web/app.rs @@ -599,13 +599,14 @@ fn framework_bindings( RawCallableIdentifiers::from_raw_parts(raw_path.into(), "pavex_runtime".into()); let path = ResolvedPath::parse(&identifiers, package_graph).unwrap(); let type_ = path.find_type(krate_collection).unwrap(); - let krate = krate_collection - .get_or_compute_by_id(&path.package_id) + let package_id = krate_collection + .get_defining_package_id_for_item(&path.package_id, &type_.id) + .unwrap(); + let type_base_path = krate_collection + .get_canonical_import_path(&path.package_id, &type_.id) .unwrap(); - let summary = krate.get_summary_by_id(&type_.id).unwrap(); - let type_base_path = summary.path.clone(); ResolvedType { - package_id: path.package_id, + package_id, base_type: type_base_path, generic_arguments: vec![], } @@ -731,53 +732,9 @@ fn process_type( } } } - let used_by_krate = krate_collection.get_or_compute_by_id(used_by_package_id)?; - let type_summary = used_by_krate.get_summary_by_id(id)?; - let type_package_id = if type_summary.crate_id == 0 { - used_by_krate.package_id().to_owned() - } else { - let (owning_crate, owning_crate_version) = used_by_krate - .get_external_crate_name(type_summary.crate_id) - .unwrap(); - if owning_crate.name == "std" { - PackageId::new(STD_PACKAGE_ID) - } else { - let transitive_dependencies = package_graph - .query_forward([used_by_krate.package_id()]) - .unwrap() - .resolve(); - let mut iterator = - transitive_dependencies.links(guppy::graph::DependencyDirection::Forward); - iterator - .find(|link| { - link.to().name() == owning_crate.name - && owning_crate_version - .as_ref() - .map(|v| link.to().version() == v) - .unwrap_or(true) - }) - .ok_or_else(|| { - anyhow!( - "I could not find the package id for the crate where `{}` is defined", - type_summary.path.join("::") - ) - }) - .unwrap() - .to() - .id() - .to_owned() - } - }; - let referenced_base_type_path = type_summary.path.clone(); - let base_type = if type_summary.crate_id == 0 { - used_by_krate.get_importable_path(id) - } else { - // The crate where the type is actually defined. - let source_crate = krate_collection.get_or_compute_by_id(&type_package_id)?; - let type_definition_id = source_crate.get_id_by_path(&referenced_base_type_path)?; - source_crate.get_importable_path(type_definition_id) - } - .to_owned(); + let type_package_id = + krate_collection.get_defining_package_id_for_item(used_by_package_id, id)?; + let base_type = krate_collection.get_canonical_import_path(used_by_package_id, id)?; Ok(ResolvedType { package_id: type_package_id, base_type, From 2880b6eb0f9f4a26a272ecc80e40c700f8a66ac7 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Tue, 20 Sep 2022 14:15:13 +0200 Subject: [PATCH 10/17] Update rustdoc --- libs/pavex/Cargo.toml | 2 +- libs/pavex/src/rustdoc.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/pavex/Cargo.toml b/libs/pavex/Cargo.toml index ed1ac7c1f..3f9b7b935 100644 --- a/libs/pavex/Cargo.toml +++ b/libs/pavex/Cargo.toml @@ -6,7 +6,7 @@ edition = "2021" [dependencies] pavex_builder = { path = "../pavex_builder" } syn = { version = "1.0.96", features = ["full", "extra-traits", "visit"] } -rustdoc-types = "0.14.0" +rustdoc-types = "0.17.0" serde_json = "1" anyhow = "1.0.58" fs-err = "2.7.0" diff --git a/libs/pavex/src/rustdoc.rs b/libs/pavex/src/rustdoc.rs index 5fc6ac63e..15e2804a9 100644 --- a/libs/pavex/src/rustdoc.rs +++ b/libs/pavex/src/rustdoc.rs @@ -49,7 +49,7 @@ fn get_toolchain_crate_data( ) -> Result { // TODO: determine the correct path to the files using `rustup show home`, // `rustup show active-toolchain` and `rustup component list --installed --toolchain <...>`. - let root_folder = PathBuf::from_str("/Users/luca/code/pavex/").unwrap(); + let root_folder = PathBuf::from_str("/Users/luca/code/pavex/json-docs").unwrap(); let json_path = root_folder.join(format!("{}.json", package_id_spec.name)); let json = fs_err::read_to_string(json_path).with_context(|| { format!( From b4586cffa6d83a5fb4cf384f183ba1fa02c4c5a3 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Wed, 21 Sep 2022 13:55:18 +0200 Subject: [PATCH 11/17] Add utility CLI to quickly review snapshots. --- CONTRIBUTING.md | 30 ++++--- .../ui_tests/app_builder/expectations/app.rs | 4 +- .../app_builder/expectations/diagnostics.dot | 4 +- .../expectations/app.rs | 4 +- .../expectations/diagnostics.dot | 2 +- .../expectations/app.rs | 2 +- .../expectations/diagnostics.dot | 2 +- .../expectations/app.rs | 2 +- .../expectations/diagnostics.dot | 2 +- .../expectations/app.rs | 2 +- .../expectations/diagnostics.dot | 2 +- libs/pavex_test_runner/Cargo.toml | 5 ++ libs/pavex_test_runner/src/lib.rs | 2 + libs/pavex_test_runner/src/main.rs | 86 +++++++++++++++++++ libs/pavex_test_runner/src/snapshot.rs | 7 +- 15 files changed, 128 insertions(+), 28 deletions(-) create mode 100644 libs/pavex_test_runner/src/main.rs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c3859539e..93e6b58ab 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,9 +1,7 @@ # Contributing This project is not open to unsolicited code contributions (for the time being). -You are more than free to play around with the code though! The instructions below should be enough to get -you started. I suggest looking at [`ARCHITECTURE.md`](ARCHITECTURE.md) as well to get a sense of the overall project -structure. +You are more than free to play around with the code though! The instructions below should be enough to get you started. I suggest looking at [`ARCHITECTURE.md`](ARCHITECTURE.md) as well to get a sense of the overall project structure. # Prerequisites @@ -19,8 +17,7 @@ cargo nextest run ``` We primarily rely on end-to-end testing to check that `pavex`'s behaviour meets our expectations. -All tests are located in `libs/pavex_cli/tests` and are launched using a custom test runner that you can find -in `libs/pavex_test_runner`. +All tests are located in `libs/pavex_cli/tests` and are launched using a custom test runner that you can find in `libs/pavex_test_runner`. In a nutshell: @@ -30,9 +27,20 @@ In a nutshell: - if the test is expected to pass, we check the generated code and the graph diagnostics; - if the test is expected to fail, we check `stderr` to verify the quality of the error message returned to users. -For each, a runtime environment is created as a sub-folder of `ui_test_envs`, which is in turn generated at the root -of `pavex`'s workspace. -We use a consistent folder to leverage `cargo` caching and speed up successive test runs. It also allows you to easily -inspect the artifacts generated during the test run. -If you suspect that something funny is going on due to cross-run contamination, delete the `ui_test_envs` folder to get -a clean slate. \ No newline at end of file +## Test runtime environment + +For each, a runtime environment is created as a sub-folder of `ui_test_envs`, which is in turn generated at the root of `pavex`'s workspace. +We use a consistent folder to leverage `cargo` caching and speed up successive test runs. It also allows you to easily inspect the artifacts generated during the test run. +If you suspect that something funny is going on due to cross-run contamination, delete the `ui_test_envs` folder to get a clean slate. + +## Updating saved snapshots + +The generated code or the graph diagnostics may not match our expectations. +The test runner will save the unexpected output in a file named like the expectation file with an additional `.snap` suffix. You can then choose to update the saved snapshot via our utility CLI: + +```bash +cargo r --bin snaps +``` + +It will cycle through all `.snap` files and print the changeset with respect to our previous expectations. +You will then be prompted to decide if you want to update the saved snapshot to match the new value or if you prefer to keep it as it. diff --git a/libs/pavex_cli/tests/ui_tests/app_builder/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/app_builder/expectations/app.rs index f90bc0489..9a0bdd1cb 100644 --- a/libs/pavex_cli/tests/ui_tests/app_builder/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/app_builder/expectations/app.rs @@ -63,8 +63,8 @@ fn route_request( } pub fn route_handler_0( v0: app::HttpClient, - v1: http::request::Request, -) -> http::response::Response { + v1: http::Request, +) -> http::Response { let v2 = app::extract_path(v1); let v3 = app::logger(); app::stream_file(v2, v3, v0) diff --git a/libs/pavex_cli/tests/ui_tests/app_builder/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/app_builder/expectations/diagnostics.dot index 54a137978..845429756 100644 --- a/libs/pavex_cli/tests/ui_tests/app_builder/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/app_builder/expectations/diagnostics.dot @@ -1,9 +1,9 @@ digraph "/home" { - 0 [ label = "app::stream_file(std::path::PathBuf, app::Logger, app::HttpClient) -> http::response::Response"] + 0 [ label = "app::stream_file(std::path::PathBuf, app::Logger, app::HttpClient) -> http::Response"] 1 [ label = "app::HttpClient"] 2 [ label = "app::Logger"] 3 [ label = "std::path::PathBuf"] - 4 [ label = "http::request::Request"] + 4 [ label = "http::Request"] 1 -> 0 [ ] 2 -> 0 [ ] 3 -> 0 [ ] diff --git a/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/app.rs index 07fc1b7aa..abcbefd8d 100644 --- a/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/app.rs @@ -61,8 +61,6 @@ fn route_request( _ => panic!("This is a bug, no route registered for a route id"), } } -pub fn route_handler_0( - v0: app::Streamer, -) -> http::response::Response { +pub fn route_handler_0(v0: app::Streamer) -> http::Response { app::stream_file(v0) } \ No newline at end of file diff --git a/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/diagnostics.dot index 8ce509f62..d9604820c 100644 --- a/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/pattern_bindings_in_input_parameters_are_supported/expectations/diagnostics.dot @@ -1,5 +1,5 @@ digraph "/home" { - 0 [ label = "app::stream_file(app::Streamer) -> http::response::Response"] + 0 [ label = "app::stream_file(app::Streamer) -> http::Response"] 1 [ label = "app::Streamer"] 1 -> 0 [ ] } diff --git a/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/app.rs index 11f648570..bcc3ce7c1 100644 --- a/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/app.rs @@ -58,6 +58,6 @@ fn route_request( _ => panic!("This is a bug, no route registered for a route id"), } } -pub fn route_handler_0() -> http::response::Response { +pub fn route_handler_0() -> http::Response { app::Streamer::stream_file() } \ No newline at end of file diff --git a/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/diagnostics.dot index 79dca5e90..937b65650 100644 --- a/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/static_methods_are_supported/expectations/diagnostics.dot @@ -1,5 +1,5 @@ digraph "/home" { - 0 [ label = "app::Streamer::stream_file() -> http::response::Response"] + 0 [ label = "app::Streamer::stream_file() -> http::Response"] } digraph app_state { 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] diff --git a/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/app.rs index 8bb419681..d01e2c41f 100644 --- a/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/app.rs @@ -58,7 +58,7 @@ fn route_request( _ => panic!("This is a bug, no route registered for a route id"), } } -pub fn route_handler_0() -> http::response::Response { +pub fn route_handler_0() -> http::Response { let v0 = dep::new_logger(); app::Streamer::stream_file(v0) } \ No newline at end of file diff --git a/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/diagnostics.dot index 28c97e0dc..fad4809a0 100644 --- a/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/the_latest_registered_constructor_takes_precedence/expectations/diagnostics.dot @@ -1,5 +1,5 @@ digraph "/home" { - 0 [ label = "app::Streamer::stream_file(dep::Logger) -> http::response::Response"] + 0 [ label = "app::Streamer::stream_file(dep::Logger) -> http::Response"] 1 [ label = "dep::Logger"] 1 -> 0 [ ] } diff --git a/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/app.rs b/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/app.rs index 63b15f7bf..730dd2c7e 100644 --- a/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/app.rs +++ b/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/app.rs @@ -58,7 +58,7 @@ fn route_request( _ => panic!("This is a bug, no route registered for a route id"), } } -pub fn route_handler_0() -> http::response::Response { +pub fn route_handler_0() -> http::Response { let v0 = app::new_logger(); app::Streamer::stream_file(v0) } \ No newline at end of file diff --git a/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/diagnostics.dot b/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/diagnostics.dot index c073d20f9..76a94095e 100644 --- a/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/diagnostics.dot +++ b/libs/pavex_cli/tests/ui_tests/the_same_constructor_can_be_registered_multiple_times_using_the_same_lifecycle/expectations/diagnostics.dot @@ -1,5 +1,5 @@ digraph "/home" { - 0 [ label = "app::Streamer::stream_file(app::Logger) -> http::response::Response"] + 0 [ label = "app::Streamer::stream_file(app::Logger) -> http::Response"] 1 [ label = "app::Logger"] 1 -> 0 [ ] } diff --git a/libs/pavex_test_runner/Cargo.toml b/libs/pavex_test_runner/Cargo.toml index e1a315efb..c16fc543e 100644 --- a/libs/pavex_test_runner/Cargo.toml +++ b/libs/pavex_test_runner/Cargo.toml @@ -3,6 +3,10 @@ name = "pavex_test_runner" version = "0.1.0" edition = "2021" +[[bin]] +path = "src/main.rs" +name = "snaps" + [dependencies] anyhow = "1.0.59" console = "0.15.1" @@ -12,3 +16,4 @@ serde = { version = "1.0.141", features = ["derive"] } similar = { version = "2.2.0", features = ["inline"] } textwrap = "0.15.0" toml = { version = "0.5.9", features = ["preserve_order"] } +miette = "5" \ No newline at end of file diff --git a/libs/pavex_test_runner/src/lib.rs b/libs/pavex_test_runner/src/lib.rs index df01d2545..268b74cce 100644 --- a/libs/pavex_test_runner/src/lib.rs +++ b/libs/pavex_test_runner/src/lib.rs @@ -8,6 +8,8 @@ use libtest_mimic::Conclusion; use libtest_mimic::Outcome; use toml::toml; +pub use snapshot::print_changeset; + use crate::snapshot::SnapshotTest; mod snapshot; diff --git a/libs/pavex_test_runner/src/main.rs b/libs/pavex_test_runner/src/main.rs new file mode 100644 index 000000000..974eaef6a --- /dev/null +++ b/libs/pavex_test_runner/src/main.rs @@ -0,0 +1,86 @@ +use std::path::PathBuf; + +use console::{style, Key}; + +use pavex_test_runner::print_changeset; + +fn main() -> Result<(), Box> { + miette::set_hook(Box::new(move |_| { + let config = miette::MietteHandlerOpts::new(); + Box::new(config.build()) + })) + .unwrap(); + let test_folder: PathBuf = "libs/pavex_cli/tests/ui_tests".into(); + let terminal = console::Term::stdout(); + for ui_test_dir in fs_err::read_dir(&test_folder)? { + let ui_test_dir = ui_test_dir?; + assert!(ui_test_dir.metadata()?.is_dir()); + let test_name = ui_test_dir.file_name().to_string_lossy().to_string(); + let expectations_dir = ui_test_dir.path().join("expectations"); + for file in fs_err::read_dir(&expectations_dir)? { + let file = file?; + let file_name = file.file_name().to_string_lossy().to_string(); + if let Some(expected_filename) = file_name.strip_suffix(".snap") { + let actual_snapshot = fs_err::read_to_string(file.path())?; + let expected_path = expectations_dir.join(expected_filename); + let expected_snapshot = fs_err::read_to_string(&expected_path)?; + + terminal.clear_screen()?; + println!( + "{}{}\n{}{}", + style("Test name: ").bold(), + style(format!("{}", &test_name)).yellow().bold(), + style("Snapshot name: ").bold(), + style(format!("{}", &expected_filename)).green().bold(), + ); + print_changeset(&expected_snapshot, &actual_snapshot); + let decision = prompt(&terminal)?; + + match decision { + Decision::Accept => { + fs_err::rename(file.path(), &expected_path)?; + } + Decision::Reject => { + fs_err::remove_file(file.path())?; + } + Decision::Skip => continue, + } + } + } + } + + Ok(()) +} + +fn prompt(terminal: &console::Term) -> std::io::Result { + println!(); + println!( + " {} accept {}", + style("a").green().bold(), + style("keep the new snapshot").dim() + ); + println!( + " {} reject {}", + style("r").red().bold(), + style("keep the old snapshot").dim() + ); + println!( + " {} skip {}", + style("s").yellow().bold(), + style("keep both for now").dim() + ); + loop { + match terminal.read_key()? { + Key::Char('a') | Key::Enter => return Ok(Decision::Accept), + Key::Char('r') | Key::Escape => return Ok(Decision::Reject), + Key::Char('s') | Key::Char(' ') => return Ok(Decision::Skip), + _ => {} + } + } +} + +enum Decision { + Accept, + Reject, + Skip, +} diff --git a/libs/pavex_test_runner/src/snapshot.rs b/libs/pavex_test_runner/src/snapshot.rs index a8adcbcbd..2b27b3272 100644 --- a/libs/pavex_test_runner/src/snapshot.rs +++ b/libs/pavex_test_runner/src/snapshot.rs @@ -1,9 +1,10 @@ -use console::style; -use similar::{Algorithm, ChangeTag, TextDiff}; use std::io::ErrorKind; use std::path::PathBuf; use std::time::Duration; +use console::style; +use similar::{Algorithm, ChangeTag, TextDiff}; + fn term_width() -> usize { console::Term::stdout().size().1 as usize } @@ -46,7 +47,7 @@ impl SnapshotTest { } } -fn print_changeset(old: &str, new: &str) { +pub fn print_changeset(old: &str, new: &str) { let width = term_width(); let diff = TextDiff::configure() .algorithm(Algorithm::Patience) From 3ce5ba86fdca23565a7396b5698ee69e0e65b571 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Wed, 21 Sep 2022 14:08:14 +0200 Subject: [PATCH 12/17] Specify requirements. --- CONTRIBUTING.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 93e6b58ab..a85517bdf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -29,7 +29,7 @@ In a nutshell: ## Test runtime environment -For each, a runtime environment is created as a sub-folder of `ui_test_envs`, which is in turn generated at the root of `pavex`'s workspace. +For each test, a runtime environment is created as a sub-folder of `ui_test_envs`, which is in turn generated at the root of `pavex`'s workspace. We use a consistent folder to leverage `cargo` caching and speed up successive test runs. It also allows you to easily inspect the artifacts generated during the test run. If you suspect that something funny is going on due to cross-run contamination, delete the `ui_test_envs` folder to get a clean slate. @@ -39,6 +39,7 @@ The generated code or the graph diagnostics may not match our expectations. The test runner will save the unexpected output in a file named like the expectation file with an additional `.snap` suffix. You can then choose to update the saved snapshot via our utility CLI: ```bash +# It must be run from the root folder of the workspace cargo r --bin snaps ``` From d81cee25076fca24365794420e431928155e628c Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Wed, 21 Sep 2022 14:10:03 +0200 Subject: [PATCH 13/17] Refactor. --- libs/pavex_test_runner/src/main.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/pavex_test_runner/src/main.rs b/libs/pavex_test_runner/src/main.rs index 974eaef6a..3380f255c 100644 --- a/libs/pavex_test_runner/src/main.rs +++ b/libs/pavex_test_runner/src/main.rs @@ -34,9 +34,8 @@ fn main() -> Result<(), Box> { style(format!("{}", &expected_filename)).green().bold(), ); print_changeset(&expected_snapshot, &actual_snapshot); - let decision = prompt(&terminal)?; - match decision { + match prompt(&terminal)? { Decision::Accept => { fs_err::rename(file.path(), &expected_path)?; } From 7dbec25ba42665f10475f109d18900a33957e21d Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Wed, 21 Sep 2022 14:12:55 +0200 Subject: [PATCH 14/17] Refactor. --- libs/pavex_test_runner/src/main.rs | 38 +++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/libs/pavex_test_runner/src/main.rs b/libs/pavex_test_runner/src/main.rs index 3380f255c..7213b1f67 100644 --- a/libs/pavex_test_runner/src/main.rs +++ b/libs/pavex_test_runner/src/main.rs @@ -25,17 +25,13 @@ fn main() -> Result<(), Box> { let expected_path = expectations_dir.join(expected_filename); let expected_snapshot = fs_err::read_to_string(&expected_path)?; - terminal.clear_screen()?; - println!( - "{}{}\n{}{}", - style("Test name: ").bold(), - style(format!("{}", &test_name)).yellow().bold(), - style("Snapshot name: ").bold(), - style(format!("{}", &expected_filename)).green().bold(), - ); - print_changeset(&expected_snapshot, &actual_snapshot); - - match prompt(&terminal)? { + match review_snapshot( + &terminal, + &test_name, + expected_filename, + &expected_snapshot, + &actual_snapshot, + )? { Decision::Accept => { fs_err::rename(file.path(), &expected_path)?; } @@ -51,6 +47,26 @@ fn main() -> Result<(), Box> { Ok(()) } +fn review_snapshot( + terminal: &console::Term, + test_name: &str, + snapshot_name: &str, + expected_snapshot: &str, + actual_snapshot: &str, +) -> std::io::Result { + terminal.clear_screen()?; + println!( + "{}{}\n{}{}", + style("Test name: ").bold(), + style(format!("{}", &test_name)).yellow().bold(), + style("Snapshot name: ").bold(), + style(format!("{}", &snapshot_name)).green().bold(), + ); + print_changeset(&expected_snapshot, &actual_snapshot); + + prompt(&terminal) +} + fn prompt(terminal: &console::Term) -> std::io::Result { println!(); println!( From 689cf4dfc3aef336e8773b3711f5addcfdb2059c Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Wed, 21 Sep 2022 14:16:19 +0200 Subject: [PATCH 15/17] Clarify --- libs/pavex/src/language/resolved_type.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/pavex/src/language/resolved_type.rs b/libs/pavex/src/language/resolved_type.rs index d13591b8f..8538f637b 100644 --- a/libs/pavex/src/language/resolved_type.rs +++ b/libs/pavex/src/language/resolved_type.rs @@ -12,6 +12,8 @@ use crate::language::ImportPath; pub struct ResolvedType { #[serde(serialize_with = "serialize_package_id")] #[serde(deserialize_with = "deserialize_package_id")] + // `PackageId` does not implement serde::Deserialize/serde::Serialize, therefore we must + // manually specify deserializer and serializer for make the whole `ResolvedType` (de)serializable. pub package_id: PackageId, pub base_type: ImportPath, pub generic_arguments: Vec, From cf906dd20f35e1dd8146f2026980036811513831 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sun, 2 Oct 2022 16:55:21 +0100 Subject: [PATCH 16/17] Do not use `syn` in the path-related types in `language`. Ensure that all generic type parameters are using fully-qualified paths, instead of checking just the base path. --- libs/pavex/src/language/callable_path.rs | 140 ++++++++++++-- libs/pavex/src/language/mod.rs | 10 +- libs/pavex/src/language/resolved_path.rs | 183 +++++++++++------- libs/pavex/src/lib.rs | 4 +- libs/pavex/src/rustdoc.rs | 3 +- libs/pavex/src/web/app.rs | 38 ++-- .../src/web/application_state_call_graph.rs | 44 +++-- libs/pavex/src/web/codegen.rs | 27 +-- libs/pavex/src/web/codegen_utils.rs | 17 +- libs/pavex/src/web/handler_call_graph.rs | 10 +- 10 files changed, 322 insertions(+), 154 deletions(-) diff --git a/libs/pavex/src/language/callable_path.rs b/libs/pavex/src/language/callable_path.rs index fb8105f88..101242e5a 100644 --- a/libs/pavex/src/language/callable_path.rs +++ b/libs/pavex/src/language/callable_path.rs @@ -1,18 +1,20 @@ use std::fmt::{Display, Formatter}; -use quote::quote; -use syn::ExprPath; +use syn::{ExprPath, GenericArgument, PathArguments, Type}; use pavex_builder::RawCallableIdentifiers; /// A path that can be used in expression position (i.e. to refer to a function or a static method). #[derive(Clone, Debug, Hash, Eq, PartialEq)] -pub(crate) struct CallPath(pub ExprPath); +pub(crate) struct CallPath { + pub has_leading_colon: bool, + pub segments: Vec, +} -impl AsRef for CallPath { - fn as_ref(&self) -> &ExprPath { - &self.0 - } +#[derive(Clone, Debug, Hash, Eq, PartialEq)] +pub(crate) struct CallPathSegment { + pub ident: syn::Ident, + pub generic_arguments: Vec, } impl CallPath { @@ -22,7 +24,48 @@ impl CallPath { raw_identifiers: callable_identifiers.to_owned(), parsing_error: e, })?; - Ok(Self(callable_path)) + Self::parse_from_path(callable_path.path) + } + + pub(crate) fn parse_from_path(path: syn::Path) -> Result { + let has_leading_colon = path.leading_colon.is_some(); + let mut segments = Vec::with_capacity(path.segments.len()); + for syn_segment in path.segments { + let generic_arguments = match syn_segment.arguments { + PathArguments::None => vec![], + PathArguments::AngleBracketed(syn_arguments) => { + let mut arguments = Vec::with_capacity(syn_arguments.args.len()); + for syn_argument in syn_arguments.args { + let argument = match syn_argument { + GenericArgument::Type(p) => match p { + Type::Path(p) => Self::parse_from_path(p.path)?, + _ => unreachable!(), + }, + GenericArgument::Lifetime(_) + | GenericArgument::Binding(_) + | GenericArgument::Constraint(_) + | GenericArgument::Const(_) => todo!( + "We can only handle generic type parameters for the time being." + ), + }; + arguments.push(argument) + } + arguments + } + PathArguments::Parenthesized(_) => { + todo!("We do not handle paranthesized generic parameters") + } + }; + let segment = CallPathSegment { + ident: syn_segment.ident, + generic_arguments, + }; + segments.push(segment) + } + Ok(Self { + has_leading_colon, + segments, + }) } /// Return the first segment in the path. @@ -31,24 +74,83 @@ impl CallPath { /// return `my_module`. pub fn leading_path_segment(&self) -> &proc_macro2::Ident { // This unwrap never fails thanks to the validation done in `parse` - &self.0.path.segments.first().unwrap().ident + &self.segments.first().unwrap().ident } } -impl std::fmt::Display for CallPath { +impl Display for CallPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let path = &self.0; - let s = quote! { #path }.to_string(); - write!( - f, - "{}", - s.replace(" :: ", "::") - .replace("< ", "<") - .replace(" >", ">") - ) + let leading_colon = if self.has_leading_colon { "::" } else { "" }; + write!(f, "{}", leading_colon)?; + let last_segment_index = self.segments.len().saturating_sub(1); + for (i, segment) in self.segments.iter().enumerate() { + write!(f, "{}", segment)?; + if i != last_segment_index { + write!(f, "::")?; + } + } + Ok(()) } } +impl Display for CallPathSegment { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.ident)?; + let last_argument_index = self.generic_arguments.len().saturating_sub(1); + for (j, argument) in self.generic_arguments.iter().enumerate() { + write!(f, "{}", argument)?; + if j != last_argument_index { + write!(f, ", ")?; + } + } + Ok(()) + } +} + +// /// A path that can be used in expression position (i.e. to refer to a function or a static method). +// #[derive(Clone, Debug, Hash, Eq, PartialEq)] +// pub(crate) struct CallPath(pub ExprPath); +// +// impl AsRef for CallPath { +// fn as_ref(&self) -> &ExprPath { +// &self.0 +// } +// } +// +// impl CallPath { +// pub fn parse(callable_identifiers: &RawCallableIdentifiers) -> Result { +// let callable_path: ExprPath = +// syn::parse_str(callable_identifiers.raw_path()).map_err(|e| InvalidCallPath { +// raw_identifiers: callable_identifiers.to_owned(), +// parsing_error: e, +// })?; +// Ok(Self(callable_path)) +// } +// +// /// Return the first segment in the path. +// /// +// /// E.g. `my_crate::my_module::MyType` will return `my_crate` while `my_module::MyType` will +// /// return `my_module`. +// pub fn leading_path_segment(&self) -> &proc_macro2::Ident { +// // This unwrap never fails thanks to the validation done in `parse` +// &self.0.path.segments.first().unwrap().ident +// } +// } +// +// impl std::fmt::Display for CallPath { +// fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { +// let path = &self.0; +// let s = quote! { #path }.to_string(); +// write!( +// f, +// "{}", +// s.replace(" :: ", "::") +// .replace("< ", "<") +// .replace(" >", ">") +// ) +// } +// } + #[derive(Debug, thiserror::Error)] pub(crate) struct InvalidCallPath { pub raw_identifiers: RawCallableIdentifiers, diff --git a/libs/pavex/src/language/mod.rs b/libs/pavex/src/language/mod.rs index f43c10129..7f970a0d1 100644 --- a/libs/pavex/src/language/mod.rs +++ b/libs/pavex/src/language/mod.rs @@ -1,3 +1,8 @@ +pub(crate) use callable::Callable; +pub(crate) use callable_path::{CallPath, InvalidCallPath}; +pub(crate) use resolved_path::{ParseError, ResolvedPath, ResolvedPathSegment, UnknownPath}; +pub(crate) use resolved_type::ResolvedType; + mod callable; mod callable_path; mod resolved_path; @@ -5,8 +10,3 @@ mod resolved_type; // E.g. `["std", "path", "PathBuf"]`. pub type ImportPath = Vec; - -pub(crate) use callable::Callable; -pub(crate) use callable_path::{CallPath, InvalidCallPath}; -pub(crate) use resolved_path::{EncodedResolvedPath, ParseError, ResolvedPath, UnknownPath}; -pub(crate) use resolved_type::ResolvedType; diff --git a/libs/pavex/src/language/resolved_path.rs b/libs/pavex/src/language/resolved_path.rs index df53b289f..88f4f262a 100644 --- a/libs/pavex/src/language/resolved_path.rs +++ b/libs/pavex/src/language/resolved_path.rs @@ -1,16 +1,18 @@ +use std::fmt::Write; use std::fmt::{Display, Formatter}; use std::hash::Hash; +use anyhow::Context; +use bimap::BiHashMap; use guppy::PackageId; use itertools::Itertools; use quote::format_ident; use rustdoc_types::{Item, ItemEnum}; -use syn::ExprPath; use pavex_builder::RawCallableIdentifiers; use crate::language::{CallPath, InvalidCallPath}; -use crate::rustdoc::CrateCollection; +use crate::rustdoc::{CrateCollection, STD_PACKAGE_ID, TOOLCHAIN_CRATES}; /// A resolved import path. /// @@ -35,23 +37,27 @@ use crate::rustdoc::CrateCollection; /// and well-defined within a `cargo` workspace. #[derive(Clone, Debug, Hash, Eq)] pub(crate) struct ResolvedPath { - pub path: CallPath, + pub segments: Vec, pub package_id: PackageId, } +#[derive(Clone, Debug, Hash, Eq, PartialEq)] +pub(crate) struct ResolvedPathSegment { + pub ident: String, + pub generic_arguments: Vec, +} + impl PartialEq for ResolvedPath { fn eq(&self, other: &Self) -> bool { - let is_equal = self.package_id == other.package_id - && self.path.0.attrs == other.path.0.attrs - && self.path.0.qself == other.path.0.qself - && self.path.0.path.segments.len() == other.path.0.path.segments.len(); + let is_equal = + self.package_id == other.package_id && self.segments.len() == other.segments.len(); if is_equal { // We want to ignore the first segment of the path, because dependencies can be // renamed and this can lead to equivalent paths not being considered equal. // Given that we already have the package id as part of the type, it is safe // to disregard the first segment when determining equality. - let self_segments = self.path.0.path.segments.iter().skip(1); - let other_segments = other.path.0.path.segments.iter().skip(1); + let self_segments = self.segments.iter().skip(1); + let other_segments = other.segments.iter().skip(1); for (self_segment, other_segment) in self_segments.zip_eq(other_segments) { if self_segment != other_segment { return false; @@ -70,25 +76,43 @@ impl ResolvedPath { graph: &guppy::graph::PackageGraph, ) -> Result { let mut path = CallPath::parse(identifiers)?; - // Normalize by removing the leading semicolon for external paths. - path.0.path.leading_colon = None; if path.leading_path_segment() == "crate" { let first_segment = path - .0 - .path .segments .first_mut() - .expect("Bug: an `ExprPath` with no path segments!"); + .expect("Bug: a `CallPath` with no path segments!"); // Unwrapping here is safe: there is always at least one path segment in a successfully // parsed `ExprPath`. first_segment.ident = format_ident!("{}", identifiers.registered_at()); } + Self::parse_call_path(path, identifiers, graph) + } + + fn parse_call_path( + path: CallPath, + identifiers: &RawCallableIdentifiers, + graph: &guppy::graph::PackageGraph, + ) -> Result { + let registered_at = identifiers.registered_at(); + let krate_name_candidate = path.leading_path_segment().to_string(); + + let mut segments = vec![]; + for raw_segment in path.segments { + let generic_arguments = raw_segment + .generic_arguments + .into_iter() + .map(|arg| Self::parse_call_path(arg, identifiers, graph)) + .collect::, _>>()?; + let segment = ResolvedPathSegment { + ident: raw_segment.ident.to_string(), + generic_arguments, + }; + segments.push(segment); + } - let registration_crate_name = identifiers.registered_at(); let registration_package = graph.packages() - .find(|p| p.name() == registration_crate_name) + .find(|p| p.name() == registered_at) .expect("There is no package in the current workspace whose name matches the registration crate for these identifiers"); - let krate_name_candidate = path.leading_path_segment().to_string(); let package_id = if registration_package.name() == krate_name_candidate { registration_package.id().to_owned() } else if let Some(dependency) = registration_package @@ -96,20 +120,26 @@ impl ResolvedPath { .find(|d| d.resolved_name() == krate_name_candidate) { dependency.to().id().to_owned() + } else if TOOLCHAIN_CRATES.contains(&krate_name_candidate.as_str()) { + PackageId::new(STD_PACKAGE_ID) } else { return Err(PathMustBeAbsolute { raw_identifiers: identifiers.to_owned(), } .into()); }; - Ok(Self { path, package_id }) + Ok(Self { + segments, + package_id, + }) } /// Return the name of the crate that this type belongs to. /// /// E.g. `my_crate::my_module::MyType` will return `my_crate`. - pub fn crate_name(&self) -> &proc_macro2::Ident { - self.path.leading_path_segment() + pub fn crate_name(&self) -> &str { + // This unwrap never fails thanks to the validation done in `parse` + &self.segments.first().unwrap().ident } /// Find information about the type that the path points at. @@ -119,9 +149,6 @@ impl ResolvedPath { .get_or_compute_by_id(&self.package_id) .unwrap(); let path_segments: Vec<_> = self - .path - .0 - .path .segments .iter() .map(|path_segment| path_segment.ident.to_string()) @@ -162,11 +189,68 @@ impl ResolvedPath { } Err(UnknownPath(self.to_owned().into())) } + + pub fn render_path(&self, id2name: &BiHashMap<&PackageId, String>) -> String { + let mut buffer = String::new(); + let crate_name = id2name + .get_by_left(&self.package_id) + .with_context(|| { + format!( + "The package id '{}' is missing from the id<>name mapping for crates.", + self.package_id + ) + }) + .unwrap(); + write!(&mut buffer, "{}", crate_name).unwrap(); + for path_segment in &self.segments[1..] { + write!(&mut buffer, "::{}", path_segment.ident).unwrap(); + let generic_arguments = &path_segment.generic_arguments; + if !generic_arguments.is_empty() { + write!(&mut buffer, "<").unwrap(); + let mut arguments = generic_arguments.iter().peekable(); + while let Some(argument) = arguments.next() { + write!(&mut buffer, "{}", argument.render_path(id2name)).unwrap(); + if arguments.peek().is_some() { + write!(&mut buffer, ", ").unwrap(); + } + } + write!(&mut buffer, ">").unwrap(); + } + } + buffer + } } impl Display for ResolvedPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.path) + let last_segment_index = self.segments.len().saturating_sub(1); + for (i, segment) in self.segments.iter().enumerate() { + write!(f, "{}", segment)?; + if i != last_segment_index { + write!(f, "::")?; + } + } + Ok(()) + } +} + +impl Display for ResolvedPathSegment { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.ident)?; + if !self.generic_arguments.is_empty() { + write!(f, "::<")?; + } + let last_argument_index = self.generic_arguments.len().saturating_sub(1); + for (j, argument) in self.generic_arguments.iter().enumerate() { + write!(f, "{}", argument)?; + if j != last_argument_index { + write!(f, ", ")?; + } + } + if !self.generic_arguments.is_empty() { + write!(f, ">")?; + } + Ok(()) } } @@ -190,59 +274,16 @@ impl Display for PathMustBeAbsolute { } } -impl AsRef for ResolvedPath { - fn as_ref(&self) -> &ExprPath { - self.path.as_ref() - } -} - -#[derive(Clone, Debug, Hash, Eq, PartialEq)] -/// Why does this type even exist? -/// -/// It turns out that most of `syn`'s types are neither `Send` nor `Sync` because, deep down, they -/// are holding on to one or more `Span`s, which are not `Send` nor `Sync`. -/// -/// As a consequence, we can't store `syn`'s types as fields on our error types, because `miette` -/// expects the source error types to be `Send` and `Sync`. -/// -/// This type is a crutch - every time we need to store a [`ResolvedPath`] in an error -/// type, we encode it as a string. By wrapping it in a new type, we make it obvious what that -/// string means and we keep track of the fact that this can be infallibly deserialized back -/// into a `syn::ExprPath`. -pub(crate) struct EncodedResolvedPath(String, PackageId); - -impl EncodedResolvedPath { - pub fn decode(&self) -> ResolvedPath { - ResolvedPath { - path: CallPath(syn::parse_str(&self.0).unwrap()), - package_id: self.1.clone(), - } - } -} - -impl std::fmt::Display for EncodedResolvedPath { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let decoded = self.decode(); - std::fmt::Display::fmt(&decoded, f) - } -} - -impl From for EncodedResolvedPath { - fn from(p: ResolvedPath) -> Self { - Self(p.path.to_string(), p.package_id) - } -} - #[derive(thiserror::Error, Debug)] -pub(crate) struct UnknownPath(pub EncodedResolvedPath); +pub(crate) struct UnknownPath(pub ResolvedPath); impl Display for UnknownPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - let decoded = self.0.decode(); - let krate = decoded.crate_name().to_string(); + let path = &self.0; + let krate = path.crate_name().to_string(); write!( f, - "I could not find '{decoded}' in the auto-generated documentation for '{krate}'" + "I could not find '{path}' in the auto-generated documentation for '{krate}'" ) } } diff --git a/libs/pavex/src/lib.rs b/libs/pavex/src/lib.rs index ca9a009a7..54d964d76 100644 --- a/libs/pavex/src/lib.rs +++ b/libs/pavex/src/lib.rs @@ -1,8 +1,6 @@ -extern crate core; +pub use web::App; mod graphmap; pub(crate) mod language; mod rustdoc; mod web; - -pub use web::App; diff --git a/libs/pavex/src/rustdoc.rs b/libs/pavex/src/rustdoc.rs index 15e2804a9..ed755bb45 100644 --- a/libs/pavex/src/rustdoc.rs +++ b/libs/pavex/src/rustdoc.rs @@ -19,7 +19,8 @@ pub struct CannotGetCrateData { pub source: anyhow::Error, } -const TOOLCHAIN_CRATES: [&str; 3] = ["std", "core", "alloc"]; +pub const STD_PACKAGE_ID: &str = "std"; +pub const TOOLCHAIN_CRATES: [&str; 3] = ["std", "core", "alloc"]; pub fn get_crate_data( root_folder: &Path, diff --git a/libs/pavex/src/web/app.rs b/libs/pavex/src/web/app.rs index 08ac78a29..b5a1a2089 100644 --- a/libs/pavex/src/web/app.rs +++ b/libs/pavex/src/web/app.rs @@ -10,7 +10,7 @@ use guppy::PackageId; use indexmap::{IndexMap, IndexSet}; use miette::{miette, NamedSource, SourceSpan}; use proc_macro2::{Ident, TokenStream}; -use quote::{format_ident, quote}; +use quote::format_ident; use rustdoc_types::{GenericArg, GenericArgs, ItemEnum, Type}; use syn::spanned::Spanned; use syn::FnArg; @@ -19,8 +19,8 @@ use pavex_builder::Lifecycle; use pavex_builder::{AppBlueprint, RawCallableIdentifiers}; use crate::language::{Callable, ParseError, ResolvedType}; -use crate::language::{EncodedResolvedPath, ResolvedPath, UnknownPath}; -use crate::rustdoc::{CannotGetCrateData, CrateCollection}; +use crate::language::{ResolvedPath, UnknownPath}; +use crate::rustdoc::{CannotGetCrateData, CrateCollection, STD_PACKAGE_ID}; use crate::web::application_state_call_graph::ApplicationStateCallGraph; use crate::web::dependency_graph::CallableDependencyGraph; use crate::web::diagnostic::{ @@ -30,7 +30,6 @@ use crate::web::diagnostic::{ use crate::web::handler_call_graph::HandlerCallGraph; use crate::web::{codegen, diagnostic}; -pub(crate) const STD_PACKAGE_ID: &str = "std"; pub(crate) const GENERATED_APP_PACKAGE_ID: &str = "crate"; pub struct App { @@ -270,11 +269,9 @@ impl App { // We only report a single registration site in the error report even though // the same callable might have been registered in multiple locations. // We may or may not want to change this in the future. - let type_path = e.0.decode(); - let raw_identifier = resolved_paths2identifiers[&type_path] - .iter() - .next() - .unwrap(); + let type_path = &e.0; + let raw_identifier = + resolved_paths2identifiers[type_path].iter().next().unwrap(); let location = app_blueprint.handler_locations[raw_identifier] .first() .unwrap(); @@ -357,8 +354,8 @@ impl App { } }; - let callable_path = e.callable_path.decode(); - let raw_identifier = resolved_paths2identifiers[&callable_path] + let callable_path = &e.callable_path; + let raw_identifier = resolved_paths2identifiers[callable_path] .iter() .next() .unwrap(); @@ -635,7 +632,7 @@ pub(crate) enum BuildError { fn process_constructors( constructor_paths: &IndexSet, krate_collection: &mut CrateCollection, - package_graph: &guppy::graph::PackageGraph, + package_graph: &PackageGraph, ) -> Result< ( BiHashMap, @@ -659,7 +656,7 @@ fn process_constructors( fn process_constructor( constructor_path: &ResolvedPath, krate_collection: &mut CrateCollection, - package_graph: &guppy::graph::PackageGraph, + package_graph: &PackageGraph, ) -> Result { let constructor = process_callable(krate_collection, constructor_path, package_graph)?; if constructor.output_fq_path.base_type == vec!["()"] { @@ -675,7 +672,7 @@ fn process_constructor( fn process_handlers( handler_paths: &IndexSet, krate_collection: &mut CrateCollection, - package_graph: &guppy::graph::PackageGraph, + package_graph: &PackageGraph, ) -> Result<(HashMap, IndexSet), CallableResolutionError> { let mut handlers = IndexSet::with_capacity(handler_paths.len()); let mut handler_resolver = HashMap::new(); @@ -688,7 +685,7 @@ fn process_handlers( } fn process_type( - type_: &rustdoc_types::Type, + type_: &Type, // The package id where the type we are trying to process has been referenced (e.g. as an // input/output parameter). used_by_package_id: &PackageId, @@ -759,9 +756,8 @@ fn process_callable( ItemEnum::Function(f) => &f.decl, ItemEnum::Method(m) => &m.decl, kind => { - let path = &callable_path.path.0; return Err(UnsupportedCallableKind { - import_path: quote! { #path }.to_string(), + import_path: callable_path.to_string(), // TODO: review how this gets formatted item_kind: format!("{:?}", kind), } @@ -849,9 +845,9 @@ pub(crate) struct UnsupportedCallableKind { #[derive(Debug, thiserror::Error)] #[error("One of the input parameters for `{callable_path}` has a type that I cannot handle.")] pub(crate) struct ParameterResolutionError { - pub callable_path: EncodedResolvedPath, + pub callable_path: ResolvedPath, pub callable_item: rustdoc_types::Item, - pub parameter_type: rustdoc_types::Type, + pub parameter_type: Type, pub parameter_index: usize, #[source] pub source: anyhow::Error, @@ -860,9 +856,9 @@ pub(crate) struct ParameterResolutionError { #[derive(Debug, thiserror::Error)] #[error("I do not know how to handle the type returned by `{callable_path}`.")] pub(crate) struct OutputTypeResolutionError { - pub callable_path: EncodedResolvedPath, + pub callable_path: ResolvedPath, pub callable_item: rustdoc_types::Item, - pub output_type: rustdoc_types::Type, + pub output_type: Type, #[source] pub source: anyhow::Error, } diff --git a/libs/pavex/src/web/application_state_call_graph.rs b/libs/pavex/src/web/application_state_call_graph.rs index 445f29cbc..a65815110 100644 --- a/libs/pavex/src/web/application_state_call_graph.rs +++ b/libs/pavex/src/web/application_state_call_graph.rs @@ -9,11 +9,11 @@ use petgraph::visit::{DfsPostOrder, Reversed}; use petgraph::Direction; use proc_macro2::{Ident, TokenStream}; use quote::{quote, ToTokens}; -use syn::{parse_quote, ExprStruct, ItemFn}; +use syn::{ExprStruct, ItemFn}; use pavex_builder::Lifecycle; -use crate::language::{CallPath, Callable, ResolvedPath, ResolvedType}; +use crate::language::{Callable, ResolvedPath, ResolvedPathSegment, ResolvedType}; use crate::web::app::GENERATED_APP_PACKAGE_ID; use crate::web::codegen_utils::{codegen_call_block, Fragment, VariableNameGenerator}; use crate::web::dependency_graph::{CallableDependencyGraph, DependencyGraphNode}; @@ -43,7 +43,16 @@ impl ApplicationStateCallGraph { generic_arguments: vec![], }, callable_fq_path: ResolvedPath { - path: CallPath(syn::parse_str("crate::ApplicationState").unwrap()), + segments: vec![ + ResolvedPathSegment { + ident: "crate".into(), + generic_arguments: vec![], + }, + ResolvedPathSegment { + ident: "ApplicationState".into(), + generic_arguments: vec![], + }, + ], package_id: PackageId::new(GENERATED_APP_PACKAGE_ID), }, inputs: runtime_singleton_bindings.right_values().cloned().collect(), @@ -150,6 +159,7 @@ impl ApplicationStateCallGraph { node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; let block = quote! { let #parameter_name = #block; @@ -167,6 +177,7 @@ impl ApplicationStateCallGraph { node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; blocks.insert(node_index, block); } @@ -181,6 +192,7 @@ impl ApplicationStateCallGraph { node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; blocks.insert(node_index, block); } @@ -211,12 +223,13 @@ impl ApplicationStateCallGraph { } Fragment::Statement(s) => s.to_token_stream(), }; - parse_quote! { + syn::parse2(quote! { pub fn build_application_state(#(#inputs),*) -> #output_type { #(#singleton_constructors)* #b } - } + }) + .unwrap() } }; Ok(code) @@ -252,14 +265,15 @@ impl ApplicationStateCallGraph { pub(crate) fn codegen_struct_init_block( call_graph: &StableDiGraph, callable: &Callable, - struct_fields: &BiHashMap, + struct_fields: &BiHashMap, node_index: NodeIndex, blocks: &mut HashMap, variable_generator: &mut VariableNameGenerator, + package_id2name: &BiHashMap<&PackageId, String>, ) -> Result { let dependencies = call_graph.neighbors_directed(node_index, Direction::Incoming); let mut block = quote! {}; - let mut dependency_bindings: BiHashMap = BiHashMap::new(); + let mut dependency_bindings: BiHashMap = BiHashMap::new(); for dependency_index in dependencies { let fragment = &blocks[&dependency_index]; let dependency_type = match &call_graph[dependency_index] { @@ -290,13 +304,15 @@ pub(crate) fn codegen_struct_init_block( &callable.callable_fq_path, struct_fields, &dependency_bindings, + package_id2name, )?; - let block: syn::Block = parse_quote! { + let block: syn::Block = syn::parse2(quote! { { #block #constructor_invocation } - }; + }) + .unwrap(); if block.stmts.len() == 1 { Ok(Fragment::Statement(Box::new( block.stmts.first().unwrap().to_owned(), @@ -308,21 +324,23 @@ pub(crate) fn codegen_struct_init_block( pub(crate) fn codegen_struct_init( struct_path: &ResolvedPath, - struct_fields: &BiHashMap, - variable_bindings: &BiHashMap, + struct_fields: &BiHashMap, + variable_bindings: &BiHashMap, + id2name: &BiHashMap<&PackageId, String>, ) -> Result { - let struct_path = struct_path.as_ref(); + let struct_path: syn::ExprPath = syn::parse_str(&struct_path.render_path(id2name)).unwrap(); let fields = struct_fields.iter().map(|(field_name, field_type)| { let binding = variable_bindings.get_by_left(field_type).unwrap(); quote! { #field_name: #binding } }); - Ok(parse_quote! { + Ok(syn::parse2(quote! { #struct_path { #(#fields),* } }) + .unwrap()) } /// Return the set of types that must be provided as input to build the application state. diff --git a/libs/pavex/src/web/codegen.rs b/libs/pavex/src/web/codegen.rs index 242040c6d..84776ff7c 100644 --- a/libs/pavex/src/web/codegen.rs +++ b/libs/pavex/src/web/codegen.rs @@ -8,11 +8,12 @@ use guppy::{PackageId, Version}; use indexmap::{IndexMap, IndexSet}; use proc_macro2::{Ident, TokenStream}; use quote::{format_ident, quote}; -use syn::{parse_quote, ItemFn, ItemStruct}; +use syn::{ItemFn, ItemStruct}; use crate::language::ResolvedPath; use crate::language::{Callable, ResolvedType}; -use crate::web::app::{GENERATED_APP_PACKAGE_ID, STD_PACKAGE_ID}; +use crate::rustdoc::STD_PACKAGE_ID; +use crate::web::app::GENERATED_APP_PACKAGE_ID; use crate::web::application_state_call_graph::ApplicationStateCallGraph; use crate::web::dependency_graph::DependencyGraphNode; use crate::web::handler_call_graph::{codegen, HandlerCallGraph}; @@ -82,7 +83,7 @@ pub(crate) fn codegen_app( } fn server_startup() -> ItemFn { - parse_quote! { + syn::parse2(quote! { pub async fn run( server_builder: pavex_runtime::hyper::server::Builder, application_state: ApplicationState @@ -102,7 +103,7 @@ fn server_startup() -> ItemFn { }); server_builder.serve(make_service).await.map_err(Into::into) } - } + }).unwrap() } fn define_application_state( @@ -113,20 +114,22 @@ fn define_application_state( let field_type = type_.syn_type(package_id2name); quote! { #field_name: #field_type } }); - parse_quote! { + syn::parse2(quote! { pub struct ApplicationState { #(#singleton_fields),* } - } + }) + .unwrap() } fn define_server_state() -> ItemStruct { - parse_quote! { + syn::parse2(quote! { struct ServerState { router: pavex_runtime::routing::Router, application_state: ApplicationState } - } + }) + .unwrap() } fn get_application_state_init( @@ -148,12 +151,12 @@ fn get_router_init(route_id2path: &BiHashMap) -> ItemFn { router.insert(#path, #route_id)?; }; } - parse_quote! { + syn::parse2(quote! { fn build_router() -> Result, pavex_runtime::routing::InsertError> { #router_init Ok(router) } - } + }).unwrap() } fn get_request_dispatcher( @@ -183,7 +186,7 @@ fn get_request_dispatcher( } } - parse_quote! { + syn::parse2(quote! { fn route_request(request: pavex_runtime::http::Request, server_state: std::sync::Arc) -> pavex_runtime::http::Response { let route_id = server_state.router.at(request.uri().path()).expect("Failed to match incoming request path"); match route_id.value { @@ -191,7 +194,7 @@ fn get_request_dispatcher( _ => panic!("This is a bug, no route registered for a route id"), } } - } + }).unwrap() } pub(crate) fn codegen_manifest<'a>( diff --git a/libs/pavex/src/web/codegen_utils.rs b/libs/pavex/src/web/codegen_utils.rs index 3e157e806..4b278f39a 100644 --- a/libs/pavex/src/web/codegen_utils.rs +++ b/libs/pavex/src/web/codegen_utils.rs @@ -1,11 +1,11 @@ use std::collections::HashMap; use bimap::BiHashMap; +use guppy::PackageId; use petgraph::stable_graph::{NodeIndex, StableDiGraph}; use petgraph::Direction; use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; -use syn::parse_quote; use crate::language::{Callable, ResolvedType}; use crate::web::dependency_graph::DependencyGraphNode; @@ -46,6 +46,7 @@ pub(crate) fn codegen_call_block( node_index: NodeIndex, blocks: &mut HashMap, variable_generator: &mut VariableNameGenerator, + package_id2name: &BiHashMap<&PackageId, String>, ) -> Result { let dependencies = call_graph.neighbors_directed(node_index, Direction::Incoming); let mut block = quote! {}; @@ -76,13 +77,14 @@ pub(crate) fn codegen_call_block( blocks.remove(&dependency_index); } } - let constructor_invocation = codegen_call(callable, &dependency_bindings)?; - let block: syn::Block = parse_quote! { + let constructor_invocation = codegen_call(callable, &dependency_bindings, package_id2name)?; + let block: syn::Block = syn::parse2(quote! { { #block #constructor_invocation } - }; + }) + .unwrap(); if block.stmts.len() == 1 { Ok(Fragment::Statement(Box::new( block.stmts.first().unwrap().to_owned(), @@ -95,13 +97,16 @@ pub(crate) fn codegen_call_block( pub(crate) fn codegen_call( callable: &Callable, variable_bindings: &BiHashMap, + package_id2name: &BiHashMap<&PackageId, String>, ) -> Result { - let callable_path = callable.callable_fq_path.as_ref(); + let callable_path: syn::ExprPath = + syn::parse_str(&callable.callable_fq_path.render_path(package_id2name)).unwrap(); let parameters = callable .inputs .iter() .map(|i| variable_bindings.get_by_left(i).unwrap()); - Ok(parse_quote! { + Ok(syn::parse2(quote! { #callable_path(#(#parameters),*) }) + .unwrap()) } diff --git a/libs/pavex/src/web/handler_call_graph.rs b/libs/pavex/src/web/handler_call_graph.rs index 428f043ec..3570afc12 100644 --- a/libs/pavex/src/web/handler_call_graph.rs +++ b/libs/pavex/src/web/handler_call_graph.rs @@ -9,7 +9,7 @@ use petgraph::visit::Reversed; use petgraph::Direction; use proc_macro2::TokenStream; use quote::{quote, ToTokens}; -use syn::{parse_quote, ItemFn}; +use syn::ItemFn; use pavex_builder::Lifecycle; @@ -216,6 +216,7 @@ pub(crate) fn codegen<'a>( node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; blocks.insert(node_index, block); } @@ -232,6 +233,7 @@ pub(crate) fn codegen<'a>( node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; let block = quote! { let #parameter_name = #block; @@ -250,6 +252,7 @@ pub(crate) fn codegen<'a>( node_index, &mut blocks, &mut variable_generator, + package_id2name, )?; blocks.insert(node_index, block); } @@ -280,12 +283,13 @@ pub(crate) fn codegen<'a>( } Fragment::Statement(s) => s.to_token_stream(), }; - parse_quote! { + syn::parse2(quote! { pub fn handler(#(#inputs),*) -> #output_type { #(#scoped_constructors)* #b } - } + }) + .unwrap() } }; Ok(code) From 2db929c7b4eff26cbfb79e0f2bada970418ee129 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Sun, 2 Oct 2022 16:58:51 +0100 Subject: [PATCH 17/17] Remove dead code that was commented out. --- libs/pavex/src/language/callable_path.rs | 44 ------------------------ 1 file changed, 44 deletions(-) diff --git a/libs/pavex/src/language/callable_path.rs b/libs/pavex/src/language/callable_path.rs index 101242e5a..15eaac637 100644 --- a/libs/pavex/src/language/callable_path.rs +++ b/libs/pavex/src/language/callable_path.rs @@ -107,50 +107,6 @@ impl Display for CallPathSegment { } } -// /// A path that can be used in expression position (i.e. to refer to a function or a static method). -// #[derive(Clone, Debug, Hash, Eq, PartialEq)] -// pub(crate) struct CallPath(pub ExprPath); -// -// impl AsRef for CallPath { -// fn as_ref(&self) -> &ExprPath { -// &self.0 -// } -// } -// -// impl CallPath { -// pub fn parse(callable_identifiers: &RawCallableIdentifiers) -> Result { -// let callable_path: ExprPath = -// syn::parse_str(callable_identifiers.raw_path()).map_err(|e| InvalidCallPath { -// raw_identifiers: callable_identifiers.to_owned(), -// parsing_error: e, -// })?; -// Ok(Self(callable_path)) -// } -// -// /// Return the first segment in the path. -// /// -// /// E.g. `my_crate::my_module::MyType` will return `my_crate` while `my_module::MyType` will -// /// return `my_module`. -// pub fn leading_path_segment(&self) -> &proc_macro2::Ident { -// // This unwrap never fails thanks to the validation done in `parse` -// &self.0.path.segments.first().unwrap().ident -// } -// } -// -// impl std::fmt::Display for CallPath { -// fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { -// let path = &self.0; -// let s = quote! { #path }.to_string(); -// write!( -// f, -// "{}", -// s.replace(" :: ", "::") -// .replace("< ", "<") -// .replace(" >", ">") -// ) -// } -// } - #[derive(Debug, thiserror::Error)] pub(crate) struct InvalidCallPath { pub raw_identifiers: RawCallableIdentifiers,