Misleading/confusing error message for TOML type error (for lto profile)

[djmcgill]

Problem

I had, in my toml file:

[profile.dev]
lto="false"

and this results in the error message:

error: incorrect value `false` for codegen option `lto` - either a boolean (`yes`, `no`, `on`, `off`, etc), `thin`, `fat`, or omitted was expected

As per the docs, both:

[profile.dev]
lto=false

[profile.dev]
lto="off"

work as expected, but just not the string "false". It's quite surprising to have "false" and be told that a boolean such as yes was expected instead.

Steps

1. put the following in your Cargo.toml

[profile.dev]
lto="false"

2. run cargo build

Possible Solution(s)

I propose that the error message clarify this string vs bool (I note it doesn't include "false" in the list of valid booleans), possibly special cased to the strings "true" or "false". Presumably this behaviour is like this for legacy reasons, believe me I've been there myself.

I would prefer (by which I mean, I think it's more intuitive) if the strings true/false were also accepted, which also allows for potentially deprecating the legacy yaml bools, but I understand that it's redundant and also a semantics change.

Notes

I'm happy enough to make this change myself, the error message change doesn't seem too bad but I don't exactly have the attention span the shepherd a breaking change through an RFC and a deprecation cycle or whatever.

Version

cargo 1.62.0-nightly (dba5baf 2022-04-13)
release: 1.62.0-nightly
commit-hash: dba5baf4345858c591517b24801902a062c399f8
commit-date: 2022-04-13
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1m)
os: Debian 10 (buster) [64-bit]

[bstrie]

I'm happy enough to make this change myself, the error message change doesn't seem too bad but I don't exactly have the attention span the shepherd a breaking change through an RFC and a deprecation cycle or whatever.

Nothing so involved is necessary, error messages aren't considered stable. Making Cargo accept "false" rather than false would be a larger change, so I'd just stick to improving the error message for now. That said, it's completely news to me that "on"/"off"/"yes"/"no" would be accepted at all... given that apparently they already work I don't think it would much of a stretch to also accept "true"/"false" (or maybe we should just deprecate those strings in favor of actually using booleans, I dunno).

[djmcgill]

I am pretty sure (but have not actually checked) that it used to be a boolean property, and then the "thin" option was added as the classic 3rd boolean variant, leading to the current situation. Then, whoever was implementing it, was like "well if we're accepting strings here anyway might as well accept truthy things" or something. Like I said, believe me I've been there myself.

[djmcgill]

Ah, I did search before but just found a related (but not duplicate) issue is #7491

[weihanglo]

Actually, cargo does little work here. The value itself is passed down into rustc (for both parsing and error message). It's confusing because the message is not from cargo but rustc.

I agree the error message could be better, though the deserialization of TomlProfile is a simple serde derive. I cannot think of a good way to improve it without piling up the complexity 😞

[djmcgill]

Ah, I see thanks, would it be better to close and re-open there? Unless somebody has the ability to move the issue between repos (if that's a thing).

On looking at the code, I see that the only string options that are actually accepted are "thin"/"fat" (ignoring the wildcard case), and then all the other valid values are purely from how the YAML spec specifies boolean values.

If I were inclined to change semantics, the shortest possible change is just adding extra cases to the string pattern match (in a backwards compatible way) while keeping the StringOrBool parsing the same. Something slightly more involved would be to replace StringOrBool with a bespoke type and serde instances although that wouldn't really be appreciably different from the existing parse_lto method in practice probably.

The smallest possible change in general would be to just edit that error message string with something with extra information in like, e.g.

either a boolean value (`true`, `false`, `yes`, `no`, `on`, `off`, etc), or a string value (`"thin"` or `"fat"`), or omitted

I'm now convinced the true culprit here is YAML's frankly unreasonable permissiveness but there's not much we can do about that.

@rustbot label +A-errors

[rustbot]

Error: The feature relabel is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please let @rust-lang/release know if you're having trouble with this bot.

[weihanglo]

Hmm… I feel like the meaning of true and false are things defined in cargo. Rustc knows nothing about boolean options. So transfering this issue to rust-lang/rust might not be a good idea.

I sent myself back to the history and found that the y|yes|n|no seems to exist in pre 1.0 era. I can't say there wasn't a YAML config at that time. It's out of my knowledge 😆. But nowadays Cargo chooses TOML for its config and manifest file, which doesn't accept string-as-boolean. Maybe we shouldn't introduce too many implicit conversions here? I'd say it's convenience but somewhat obscure. Even YAML won't treat quoted "false" as boolean.

[djmcgill]

Ah yes sorry, TOML not YAML, don't know why I thought that. parse_opt_bools (current) semantics to semi-recreate YAML's bool parsing certainly still seems weird to me to not allow "true"/"false", when we're already implicitly converting to a boolean anyway, but you've convinced me that the semantics are at least intentional so maybe just changing the error message to make the string/boolean distinction clear is the best bet then.

[djmcgill]

After discussion on the rust side of things, it turns out that rustc's semantics for the CLI args are different than the cargo profile value. Because cargo is just deriving Deserialize on an untagged String|Bool enum, the boolean values true/false are parsed. Rustc, however, only parses from a String. The bools which cargo accepts are converted into Strings before being passed to rustc.

The end result of this is that simply changing the error message in rustc is not correct since rustc and cargo have different accepted values.

It was suggested that I add special-cased validation logic here i.e. while the arg is still an unparsed (toml) string, before rustc's parse_opt_bool has had a chance to reject it.

All that's left is the choice between the slightly weird option of only special casing "true"/"false" and letting anything else fall through to rustc's error, or duplicating rustc's parse logic purely for validation but then at least there's only one possible error string for incorrect string values in this field.
The former would look like

        if let Some(lto) = &self.lto {
            if let StringOrBool::String(a@"true" | a@"false") = lto {
                bail!(
                    "`lto` setting of string `\"{}\"` is not a valid setting, \
                     must be a boolean `true`/`false` or a string \
                     `\"yes\"`/`\"no\"`/`\"on\"`/`\"off\"`/`\"y\"`/`\"n\"`/ \
                     `\"thin\"`/`\"fat\"` or omitted.",
                     a
                );
            }
        }

and the latter would be a negation of the match as allow-list semantics instead of block-list ones.

[weihanglo]

Thanks for the investigation. The former looks good to me, though I would try to avoid hardcoding all the LTO options in the message in case of out-of-sync.