Remove all casts that use the as operator

[briansmith]

Rust doesn't have implicit widening, so casts that are lossy (not necessarily safe) look just like casts that are 100% safe. Instead of using the as operator for widening casts, we should use (and create, if necessary) functions that safely do widening conversions, and use those. Then the use of as would be limited to only those functions.

This depends on #4.

[Ms2ger]

std::convert::From is implemented for widening conversions in Rust nightly, so let x: u8 = ...; let y: u16 = x.into(); should now work.

[briansmith]

Thanks, Ms2ger. I'm looking forward to taking advantage of that.

See rust-lang/rfcs#1218 and rust-lang/rust#28921.

I left a comment rust-lang/rust#28921 (comment) wherein I ask for additional cast-free conversions.

As part of fixing this issue, or in a follow-up, we should add a lint (static analysis) that verifies there are no uses of as in the code.

[briansmith]

Fixed, at least as far as the current state of the code goes, in 377f611 and a8276b2. One line of test code from 377f611 casts u16 to usize, but that will hopefully be removed if/when my Rust PR is accepted: rust-lang/rust#29220.

[briansmith]

This will become an issue against as we'll be casting ring::der::OID as u8 for use in ring::input::Reader::peek. :(

[briansmith]

For the ones where implicit conversion doesn't work yet (e.g. conversions to/from isize), into() and from() might work:

fn main() {
    let x: u32 = 10;
    let y: u64 = x.into();
    let z: u64 = u64::from(x);
}

(from https://internals.rust-lang.org/t/the-problem-with-array-slice-vector-indexes/3129)

[briansmith]

In sha1.rs there is now this:

let data = data as *const [u8; BLOCK_LEN];

We should make a function and put it in ring::polyfill to avoid using as directly in the code. It actually needs to be a macro until generics over integers are supported in Rust.

/cc @SimonSapin

[SimonSapin]

I don’t mind moving that line into a separate function, but I don’t see how that would improve safety. As long as the input is a *const u8 raw pointer we still have to make the same assumptions about that pointer.

[briansmith]

The goal of the polyfill submodule is to identify missing functionality in Rust's stdlib or language where the lack of the functionality forces a program to unnecessarily use unsafe. My goal is to to eventually help the Rust community find improvements to the language and stdlib so that ring::polyfill can go away.

Similarly, the long-term goal of this ring issue is to shape ring into an example that demonstrates that we can (and should) live without the as operator in Rust, so that as can be deprecated and eventually removed. So, the relative safety/unsafety of each individual usage matters less than whether or not there are any usages.

Normally I don't merge any changes with as in them at all, so I just updated this issue with the above note so I can remember that I did so, and I just CC'd you in case you're interested in the issue/goal.

[briansmith]

I believe there is a non-default clippy lint for this so we should be able to enable that lint to verify this.

[briansmith]

Quoting the documentation for as_conversions:

Note that this lint is specialized in linting every single use of as regardless of whether good alternatives exist or not. If you want more precise lints for as, please consider using these separate lints: unnecessary_cast, cast_lossless/cast_possible_truncation/cast_possible_wrap/cast_precision_loss/cast_sign_loss, fn_to_numeric_cast(_with_truncation), char_lit_as_u8, ref_to_mut and ptr_as_ptr. There is a good explanation the reason why this lint should work in this way and how it is useful in this issue.

And, sure enough, it looks like "just" enabling the clippy::as_conversions lint causes way too many false positives without good solutions. So in PR #1784 I took Clippy's advice and started enforcing all of the above lints.

However, PR #1784 still leaves some things to be desired; see that PR for details.

[briansmith]

PR #1892 removes Pointer -> usize conversions that were done for the purpose of aligning pointers. I believe that's the last change that warrants having a tracking issue, so I'm closing this.