Get rid of autoref-specialisation

[GnomedDev]

This significantly simplifies poise internals, by removing the fancy autoderef specialization used to fallback from the poise traits, to the serenity traits, and finally to FromStr.

It is replaced by normal implementations, which does not significantly impact usability as users may define newtypes to implement the traits themselves if needed.

This PR is currently missing:

• testing to make sure I didn't break anything

[mkrasnitski]

What sort of runtime testing is required for this?

[jamesbt365]

What sort of runtime testing is required for this?

I'm guessing just testing if all the current things that parse still parse the same way, check the existing impls and cross reference i guess?

If that all works it should be fine

[mkrasnitski]

I'm not even sure what the specialization was trying to solve. Was it a workaround for trying to write a blanket implementation of impl<T: FromStr> SlashArgument for T in order to avoid overlapping impls (and likewise for PopArgument)?

[GnomedDev]

Yes.

[mkrasnitski]

Overall this looks pretty good. Having to wrap an argument in StrArg to destructure it isn't the best and it would be nice to introduce an attribute that could do the specialization automatically; that can be done later as a followup. I noticed a couple behavioral changes, so if they're intentional then I'll approve.

[mkrasnitski]

Suggested change

	<#type_ as poise::SlashArgument>::create(o)
	<#type_ as ::poise::SlashArgument>::create(o)

Minor nit

[mkrasnitski]

Suggested change

	/// The result of `<T as PopArgument>::pop_from`.
	/// The result of [`PopArgument::pop_from`].

Should keep the doc link.

[mkrasnitski]

Just noting the behavioral change here: string arguments now have leading whitespace trimmed, whereas it seems like that wasn't the case before. This is because String used the blanket PopArgument implementation for any T: ArgumentConvert, which in turn used the T: FromStr blanket impl for ArgumentConvert.

[mkrasnitski]

After re-examining this, the blanket PopArgument impl also used pop_string which is responsible for trimming leading whitespace. Therefore, I don't think there's a behavioral change here. It's worth testing though.

[mkrasnitski]

I'm guessing these extra implementing types just come out of the aforementioned blanket impls of T: ArgumentConvert and subsequently T: FromStr.

There's a behavioral change here in that we're no longer parsing mentions of users, channels, or roles. Rather we're just parsing the raw snowflakes. Since serenity::Mention has a FromStr implementation that works for all three types, do you think we should add that to the list?

[jamesbt365]

Doesn't this implementation remove the ping support for the Id types which was done via snowflake_pop_argument?

[mkrasnitski]

Which implementation are you referring to, do you mean impl FromStr for Mention? That one uses the same functions that snowflake_pop_argument used, as seen here: https://github.com/serenity-rs/serenity/blob/current/src/model/mention.rs#L135

[jamesbt365]

This, which allows use of the id types but users can specify mentions, isn't it just id parsing again now, not allowed to use mentions?

poise/src/prefix_argument/argument_trait.rs

Lines 139 to 195 in a1edcc0

	/// Macro to allow for using mentions in snowflake types
	macro_rules! snowflake_pop_argument {
	($type:ty, $parse_fn:ident, $error_type:ident) => {
	/// Error thrown when the user enters a string that cannot be parsed correctly.
	#[derive(Default, Debug)]
	pub struct $error_type {
	#[doc(hidden)]
	pub __non_exhaustive: (),
	}
	impl std::error::Error for $error_type {}
	impl std::fmt::Display for $error_type {
	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
	f.write_str(concat!(
	"Enter a valid ",
	stringify!($error_type),
	" ID or a mention."
	))
	}
	}
	#[async_trait::async_trait]
	impl<'a> PopArgumentHack<'a, $type> for &PhantomData<$type> {
	async fn pop_from(
	self,
	args: &'a str,
	attachment_index: usize,
	ctx: &serenity::Context,
	msg: &serenity::Message,
	) -> Result<
	(&'a str, usize, $type),
	(Box<dyn std::error::Error + Send + Sync>, Option<String>),
	> {
	let (args, string) =
	pop_string(args).map_err(|_| (TooFewArguments::default().into(), None))?;
	if let Some(parsed_id) = string
	.parse()
	.ok()
	.or_else(|| serenity::utils::$parse_fn(&string))
	{
	Ok((args.trim_start(), attachment_index, parsed_id))
	} else {
	Err(($error_type::default().into(), Some(string)))
	}
	}
	}
	};
	}
	snowflake_pop_argument!(serenity::UserId, parse_user_mention, InvalidUserId);
	snowflake_pop_argument!(
	serenity::GenericChannelId,
	parse_channel_mention,
	InvalidChannelId
	);
	snowflake_pop_argument!(serenity::RoleId, parse_role_mention, InvalidRoleId);

[mkrasnitski]

Yes, that's what I was referring to. Since we no longer pass parse_{user,channel,role}_mention to the macro, then IDs are just parsed as snowflakes, not as mentions.

[jamesbt365]

Tested this out a bit and it looks great other than the review comment, haven't had any other issues.

[mkrasnitski]

Tested this out a bit and it looks great other than the review comment, haven't had any other issues.

Can you test to verify the behavior change around string arguments, where whitespace is now trimmed?

[arqunis]

Superseded by #357