Allow implementations of AuthorizedResource::load_roles to use async fn syntax.

[mergeconflict]

Motivation: I was reviewing some code that added an impl AuthorizedResource (#9679) and was curious about the 'fut lifetime and use of futures::BoxFuture. I learned that this is the pre-Rust 1.75 way of writing(async fn or -> impl Future in a trait).

This change enables native async fn syntax for AuthorizedResource::load_roles by replacing dynamic dispatch with associated types.

Key changes:

1. Split ApiResource into two traits:
   • ApiResource - object-safe base trait with all existing methods except parent().
   • ApiResourceWithParent - extends ApiResource, adds associated type Parent and parent() method. We use the NoParent uninhabited type for root resources (e.g. Fleet).
2. Changed AuthorizedResource::load_roles signature:
   • From: fn load_roles<'fut>(...) -> BoxFuture<'fut, Result<(), Error>>
   • To: fn load_roles(...) -> impl Future<Output = Result<(), Error>>
   • Implementations can now use async fn syntax directly instead of .boxed(), and no longer need an explicit 'fut lifetime.
3. Updated authz_resource! macro to generate both ApiResource and ApiResourceWithParent impls.
4. Minor cleanup in resource_builder.rs - removed unused import and redundant trait bound.

[david-crespo]

This makes sense to me. Couple of things I wonder while reviewing:

1. Is there any functional difference? (seems like no)
2. Why wasn't it done this way before?

When I was asking LLMs these questions, they suggested based on the diff alone that this change might be a problem if we use dyn AuthorizedResource anywhere (which we do). Once I made it clear the code compiles fine, the explanation for why it's not a problem was interesting:

edit: not true that RPITIT lets this work, see below

The key observation: the code only calls polar_class() on &dyn AuthorizedResource, not
load_roles().

With modern Rust (1.75+), RPITIT (return position impl trait in traits) doesn't make the
entire trait non-object-safe. Instead, only the specific method with -> impl Trait becomes
non-dispatchable through a trait object. You can still create dyn AuthorizedResource and
call other object-safe methods like polar_class() and on_unauthorized() through it - you
just can't call load_roles() through the trait object.

This is similar to having where Self: Sized on a method - it excludes that method from the
vtable but allows the trait to remain object-safe for other methods.

So the GPT review is overly cautious here. The change is fine because:

1. dyn AuthorizedResource is still usable
2. The only place using dyn AuthorizedResource (coverage.rs) only calls polar_class(),
   not load_roles()
3. The parent() method that previously returned &dyn AuthorizedResource has been
   refactored into ApiResourceWithParent with an associated type instead, so it doesn't need
   object-safety there

[hawkw]

It seems there are presently a bunch of places where the ApiResource trait is no longer being imported: https://github.com/oxidecomputer/omicron/actions/runs/21191186060/job/60957730295?pr=9693

[hawkw]

Regarding @david-crespo's question,

• Why wasn't it done this way before?

My guess is that it wasn't done this way before because return position impl Trait in traits was not stable until fairly recently.

[hawkw]

@david-crespo, regarding:

With modern Rust (1.75+), RPITIT (return position impl trait in traits) doesn't make the
entire trait non-object-safe. Instead, only the specific method with -> impl Trait becomes
non-dispatchable through a trait object. You can still create dyn AuthorizedResource and
call other object-safe methods like polar_class() and on_unauthorized() through it - you
just can't call load_roles() through the trait object.

did whichever LLM claimed this actually provide any reference for that claim? I don't believe this is correct at all. I haven't been able to find any source suggesting that this is the current semantics of RPITIT --- only blog posts discussing how a feature like that could be added in the future. I went and checked in the Rust playground and, indeed, this does not compile:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=2ad053a76b69281b2c21cd6ff88ead84

Any return-position impl Trait in a trait renders that whole trait not object safe, and no methods on that trait can be called through a dyn Trait, even if the method does not return an impl Trait. So, the LLM response is incorrect.

[hawkw]

I don't actually think we do use &dyn AuthorizedResource, though. It looks like we've been working around the object safety issue using a separate trait, DynAuthorizedResource, which is object-safe, as it always returns a boxed Future. We implement DynAuthorizedResource for types that implement AuthorizedResource using a macro:

omicron/nexus/db-queries/src/policy_test/resource_builder.rs

Lines 177 to 245 in 3d94f95

	/// Dynamically-dispatched version of `AuthorizedResource`
	///
	/// This is needed because calling [`OpContext::authorize()`] requires knowing
	/// at compile time exactly which resource you're authorizing. But we want to
	/// put many different resource types into a collection and do authz checks on
	/// all of them. (We could also change `authorize()` to be dynamically-
	/// dispatched. This would be a much more sprawling change. And it's not clear
	/// that our use case has much application outside of a test like this.)
	pub trait DynAuthorizedResource: std::fmt::Debug + Send + Sync {
	fn do_authorize<'a, 'b>(
	&'a self,
	opctx: &'b OpContext,
	action: authz::Action,
	) -> BoxFuture<'a, Result<(), Error>>
	where
	'b: 'a;
	fn resource_name(&self) -> String;
	}
	macro_rules! impl_dyn_authorized_resource_for_global {
	($t:ty) => {
	impl DynAuthorizedResource for $t {
	fn resource_name(&self) -> String {
	String::from(stringify!($t))
	}
	fn do_authorize<'a, 'b>(
	&'a self,
	opctx: &'b OpContext,
	action: authz::Action,
	) -> BoxFuture<'a, Result<(), Error>>
	where
	'b: 'a,
	{
	opctx.authorize(action, self).boxed()
	}
	}
	};
	}
	macro_rules! impl_dyn_authorized_resource_for_resource {
	($t:ty) => {
	impl DynAuthorizedResource for $t {
	fn resource_name(&self) -> String {
	let my_ident = match self.lookup_type() {
	LookupType::ByName(name) => format!("{:?}", name),
	LookupType::ById(id) => format!("id {:?}", id.to_string()),
	LookupType::ByCompositeId(id) => format!("id {:?}", id),
	LookupType::ByOther(_) => {
	unimplemented!()
	}
	};
	format!("{:?} {}", self.resource_type(), my_ident)
	}
	fn do_authorize<'a, 'b>(
	&'a self,
	opctx: &'b OpContext,
	action: authz::Action,
	) -> BoxFuture<'a, Result<(), Error>>
	where
	'b: 'a,
	{
	opctx.authorize(action, self).boxed()
	}
	}
	};
	}

This was the case prior to @mergeconflict's change, too.

[mergeconflict]

1. Is there any functional difference? (seems like no)

I sure hope not.

2. Why wasn't it done this way before?

What Eliza said. I updated the PR description with motivation/background.

When I was asking LLMs these questions, they suggested based on the diff alone that this change might be a problem if we use dyn AuthorizedResource anywhere (which we do). Once I made it clear the code compiles fine, the explanation for why it's not a problem was interesting:

The key observation: the code only calls polar_class() on &dyn AuthorizedResource, not
load_roles().
With modern Rust (1.75+), RPITIT (return position impl trait in traits) doesn't make the
entire trait non-object-safe. Instead, only the specific method with -> impl Trait becomes
non-dispatchable through a trait object. You can still create dyn AuthorizedResource and
call other object-safe methods like polar_class() and on_unauthorized() through it - you
just can't call load_roles() through the trait object.
This is similar to having where Self: Sized on a method - it excludes that method from the
vtable but allows the trait to remain object-safe for other methods.
So the GPT review is overly cautious here. The change is fine because:

1. dyn AuthorizedResource is still usable
2. The only place using dyn AuthorizedResource (coverage.rs) only calls polar_class(),
   not load_roles()
3. The parent() method that previously returned &dyn AuthorizedResource has been
   refactored into ApiResourceWithParent with an associated type instead, so it doesn't need
   object-safety there

Yeah I messed around with this a bit to try and understand what we could get away with... To be honest I'm still not sure I get it. I'm also not sure if it's worth merging this, or if this is the beginning of a much larger yak shave (migrating other uses of BoxFuture to async fn) that we don't actually want to do. I'd love opinions on this.

[david-crespo]

It did not provide a source, and it did sound like an elaborate claim. Sorry for the noise! We do have several mentions of dyn AuthorizedResource in the codebase, so I still don't understand how those compile if what you're saying is true.

Turns out the answer is cargo check -p nexus-db-queries --all-targets does not compile. So asking Claude to explain why the code is fine was inevitably going to be a disaster.

[hawkw]

The LLM response is ... sort of directionally correct, but it's also wrong. If one explicitly places where Self: Sized on trait methods returning an impl Trait, the trait does become object-safe, allowing other methods that don't have the where Self: Sized bound to be callable through a trait object, as in this playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=864f89fdae5e8f2b3a997a538e6e6fa1

The thing the LLM got wrong is the suggestion that any use of RPITIT causes this to happen automagically.

[david-crespo]

Well you're going to love what I got from Codex when I asked if Claude's answer was right, and then promptly ignored because I thought the code did in fact compile.

[david-crespo]

I see now the dyn AuthorizedResource is gone, so now it does compile.