FRAME: pallet tasks: code seems to handle manual definition of tasks, but it actually fail to compile.

[gui1117]

TLDR:

there is some code to handle manual definition of tasks. But the code is wrong AFAICT, not tested, and not documented. Should we fix it or drop it?

Details:

In pallet macro, there is some specific code to retrieve an item like the following, without any explicit attribute:

impl frame_support::traits::Task for Task { .. }
pub enum Task { .. }

The code is here:

polkadot-sdk/substrate/frame/support/procedural/src/pallet/parse/mod.rs

Lines 296 to 352 in 21b3a46

	/// Tries to locate task enum based on the tasks impl target if attribute is not specified
	/// but impl is present. If one is found, `task_enum` is set appropriately.
	fn resolve_manual_task_enum(
	tasks: &Option<tasks::TasksDef>,
	task_enum: &mut Option<tasks::TaskEnumDef>,
	items: &mut Vec<syn::Item>,
	) -> syn::Result<()> {
	let (None, Some(tasks)) = (&task_enum, &tasks) else { return Ok(()) };
	let syn::Type::Path(type_path) = &*tasks.item_impl.self_ty else { return Ok(()) };
	let type_path = type_path.path.segments.iter().collect::<Vec<_>>();
	let (Some(seg), None) = (type_path.get(0), type_path.get(1)) else { return Ok(()) };
	let mut result = None;
	for item in items {
	let syn::Item::Enum(item_enum) = item else { continue };
	if item_enum.ident == seg.ident {
	result = Some(syn::parse2::<tasks::TaskEnumDef>(item_enum.to_token_stream())?);
	// replace item with a no-op because it will be handled by the expansion of
	// `task_enum`. We use a no-op instead of simply removing it from the vec
	// so that any indices collected by `Def::try_from` remain accurate
	*item = syn::Item::Verbatim(quote::quote!());
	break;
	}
	}
	*task_enum = result;
	Ok(())
	}
	/// Tries to locate a manual tasks impl (an impl implementing a trait whose last path segment is
	/// `Task`) in the event that one has not been found already via the attribute macro
	pub fn resolve_manual_tasks_impl(
	tasks: &mut Option<tasks::TasksDef>,
	task_enum: &Option<tasks::TaskEnumDef>,
	items: &Vec<syn::Item>,
	) -> syn::Result<()> {
	let None = tasks else { return Ok(()) };
	let mut result = None;
	for item in items {
	let syn::Item::Impl(item_impl) = item else { continue };
	let Some((_, path, _)) = &item_impl.trait_ else { continue };
	let Some(trait_last_seg) = path.segments.last() else { continue };
	let syn::Type::Path(target_path) = &*item_impl.self_ty else { continue };
	let target_path = target_path.path.segments.iter().collect::<Vec<_>>();
	let (Some(target_ident), None) = (target_path.get(0), target_path.get(1)) else {
	continue;
	};
	let matches_task_enum = match task_enum {
	Some(task_enum) => task_enum.item_enum.ident == target_ident.ident,
	None => true,
	};
	if trait_last_seg.ident == "Task" && matches_task_enum {
	result = Some(syn::parse2::<tasks::TasksDef>(item_impl.to_token_stream())?);
	break;
	}
	}
	*tasks = result;
	Ok(())
	}

But writing such code fails to compile, the macro panics.
Because the item is removed from input, then expanded here:

polkadot-sdk/substrate/frame/support/procedural/src/pallet/expand/tasks.rs

Line 118 in 21b3a46

tokens.extend(item_enum.to_token_stream());

Then parsed again but as 2 items:

polkadot-sdk/substrate/frame/support/procedural/src/pallet/expand/tasks.rs

Line 241 in 21b3a46

let ExpandedTaskEnum { item_enum, debug_impl } = parse_quote!(#task_enum);

so it fails to compile with End of input, expect token impl.

This code is not tested, it is also not documented. Should we simply drop it or fix it?

If we want to fix it I suggest a new attribute which just check that it declares an enum with the name Task and sensible generics:

#[pallet::task_manual]
pub enum Task<T> { ...}

[kianenigma]

cc @gupnik

[gupnik]

This code is not tested, it is also not documented. Should we simply drop it or fix it?

Yes, @gui1117 we can go ahead and drop it. This was added by Sam, as the requirements were not completely clear at the time. Later, when the macros were implemented, it was left under the assumption that the manual specification still worked. But, I don't think its worthwhile spending the time to fix it.