RFC: add a style guide

[djc]

This is based on a style guide from an existing private repository which I wrote last year. A lot of this is based on a style that I've refined with @Ralith while working on Quinn.

[djc]

Currently I believe rustls more often uses a crate, external, std/core style, so we can decide which order we prefer. I believe the outside-in style described here is more idiomatic. There is some support in rustfmt and tooling like Rust-Analyzer for splitting up blocks like this.

[codecov]

Codecov Report

Merging #1407 (4d8081d) into main (5a1b369) will not change coverage.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1407   +/-   ##
=======================================
  Coverage   96.43%   96.43%           
=======================================
  Files          72       72           
  Lines       15163    15163           
=======================================
  Hits        14622    14622           
  Misses        541      541           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[djc]

One thing that's not described here but which we do in Quinn is a discussion of optimal module sizes. For Quinn, we like to target modules (files) in the 400-800 line category. To me, that feels like a good size where on the one hand, you don't end up jumping between many small files too much but on the other hand you don't end up searching through a large file a lot.

[ctz]

Hm, I don't feel fully on board with this still. Especially, I've been considering writing a PR which refactors src/msgs/deframer.rs into a style where the comments that introduce sections of code are in individual singler-caller functions that can be understood incrementally, as opposed to a single 200-line one which has to be understood as a whole.

As an illustration as to why I think single-use functions are better than comments: there's a comment there which talks about "place the message onto the frames output queue", but there is no such queue now. If there was a single-use function like fn deframe_one_message_if_present(output: &mut VecDequeue<Message>) (or whatever) the compiler would be involved in ensuring the structural documentation (the names, arguments, returns of the smaller functions) made sense.

[djc]

I agree the deframer is pretty gnarly, although I think it's not very representative of the larger point (with its nested loops). This part of the style guide tries to thread the needle of making recommendations for the common case without imposing strict rules -- I know there have certainly been other areas where I did go for single-use functions to make the high-level flow clearer. Do you want to try to reword this into something that you'd feel more comfortable with?

For your specific example, it sounds like that comment might just be outdated, which might be a separate issue -- but I'm also curious how you would structure MessageDeframer::pop(). It can also sometimes be tricky to get the borrow checker to handle splitting up code like this.

[djc]

As a discussion point, consider something more like server::tls13::client_hello::handle_client_hello(). Do you also feel that should get split up into smaller functions?

[ctz]

Do you also feel that should get split up into smaller functions?

Yes, I think so. A prime candidate there would be the block around where resumedata needs to be mut. If all that was in a function that fully decides on resumedata/chosen_psk_index, I think it would be clearer.

I don't think I'd extract more than one or two functions from there though -- I think it's beneficial for that function if it contains all the possible transitions to subsequent states in the function itself. But quite a lot of it is of the form if client_hello.something().has_some_required_property() { return Err(cx.common.send_fatal_alert(...)); }`.

[djc]

That's fair, I think both the chosen_share == None and psk handling are kinda clear candidates here. So do you think we should just strike this language, or will it still be useful if weakened form?

[cpu]

or will it still be useful if weakened form?

I don't have a strong opinion but I think there's still utility in a weakened form.

[djc]

Here's a somewhat softer take, what do you think?

"While single-use functions can make sense if the algorithm is sufficiently complex that it warrants an explicit name and interface, using many short single-use functions can make the code harder to follow, due to having to jump around in order to gain an understanding of what's going on. When writing a single-use function, consider whether it needs the dedicated interface, or if it could be inlined into its caller instead."

[ctz]

Thanks for writing this down!

[cpu]

Thank you for putting this together @djc, it's really nice to have this written out in one place. It's a great document for newcomers to the project (or Rust generally).

I've also been collecting up a list of things I've picked up from code review over the past few months. Here are some suggestions to consider (in no particular order). Some might be too specific but I think a few are worth rolling in to this document:

1. Avoid type annotations where possible, and when required, prefer using the turbofish
   (This might be controversial, e.g. see this comment from @jsha on a rustls-ffi PR where he argued we should prefer the opposite approach).
2. Avoid free-standing functions, prefer using associated methods.
3. Avoid ref in match expressions, prefer taking a reference on the scruntinee.
4. Avoid validate type methods that could be forgotten, prefer "parse, don't validate" style.
5. Hoist Ok and Err to the outside of a match where possible, instead of duplicating in the match arms.
6. Don't elide lifetimes, always include <'_> to avoid confusion.
7. Use impl ... in arguments where possible to avoid generic type argument indirection that's harder to read in one pass.
8. Prefer expanding acronyms in function names, arguments etc (e.g. keyusage vs ku) and avoid shorthands (e.g. anon vs anonymous).
9. Avoid type aliases since they can obfuscate the underlying type without providing significant additional type safety.
10. Prefer hex for numeric literals as opposed to decimal.
11. Write enum variants in alphabetic order.
12. Prefer enum variants that are active verbs (e.g. Allow vs Allowed).
    13 Import core traits (e.g. use TryFrom vs core::convert::TryFrom).

WDYT?

[ctz]

• Don't elide lifetimes when naming types that are generic over lifetimes, always include <'_> to avoid confusion.

Edited this one. I don't think the intention is to avoid lifetime elision in all places, just in ones where there's type generic over a lifetime. That matches what the rust reference says: https://doc.rust-lang.org/reference/lifetime-elision.html (the line ending "elided - preferred".

I think we have clippy::needless_lifetimes enabled which should point out lifetime expansions that don't add to the code.

[djc]

• Don't elide lifetimes when naming types that are generic over lifetimes, always include <'_> to avoid confusion.

Edited this one. I don't think the intention is to avoid lifetime elision in all places, just in ones where there's type generic over a lifetime. That matches what the rust reference says: https://doc.rust-lang.org/reference/lifetime-elision.html (the line ending "elided - preferred".

I think we have clippy::needless_lifetimes enabled which should point out lifetime expansions that don't add to the code.

Not sure what you mean by "edited", is this coming to a force push in the future?

My intent here was definitely to avoid lifetime elision in paths (that is, type names) to use the placeholder lifetime instead of not mentioning any lifetimes at all -- so I think we're in agreement.

[ctz]

• Don't elide lifetimes when naming types that are generic over lifetimes, always include <'_> to avoid confusion.

Edited this one. I don't think the intention is to avoid lifetime elision in all places, just in ones where there's type generic over a lifetime. That matches what the rust reference says: https://doc.rust-lang.org/reference/lifetime-elision.html (the line ending "elided - preferred".
I think we have clippy::needless_lifetimes enabled which should point out lifetime expansions that don't add to the code.

Not sure what you mean by "edited", is this coming to a force push in the future?

Sorry, I had quoted and edited @cpu's item 6.

My intent here was definitely to avoid lifetime elision in paths (that is, type names) to use the placeholder lifetime instead of not mentioning any lifetimes at all -- so I think we're in agreement.

Yep!

[djc]

WDYT?

Ahh, awesome list of @djc's greatest nits. I'll see if I can splice these in.

[cpu]

Ahh, awesome list of @djc's greatest nits. I'll see if I can splice these in.

😆 Thanks!

[cpu]

@djc is this branch still on your radar? Would it be helpful if I tried integrating my big-list-of-nits into the text?

[djc]

@djc is this branch still on your radar? Would it be helpful if I tried integrating my big-list-of-nits into the text?

Yes, that would definitely be helpful! It is still on my radar, but so are a bunch of other things...

[cpu]

Yes, that would definitely be helpful!

Sounds good. I will make a PR targeting this branch so you can review the additions.

[cpu]

I will make a PR targeting this branch so you can review the additions:

Up for review in #1461

[djc]

Rebased this on main.

[cpu]

Thanks again, this is a nice update 📚