Consider removing all the type annotations on let bindings

[djc]

In places where these aren't needed, the code looks verbose and unidiomatic to me. How would you feel about removing these? In general I'm pretty sure we can trust the type system, and the compiler will point out when we need annotations (that said, I don't have much experience with FFI bindings, so maybe it's different here).

[jsha]

Thanks for the suggestion! I definitely agree we can trust the type system. I use abundant type annotations to make comprehension easier for the future reader. For instance, consider this code:

        let mut private_keys = match pkcs8_private_keys(&mut Cursor::new(private_key)) {
            Ok(v) => v,
            _ => match rsa_private_keys(&mut Cursor::new(private_key)) {
                Ok(v) => v,
                Err(_) => return rustls_result::PrivateKeyParseError,
            }
        };
        let private_key = match private_keys.pop() {
            Some(p) => PrivateKey(p),
            None => return rustls_result::PrivateKeyParseError,
        };

Even though I wrote this last week, I would be hard pressed to tell you whether private_keys is an Iterator<Item=PrivateKey>, Vec<PrivateKey>, or (the right answer) a Vec<Vec<u8>>. There are some clues that can guide you - for instance, if you look in the second statement, you can see the call to .pop(), which indicates private_keys is something with a .pop() method, probably a Vec or VecDeque. And on the next line, you can see struct literal PrivateKey(p), which suggests the elements of private_keys are not of type PrivateKey. But this requires the reader to do type inference in their head, which I think is quite hard.

Compare to this version:

        let mut private_keys: Vec<Vec<u8>> = match pkcs8_private_keys(&mut Cursor::new(private_key)) {
            Ok(v) => v,
            _ => match rsa_private_keys(&mut Cursor::new(private_key)) {
                Ok(v) => v,
                Err(_) => return rustls_result::PrivateKeyParseError,
            }
        };
        let private_key: PrivateKey = match private_keys.pop() {
            Some(p) => PrivateKey(p),
            None => return rustls_result::PrivateKeyParseError,
        };

At each let statement, it's quite clear to the reader what types they are looking at. This builds confidence and allows them to spend their mental energy elsewhere. I think this reader aid is especially valuable since we may have C programmers who are new-ish to Rust reading this codebase.

[djc]

My other point which I didn't mention is that my IDE (I use VS Code with Rust Analyzer) provides this for me while keeping the code clean, so I guess I'm still not convinced of its utility.

Also, presumably you might also want to attract Rust folks, who might be turned off by the abundant type annotations. I usually feel like it's better to write boring/common code; in Rust, leaving out unnecessary type annotations is also just unidiomatic.

It's up to you, of course.

[cpu]

I had been idly thinking about doing a pass to remove type annotations where not necessary and then I found there was already this issue about the idea. I wonder if it's a more appealing idea in 2024 than it was in 2021?

I would be a +1. To me it feels like the broader Rust community has converged on rust-analyzer providing these hints in-line.

Another thing to think about: I've been landing new code in-repo that has been omitting explicit types where the compiler allows (mostly since that's the style I've been using in other repos). I think that shows that it can be difficult to maintain consistency for a style preference like this that can't be easily linted for.

[cpu]

@jsha Now that the PR queue is cleared out, can I get your temperature on this issue? Has your perspective changed since 2021?

[jsha]

I still feel pretty strongly that explicit type annotations, used judiciously, are a great benefit to readability, particularly when types are an important part of understanding why a piece of code is correct. However, since rustls-ffi is now wrapped more fully into the rustls project the overriding value is consistent project-wide style, so let's go for it!

[cpu]

Sounds good, thanks for weighing in. When I have a branch with the types removed I'm open to a middle ground where we don't include them by default, but leave them in for certain cases that are particularly load bearing when it comes to readability if any you feel strongly about come up in review.