don't allow ? in non-result v2 benchmark function defs, require either Result or () return type

[sam0x17]

Changes the v2 benchmarking syntax to only accept () or Result<(), BenchmarkError> as valid return types on benchmark function defs. This change is merely syntactic sugar as the return type is ultimately discarded since the code in these function defs is loaded into a bunch of trait impls.

• Pallet using this syntax (balances)
• Parsing of valid return types
• Compile error messages for valid return types
• UI tests for valid return types
• Compile error when there is a ? operator but return type is ()
• UI test for ? operator
• use alias to BenchmarkResult and simplified parsing
• update UI tests for new simplified parsing
• Update docs

Related Issues

• fixes benchmarking v2 syntax is confusingly lax about return types, leading to confusion #13278
• [FRAME core] ☂ Benchmark syntax V2 follow-ups polkadot-sdk#380

[sam0x17]

So I am going to re-work this a bit to use static_assertions::assert_type_eq_all! and an alias called BenchmarkResult that points to Result<(), BenchmarkError>, which will also vastly simplify the code and the UX

[ggwpez]

I think its good to have the syntax clarification, but not sure if this check here actually works.
It would still be possible to just return Err("".into()) instead of using ? and it would exclude #[block]s from using it.
I dont think we can/have to explicitly check this, or?

[sam0x17]

We could have a visitor that checks all return statements and applies a static_assertions::assert_type_eq_all! check to each one verifying that it is compatible with BenchmarkResult, and that wouldn't be too hard, but honestly now that BenchmarkResult is so short I am leaning towards just requiring -> BenchmarkResult on all benchmark function defs, which would greatly simplify things and wouldn't be a very heavy lift. Thoughts?

[sam0x17]

Either way would work though I think

[sam0x17]

Based on my discussion with @ggwpez I think the best course of action is to actually close this and re-open #13224. There are some clear benefits to generating real functions and one of those benefits is all these weird edge cases will be resolved for free. Additionally some issues, like not using a benchmark parameter within the benchmarking code, will now get warnings that will cause CI to fail (as it should) instead of us having to write crazy visitor patterns and things to identify these ourselves.

So we've come full circle, #13224 is a good idea 😆