Clone less often

[notpeter]

This is my first time wading into the progenitor code. Please be kind!

Background

Previously, progenitor generated code that would be conservative and always .clone():

progenitor/progenitor-impl/src/cli.rs

Lines 430 to 438 in 1a5b88f

	let consumer = quote! {
	if let Some(value) =
	matches.get_one::<#arg_type_name>(#arg_name)
	{
	// clone here in case the arg type doesn't impl
	// From<&T>
	request = request.#arg_fn(value.clone());
	}
	};

If you ran generated code through Clippy you'd see lots of these:

Workaround

Inject a #[allow(clippy::redundant_clone)] at the top of each generated file.
It worked in: https://github.com/oxidecomputer/four-star/pull/322
But that's a little ugly.

Solution

Enumerate all the cases where we don't have to clone.

TypeDetails	Generates
TypeDetails::Unit (e.g. ())	*value
TypeDetails::Builtin(bool)	*value
TypeDetails::Builtin({i8,i16,i32,i64,u8,u16,u32,u64})	*value
TypeDetails::Builtin(::std::num::NonZeroU{8,16,32,64})	*value
TypeDetails::Builtin(...) for f32, f64	*value
TypeDetails::Builtin(::uuid::Uuid)	*value
TypeDetails::Builtin(::chrono::naive::NaiveDate)	*value
TypeDetails::Builtin(::chrono::DateTime<::chrono::offset::Utc>)	*value
TypeDetails::Builtin(::std::net::IpAddr)	*value
TypeDetails::Builtin(::std::net::Ipv4Addr)	*value
TypeDetails::Builtin(::std::net::Ipv6Addr)	*value
TypeDetails::Enum(...) where all variants are simple	*value
TypeDetails::String	value.as_str()
TypeDetails::String for TryInto<Option<String>>	value.clone()
TypeDetails::Enum(...) for data-carrying enums	value.clone()
TypeDetails::Builtin(::serde_json::Value)	value.clone()
TypeDetails::Builtin(...) for any Builtin not listed above	value.clone()
TypeDetails::Struct(...)	value.clone()
TypeDetails::Newtype(...)	value.clone()
TypeDetails::Option(_) for Option<T>	value.clone()
TypeDetails::Vec(_) for Vec<T>	value.clone()
TypeDetails::Map(_, _) for BTreeMap<K, V> or map equivalent	value.clone()
TypeDetails::Set(_) for set types	value.clone()
TypeDetails::Box(_) for Box<T>	value.clone()
TypeDetails::Tuple(_) for tuple types, e.g. (A, B)	value.clone()
TypeDetails::Array(_, _) for array types, e.g. [T; N]	value.clone()
any other TypeDetails variant	value.clone()

Impact

If progenitor generates code which passes clippy out-of-the-box, it's easier for dependent projects to run clippy without having to create exemptions for generated code.

I tried this on four-star, and it went from 17 clippy warnings on the generated code to 0.
Here's the diff when run using this branch.

This should have nominal impact on generation times.
I would not expect any runtime performance impact (positive or negative).
I'm pretty sure the Rust compiler will yield similar output either way.

Thoughts?

Tests

Probably worth setting up test(s) with all the types above plus a custom TypeDetails variant and assert they are all handled as expected.

Discussion

There is some complexity here and I'm not sure the juice is worth the squeeze. A naive alternative would just emit #[allow(clippy::redundant_clone)] for these files or each of these functions (whether it has redundant clones or not).

[ahl]

This is my first time wading into the progenitor code. Please be kind!

Empathy is an Oxide value!

Thoughts?

Initial thoughts: progenitor has not had as a goal generation of clippy-clean code. There's a good reason for that! Clippy is intended for code written by humans. Or written by LLMs and read by humans? For example, we would prefer a human (or LLM) write if foo_list.is_empty() ... rather than if foo_list.len() == 0, but they have the same meaning, and procude the same eventual binary (I trust). Modifying code generation to handle that case is--I think--the wrong trade-off: it adds complexity to the code we maintain while adding cleanliness to the code we don't.

My thought in this case is--I think?--the same as the strawman above. Also, this is--I assume--an example of clippy uncleanliness that you encountered, but there are others that you didn't. If the goal is to make generated code clippy-clean, how thorough would we want to be? How would we want to build out the testing framework to ensure clippy-cleanliness? More specifically, I worry about future changes (e.g. by me!) forgetting to add to this "is it Copy" list that we've hard-coded.

So those are my thoughts. I hope that can be the start of a discussion; they're not intended as an edict--just the basis on why we are where we are.

[notpeter]

Initial thoughts: progenitor has not had as a goal generation of clippy-clean code.
Clippy is intended for code written by humans. Or written by LLMs and read by humans?

This makes some sense. In this case the Clippy warnings are harmless, and like your .is_empty() vs .len() == 0 example the compiler is likely generating (effectively) the same code either way. I'm actually more interested in the other areas where Clippy may be identifying code which is valid rust but may be mislead the human reader about what the generated code does.

If the goal is to make generated code clippy-clean, how thorough would we want to be? How would we want to build out the testing framework to ensure clippy-cleanliness?

I think generating clippy-clean code is potentially a worthy goal. I have not done an audit of projects/repos that depend on progenitor to identify scope of effort -- my projects have only triggered one Clippy lint (clone_on_copy) -- no ideas whether generated code generated less simple clippy warnings. My gut would be that it might be reasonable to (optionally) pipe generated code through a cargo clippy --fix just we currently leverage rustfmt-wrapper to format the output. We could assert test output here was Clippy-compliant or explicitly document it's non-compliance.

After writing the above paragraph, that is the direction I've decided to go with. No changes to progenitor and I get cleaner code. Created an issue for tracking this idea going forward:

• Generate Clippy clean code #1359