Skip to content

Remove panic from to_digit and from_digit #30138

Closed
@alexchandel

Description

@alexchandel

The panics in from_digit and to_digit are rather arbitrary:

pub fn from_digit(num: u32, radix: u32) -> Option<char> {
    if radix > 36 {
        panic!("from_digit: radix is too high (maximum 36)");
    }
    if num < radix {
        let num = num as u8;
        if num < 10 {
            Some((b'0' + num) as char)
        } else {
            Some((b'a' + num - 10) as char)
        }
    } else {
        None
    }
}
...
   fn to_digit(self, radix: u32) -> Option<u32> {
        if radix > 36 {
            panic!("to_digit: radix is too high (maximum 36)");
        }
        let val = match self {
          '0' ... '9' => self as u32 - '0' as u32,
          'a' ... 'z' => self as u32 - 'a' as u32 + 10,
          'A' ... 'Z' => self as u32 - 'A' as u32 + 10,
          _ => return None,
        };
        if val < radix { Some(val) }
        else { None }
    }

Both functions already return a None when the number above the radix, and should also return None rather than crashing the program when the radix is greater than 36. If a crash is still desired, the Option<char> can simply be unwrapped.

This change is very simple to make. Although both functions are stable, this panic is not encoded in their signatures, and nothing in the standard library relies on this hidden panic; in fact from_digit is always unwrapd, while to_digit is either unwrapped or matched a way compatible with the elimination of this panic (though an error message may need to be tweaked). A cursory search of GitHub suggests this is in fact the case everywhere else, with every invocation I could find either passing 2, 10, 16, or 36 as the radix, unwrapping the result, or manually checking whether the radix was greater than 36 and explicitly panicking if so.

Since libcore is being stabilized right now, I'd offer that panicking in the core library should be minimized, since every possible panic reduces Rust's applicability and usefulness to baremetal systems where safety is a concern.

Activity

apasel422

apasel422 commented on Dec 1, 2015

@apasel422
Contributor

I think the original reasoning for this is that supplying a radix greater than 36 is a programmer error. These methods would always yield None in that case.

alexchandel

alexchandel commented on Dec 1, 2015

@alexchandel
Author

@apasel422 I think always yielding None is a good idea for that case.

If these weren't stable, it might be possible to avoid programming errors with an "alphanumeric" enum ranging from _0 to _35, or with dependent types and a finite set. Another way could be to change the result to something like: Digit char | NumTooLarge | RadixTooLarge and Num u32 | CharIsNotDigit | RadixTooLarge.

My concern is with radices that originate from the program's input, since there's no statically verifiable way to avoid calling to_digit with an impure radix that could be greater than 36.

It's also worth noting that the names are backwards, since "digits" are used to represent numbers in a positional numeral system of a particular-radix, and thus our from_digit actually returns digits while our "to_digit" returns numbers ;)

steveklabnik

steveklabnik commented on Dec 1, 2015

@steveklabnik
Member

This also changes well-documented semantics of a stable method, we can't really do this.

alexchandel

alexchandel commented on Dec 1, 2015

@alexchandel
Author

@steveklabnik Why not? They're dangerous semantics, and no one relies on them anyway.

steveklabnik

steveklabnik commented on Dec 1, 2015

@steveklabnik
Member

This is the burden of stability. We've made a promise to our users, and now we have to keep it.

alexchandel

alexchandel commented on Dec 1, 2015

@alexchandel
Author

:\ well here's hoping for Rust 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @steveklabnik@alexchandel@apasel422

        Issue actions

          Remove panic from to_digit and from_digit · Issue #30138 · rust-lang/rust