Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Pavex always uses a public path to refer to public items, even if they are defined in a private module #351

Merged
merged 1 commit into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions libs/pavexc/src/compiler/resolvers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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),
Expand Down
2 changes: 2 additions & 0 deletions libs/ui_tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
17 changes: 17 additions & 0 deletions libs/ui_tests/reflection/always_use_public_path/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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
41 changes: 41 additions & 0 deletions libs/ui_tests/reflection/always_use_public_path/diagnostics.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
digraph "GET / - 0" {
0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next<crate::route_0::Next0>) -> pavex::response::Response"]
1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next<crate::route_0::Next0>"]
2 [ label = "crate::route_0::Next0() -> crate::route_0::Next0"]
3 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::into_response(pavex::response::Response) -> pavex::response::Response"]
1 -> 0 [ ]
2 -> 1 [ ]
0 -> 3 [ ]
}

digraph "GET / - 1" {
0 [ label = "app_73b868ae::handler(app_73b868ae::A) -> pavex::response::Response"]
1 [ label = "app_73b868ae::a() -> app_73b868ae::A"]
2 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::into_response(pavex::response::Response) -> pavex::response::Response"]
1 -> 0 [ ]
0 -> 2 [ ]
}

digraph "* / - 0" {
0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next<crate::route_1::Next0<'a>>) -> pavex::response::Response"]
1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next<crate::route_1::Next0<'a>>"]
2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"]
4 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::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 = "<pavex::response::Response as pavex::response::IntoResponse>::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"]
}
153 changes: 153 additions & 0 deletions libs/ui_tests/reflection/always_use_public_path/expectations/app.rs
Original file line number Diff line number Diff line change
@@ -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<u32>,
#[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<u32> {
let mut router = pavex_matchit::Router::new();
router.insert("/", 0u32).unwrap();
router
}
async fn route_request(
request: http::Request<hyper::body::Incoming>,
_connection_info: Option<pavex::connection::ConnectionInfo>,
server_state: std::sync::Arc<ServerState>,
) -> 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;
<pavex::response::Response as pavex::response::IntoResponse>::into_response(v2)
}
async fn handler() -> pavex::response::Response {
let v0 = app::a();
let v1 = app::handler(v0);
<pavex::response::Response as pavex::response::IntoResponse>::into_response(v1)
}
struct Next0<T>
where
T: std::future::Future<Output = pavex::response::Response>,
{
next: fn() -> T,
}
impl<T> std::future::IntoFuture for Next0<T>
where
T: std::future::Future<Output = pavex::response::Response>,
{
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;
<pavex::response::Response as pavex::response::IntoResponse>::into_response(v3)
}
async fn handler(v0: &pavex::router::AllowedMethods) -> pavex::response::Response {
let v1 = pavex::router::default_fallback(v0).await;
<pavex::response::Response as pavex::response::IntoResponse>::into_response(v1)
}
struct Next0<'a, T>
where
T: std::future::Future<Output = pavex::response::Response>,
{
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<Output = pavex::response::Response>,
{
type Output = pavex::response::Response;
type IntoFuture = T;
fn into_future(self) -> Self::IntoFuture {
(self.next)(self.s_0)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
digraph "GET / - 0" {
0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next<crate::route_0::Next0>) -> pavex::response::Response"]
1 [ label = "pavex::middleware::Next::new(crate::route_0::Next0) -> pavex::middleware::Next<crate::route_0::Next0>"]
2 [ label = "crate::route_0::Next0() -> crate::route_0::Next0"]
3 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::into_response(pavex::response::Response) -> pavex::response::Response"]
1 -> 0 [ ]
2 -> 1 [ ]
0 -> 3 [ ]
}

digraph "GET / - 1" {
0 [ label = "app::handler(app::A) -> pavex::response::Response"]
1 [ label = "app::a() -> app::A"]
2 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::into_response(pavex::response::Response) -> pavex::response::Response"]
1 -> 0 [ ]
0 -> 2 [ ]
}

digraph "* / - 0" {
0 [ label = "pavex::middleware::wrap_noop(pavex::middleware::Next<crate::route_1::Next0<'a>>) -> pavex::response::Response"]
1 [ label = "pavex::middleware::Next::new(crate::route_1::Next0<'a>) -> pavex::middleware::Next<crate::route_1::Next0<'a>>"]
2 [ label = "crate::route_1::Next0(&'a pavex::router::AllowedMethods) -> crate::route_1::Next0<'a>"]
4 [ label = "<pavex::response::Response as pavex::response::IntoResponse>::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 = "<pavex::response::Response as pavex::response::IntoResponse>::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"]
}
31 changes: 31 additions & 0 deletions libs/ui_tests/reflection/always_use_public_path/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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!()
}
}
24 changes: 24 additions & 0 deletions libs/ui_tests/reflection/always_use_public_path/src/main.rs
Original file line number Diff line number Diff line change
@@ -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<dyn std::error::Error>> {
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(())
}
Original file line number Diff line number Diff line change
@@ -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"