From 0d40568e5af0b48566be65c88bfd934528eb6de6 Mon Sep 17 00:00:00 2001 From: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> Date: Tue, 22 Oct 2024 18:03:33 +0200 Subject: [PATCH] fix: Pavex always uses a public path to refer to public items, even if they are defined in a private module --- libs/pavexc/src/compiler/resolvers.rs | 47 +++++- libs/ui_tests/Cargo.toml | 2 + .../always_use_public_path/Cargo.toml | 17 ++ .../always_use_public_path/diagnostics.dot | 41 +++++ .../expectations/app.rs | 153 ++++++++++++++++++ .../expectations/diagnostics.dot | 41 +++++ .../always_use_public_path/src/lib.rs | 31 ++++ .../always_use_public_path/src/main.rs | 24 +++ .../always_use_public_path/test_config.toml | 4 + 9 files changed, 357 insertions(+), 3 deletions(-) create mode 100644 libs/ui_tests/reflection/always_use_public_path/Cargo.toml create mode 100644 libs/ui_tests/reflection/always_use_public_path/diagnostics.dot create mode 100644 libs/ui_tests/reflection/always_use_public_path/expectations/app.rs create mode 100644 libs/ui_tests/reflection/always_use_public_path/expectations/diagnostics.dot create mode 100644 libs/ui_tests/reflection/always_use_public_path/src/lib.rs create mode 100644 libs/ui_tests/reflection/always_use_public_path/src/main.rs create mode 100644 libs/ui_tests/reflection/always_use_public_path/test_config.toml diff --git a/libs/pavexc/src/compiler/resolvers.rs b/libs/pavexc/src/compiler/resolvers.rs index 0aed1d339..501001571 100644 --- a/libs/pavexc/src/compiler/resolvers.rs +++ b/libs/pavexc/src/compiler/resolvers.rs @@ -12,8 +12,8 @@ use rustdoc_types::{GenericArg, GenericArgs, GenericParamDefKind, ItemEnum, Type use crate::language::{ Callable, Generic, GenericArgument, GenericLifetimeParameter, InvocationStyle, PathType, - ResolvedPath, ResolvedPathGenericArgument, ResolvedPathLifetime, ResolvedPathType, - ResolvedType, Slice, Tuple, TypeReference, UnknownPath, + ResolvedPath, ResolvedPathGenericArgument, ResolvedPathLifetime, ResolvedPathSegment, + ResolvedPathType, ResolvedType, Slice, Tuple, TypeReference, UnknownPath, }; use crate::rustdoc::{CannotGetCrateData, RustdocKindExt}; use crate::rustdoc::{CrateCollection, ResolvedItem}; @@ -421,11 +421,52 @@ pub(crate) fn resolve_callable( } } }; + + // We need to make sure that we always refer to user-registered types using a public path, even though they + // might have been registered using a private path (e.g. `self::...` where `self` is a private module). + // For this reason, we fall back to the canonical path—i.e. the shortest public path for the given item. + // TODO: We should do the same for qualified self, if it's populated. + let canonical_path = + match krate_collection.get_canonical_path_by_global_type_id(&callable_type.item.item_id) { + Ok(p) => { + let segments = p.into_iter().skip(1).map(|s| ResolvedPathSegment { + ident: s.into(), + generic_arguments: vec![], + }); + // We use the first segment from the original callable path, since that's been resolved to the defining crate + // and we want to keep that information. + let mut segments: Vec<_> = std::iter::once(callable_path.segments[0].clone()) + .chain(segments) + .collect(); + // The canonical path doesn't include the (populated or omitted) generic arguments from the user-provided callable path, + // so we need to add them back in. + segments.last_mut().unwrap().generic_arguments = callable_path + .segments + .last() + .unwrap() + .generic_arguments + .clone(); + ResolvedPath { + segments, + qualified_self: callable_path.qualified_self.clone(), + package_id: callable_type.item.item_id.package_id.clone(), + } + } + Err(e) => { + tracing::warn!( + package_id = ?callable_type.item.item_id.package_id, + item_id = ?callable_type.item.item_id.rustdoc_item_id, + "Failed to find canonical path: {e:?}" + ); + callable_path.to_owned() + } + }; + let callable = Callable { is_async: header.is_async, takes_self_as_ref, output: output_type_path, - path: callable_path.to_owned(), + path: canonical_path, inputs: resolved_parameter_types, invocation_style, source_coordinates: Some(callable_type.item.item_id), diff --git a/libs/ui_tests/Cargo.toml b/libs/ui_tests/Cargo.toml index 39bd2f39e..fd23807cf 100644 --- a/libs/ui_tests/Cargo.toml +++ b/libs/ui_tests/Cargo.toml @@ -239,6 +239,8 @@ members = [ "path_parameters/path_parameters_unsupported_types/generated_app", "reflection/a_fix_is_suggested_for_function_invocations", "reflection/a_fix_is_suggested_for_function_invocations/generated_app", + "reflection/always_use_public_path", + "reflection/always_use_public_path/generated_app", "reflection/ambiguous_dependency_version_is_handled", "reflection/ambiguous_dependency_version_is_handled/generated_app", "reflection/arc_singletons_are_supported", diff --git a/libs/ui_tests/reflection/always_use_public_path/Cargo.toml b/libs/ui_tests/reflection/always_use_public_path/Cargo.toml new file mode 100644 index 000000000..278344f29 --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "app_73b868ae" +version = "0.1.0" +edition = "2021" + +[lints.rust.unexpected_cfgs] +level = "allow" +check-cfg = ["cfg(pavex_ide_hint)"] + +[dependencies.pavex] +workspace = true + +[dependencies.pavex_cli_client] +workspace = true + +[dependencies.workspace_hack] +workspace = true diff --git a/libs/ui_tests/reflection/always_use_public_path/diagnostics.dot b/libs/ui_tests/reflection/always_use_public_path/diagnostics.dot new file mode 100644 index 000000000..27b72a6fc --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/diagnostics.dot @@ -0,0 +1,41 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0() -> crate::route_0::Next0"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 3 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app_73b868ae::handler(app_73b868ae::A) -> pavex::response::Response"] + 1 [ label = "app_73b868ae::a() -> app_73b868ae::A"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 0 -> 2 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 5 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 4 [ ] + 5 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} diff --git a/libs/ui_tests/reflection/always_use_public_path/expectations/app.rs b/libs/ui_tests/reflection/always_use_public_path/expectations/app.rs new file mode 100644 index 000000000..e69a76534 --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/expectations/app.rs @@ -0,0 +1,153 @@ +//! Do NOT edit this code. +//! It was automatically generated by Pavex. +//! All manual edits will be lost next time the code is generated. +extern crate alloc; +struct ServerState { + router: pavex_matchit::Router, + #[allow(dead_code)] + application_state: ApplicationState, +} +pub struct ApplicationState {} +pub async fn build_application_state() -> crate::ApplicationState { + crate::ApplicationState {} +} +pub fn run( + server_builder: pavex::server::Server, + application_state: ApplicationState, +) -> pavex::server::ServerHandle { + let server_state = std::sync::Arc::new(ServerState { + router: build_router(), + application_state, + }); + server_builder.serve(route_request, server_state) +} +fn build_router() -> pavex_matchit::Router { + let mut router = pavex_matchit::Router::new(); + router.insert("/", 0u32).unwrap(); + router +} +async fn route_request( + request: http::Request, + _connection_info: Option, + server_state: std::sync::Arc, +) -> pavex::response::Response { + let (request_head, request_body) = request.into_parts(); + #[allow(unused)] + let request_body = pavex::request::body::RawIncomingBody::from(request_body); + let request_head: pavex::request::RequestHead = request_head.into(); + let matched_route = match server_state.router.at(&request_head.target.path()) { + Ok(m) => m, + Err(_) => { + let allowed_methods: pavex::router::AllowedMethods = pavex::router::MethodAllowList::from_iter( + vec![], + ) + .into(); + return route_1::entrypoint(&allowed_methods).await; + } + }; + let route_id = matched_route.value; + #[allow(unused)] + let url_params: pavex::request::path::RawPathParams<'_, '_> = matched_route + .params + .into(); + match route_id { + 0u32 => { + match &request_head.method { + &pavex::http::Method::GET => route_0::entrypoint().await, + _ => { + let allowed_methods: pavex::router::AllowedMethods = pavex::router::MethodAllowList::from_iter([ + pavex::http::Method::GET, + ]) + .into(); + route_1::entrypoint(&allowed_methods).await + } + } + } + i => unreachable!("Unknown route id: {}", i), + } +} +pub mod route_0 { + pub async fn entrypoint() -> pavex::response::Response { + let response = wrapping_0().await; + response + } + async fn stage_1() -> pavex::response::Response { + let response = handler().await; + response + } + async fn wrapping_0() -> pavex::response::Response { + let v0 = crate::route_0::Next0 { + next: stage_1, + }; + let v1 = pavex::middleware::Next::new(v0); + let v2 = pavex::middleware::wrap_noop(v1).await; + ::into_response(v2) + } + async fn handler() -> pavex::response::Response { + let v0 = app::a(); + let v1 = app::handler(v0); + ::into_response(v1) + } + struct Next0 + where + T: std::future::Future, + { + next: fn() -> T, + } + impl std::future::IntoFuture for Next0 + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)() + } + } +} +pub mod route_1 { + pub async fn entrypoint<'a>( + s_0: &'a pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let response = wrapping_0(s_0).await; + response + } + async fn stage_1<'a>( + s_0: &'a pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let response = handler(s_0).await; + response + } + async fn wrapping_0( + v0: &pavex::router::AllowedMethods, + ) -> pavex::response::Response { + let v1 = crate::route_1::Next0 { + s_0: v0, + next: stage_1, + }; + let v2 = pavex::middleware::Next::new(v1); + let v3 = pavex::middleware::wrap_noop(v2).await; + ::into_response(v3) + } + async fn handler(v0: &pavex::router::AllowedMethods) -> pavex::response::Response { + let v1 = pavex::router::default_fallback(v0).await; + ::into_response(v1) + } + struct Next0<'a, T> + where + T: std::future::Future, + { + s_0: &'a pavex::router::AllowedMethods, + next: fn(&'a pavex::router::AllowedMethods) -> T, + } + impl<'a, T> std::future::IntoFuture for Next0<'a, T> + where + T: std::future::Future, + { + type Output = pavex::response::Response; + type IntoFuture = T; + fn into_future(self) -> Self::IntoFuture { + (self.next)(self.s_0) + } + } +} \ No newline at end of file diff --git a/libs/ui_tests/reflection/always_use_public_path/expectations/diagnostics.dot b/libs/ui_tests/reflection/always_use_public_path/expectations/diagnostics.dot new file mode 100644 index 000000000..cedec4a16 --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/expectations/diagnostics.dot @@ -0,0 +1,41 @@ +digraph "GET / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next"] + 2 [ label = "crate::route_0::Next0() -> crate::route_0::Next0"] + 3 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 3 [ ] +} + +digraph "GET / - 1" { + 0 [ label = "app::handler(app::A) -> pavex::response::Response"] + 1 [ label = "app::a() -> app::A"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 1 -> 0 [ ] + 0 -> 2 [ ] +} + +digraph "* / - 0" { + 0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next>) -> pavex::response::Response"] + 1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next>"] + 2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"] + 4 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 5 [ label = "&pavex::router::AllowedMethods"] + 1 -> 0 [ ] + 2 -> 1 [ ] + 0 -> 4 [ ] + 5 -> 2 [ ] +} + +digraph "* / - 1" { + 0 [ label = "pavex::router::default_fallback(&pavex::router::AllowedMethods) -> pavex::response::Response"] + 2 [ label = "::into_response(pavex::response::Response) -> pavex::response::Response"] + 3 [ label = "&pavex::router::AllowedMethods"] + 0 -> 2 [ ] + 3 -> 0 [ ] +} + +digraph app_state { + 0 [ label = "crate::ApplicationState() -> crate::ApplicationState"] +} \ No newline at end of file diff --git a/libs/ui_tests/reflection/always_use_public_path/src/lib.rs b/libs/ui_tests/reflection/always_use_public_path/src/lib.rs new file mode 100644 index 000000000..bfea85344 --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/src/lib.rs @@ -0,0 +1,31 @@ +use pavex::blueprint::{router::GET, Blueprint}; +use pavex::f; +use pavex::response::Response; + +pub use private::*; + +pub fn handler(_a: A) -> Response { + todo!() +} + +pub fn blueprint() -> Blueprint { + let mut bp = Blueprint::new(); + register(&mut bp); + bp.route(GET, "/", f!(self::handler)); + bp +} + +mod private { + use pavex::blueprint::Blueprint; + use pavex::f; + + pub struct A; + + pub fn register(bp: &mut Blueprint) { + bp.request_scoped(f!(self::a)); + } + + pub fn a() -> A { + todo!() + } +} diff --git a/libs/ui_tests/reflection/always_use_public_path/src/main.rs b/libs/ui_tests/reflection/always_use_public_path/src/main.rs new file mode 100644 index 000000000..b26e8e31b --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/src/main.rs @@ -0,0 +1,24 @@ +//! This code is generated by `pavex_test_runner`, +//! Do NOT modify it manually. +use app_73b868ae::blueprint; +use pavex_cli_client::{Client, config::Color}; +use pavex_cli_client::commands::generate::GenerateError; + +fn main() -> Result<(), Box> { + let ui_test_dir: std::path::PathBuf = std::env::var("UI_TEST_DIR").unwrap().into(); + let outcome = Client::new() + .color(Color::Always) + .pavex_cli_path(std::env::var("PAVEX_TEST_CLI_PATH").unwrap().into()) + .generate(blueprint(), ui_test_dir.join("generated_app")) + .diagnostics_path("diagnostics.dot".into()) + .execute(); + match outcome { + Ok(_) => {}, + Err(GenerateError::NonZeroExitCode(_)) => { std::process::exit(1); } + Err(e) => { + eprintln!("Failed to invoke `pavex generate`.\n{:?}", e); + std::process::exit(1); + } + } + Ok(()) +} diff --git a/libs/ui_tests/reflection/always_use_public_path/test_config.toml b/libs/ui_tests/reflection/always_use_public_path/test_config.toml new file mode 100644 index 000000000..a9b150f30 --- /dev/null +++ b/libs/ui_tests/reflection/always_use_public_path/test_config.toml @@ -0,0 +1,4 @@ +description = "When an item is available via multiple paths, some private and some public, Pavex will always use one of the public ones" + +[expectations] +codegen = "pass"