Use String or Int to set the opt level

[Rustin170506]

Address https://github.com/rust-lang/rust/pull/112756/files#r1249345725

Use String or Int to set the opt level.

r? @jyn514

[jyn514]

don't have time to review this, sorry

r? bootstrap

[Rustin170506]

Thanks for your review! 💚 💙 💜 💛 ❤️

[Kobzol]

I tried it locally. When you enter anything else than the correct values, the only error you will get is this:

failed to parse TOML configuration 'config.toml': data did not match any variant of untagged enum RustOptimize for key `rust.optimize`

It is because the enum is marked as #[serde(untagged)], so when all the individual variants fail to parse, then it fails with this generic message.

This error message is not very good, and basically it makes your error checking code useless. I think that it would be better to just implement deserialize for the whole enum, and combine all the parsing code in a single function, to provide a reasonable error message. So first try to parse as bool, if it works, return bool. If not, try to parse as string, if it's "s"/"z", return string. Lastly, try to parse as an u8, if it's <= 3, return int. Otherwise show the full error message.

It would also be nice to document the possible values to config.example.toml, to the rust.optimize key comment.

[Rustin170506]

Tested locally:

➜  rust git:(master) ✗ ./x.py build
Building bootstrap
    Finished dev [unoptimized] target(s) in 0.12s
failed to parse TOML configuration 'config.toml': unrecognized option for rust optimize: "9", expected one of 0, 1, 2, 3, "s", "z", true, false for key `rust.optimize`

[Rustin170506]

Self check

[Kobzol]

I was hoping that we could implement the deserialize method more nicely, like this:

impl<'de> Deserialize<'de> for RustOptimize {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        use serde::de::Error;
        if let Ok(value) = bool::deserialize(deserializer) {
            return Ok(RustOptimize::Bool(value));
        }
        if let Ok(value) = u8::deserialize(deserializer) {
            if value <= 3 {
                return Ok(RustOptimize::Int(value));
            }
        }
        if let Ok(value) = String::deserialize(deserializer) {
            if value == "s" || value == "z" {
                return Ok(RustOptimize::String(value));
            }
        }
        return Err(D::Error::custom(
            r#"unrecognized option for rust optimize: "{}", expected one of 0, 1, 2, 3, "s", "z", true, false"#,
        ));
    }
}

but sadly it seems that this is not possible, because we cannot use deserializer multiple times. It would be possible with serde::Value, but I don't want to include another dependency just for this or use serde_json::Value. So let's keep it as it is :)

Delete the visit_i64 method and I think that this is good to go.

[Rustin170506]

Delete the visit_i64 method and I think that this is good to go.

If we don't add it then we can not handle rust.optimize = 0. We can handle it as i64 to avoid this issue.

[Kobzol]

Oh, damn, good point. Ok, then let's delete visit_u64 instead and do e.g. this:

fn visit_i64<E>(self, value: i64) -> Result<Self::Value, E>
where
    E: serde::de::Error,
{
    if matches!(value, 0..=3) {
        Ok(RustOptimize::Int(value as u8))
    } else {
        Err(format_optimize_error_msg(value)).map_err(serde::de::Error::custom)
    }
}

[Kobzol]

Great, thanks!

@jyn514 I'm not part of the bootstrap team, are you OK with me approving this?

[Mark-Simulacrum]

Feel free to approve. It would be good to squash commits here though.

[Mark-Simulacrum]

r? @Kobzol

[Kobzol]

Ok, @hi-rustin please squash the commits (let me know if you want me to help with that!) and I'll approve it.

[Rustin170506]

Ok, @hi-rustin please squash the commits (let me know if you want me to help with that!) and I'll approve it.

Done.
Thanks for your review! 💚 💙 💜 💛 ❤️

[Kobzol]

@bors r+

[bors]

📌 Commit 92b5d0c has been approved by Kobzol

It is now in the queue for this repository.