Allow integral edition specifier

[est31]

This allows for more ergonomic edition switching. Instead of edition = "2018" you can now do edition = 2018 which is 2 characters less. The string version is still allowed.

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[est31]

r? @matklad as acrichto is not available atm.

[matklad]

Let's use the same code for TomlOptLevel as well? It already does something similar, but in a slightly different way. Maybe it's better even to go in the opposite direction and reuse that logic here?

[matklad]

Hm, or perhaps the best way to rewrite both with #[serde(deserialize_with="string_or_int")]?

[matklad]

An excellent idea!

Let's add a test for it here:

cargo/tests/testsuite/package.rs

Line 1103 in f35302c

fn test_edition() {

And let's use this better way in the docs: https://github.com/rust-lang/cargo/blob/9f097787b04b06cdde4fc42b26a531b22c1b37a6/src/doc/src/reference/unstable.md#edition

@rust-lang/cargo anyone sees any drawbacks here? Seems relatively minor and robust to me, but, because it affects Cargo's public interface, I'd like to double-check with you.

cc @steveklabnik: a small thing, but it affects how your first Cargo.toml looks like

[est31]

@matklad The PR already changes the docs and it also adds a test. Should I do it differently?

As for the suggestion about the serialization, sure can do it.

[steveklabnik]

This adds 87 lines of code to save two characters. (Okay, some of it is whitespace, and some of it is tests, but the point is, you wouldn't need those tests without this.)

Is this actually worth it? I'm not convinced.

[withoutboats]

I prefer not to support integers in this position for consistency. We don't support integers or floating points as version constraints, even though that would have the same motivation. These fields are strings.

[est31]

Semver versions are a.b.c where a,b,c are numbers and you can also have more stuff like 1.2.3-dev or 1.2.3-beta.5. Only in the rare case where you have sth like cratename = "2" you can actually represent it as integer. cratename = "2.5" can be represented as float, but allowing that'd be pretty ugly given that most floats only store approximations... you don't want 0.3 to be the same version as 0.300000004 :p.

Editions are different. For them we only allow 2015 and 2018. The entire value space can be represented as integers. Not sure about future decisions, but if that value space is extended in a fashion that does not allow integer representation, we can still use strings. This PR only allows integers to be used, it does not mandate their use.

@steveklabnik I don't think that point is really valid anyway (how large is the impl Trait in argument position patch xD), but applying @matklad 's suggestion makes my patch much smaller. Just watch!

[matklad]

The PR already changes the docs and it also adds a test. Should I do it differently?

Apparently, it's too early in the morning for me to do code reviews =/ Tests and docs look great to me now that I've actually saw them!

To clarify why I think this is a good idea:

Currently, edition numbers are integers. I think we might introduce something like 2018-pre, or even 2018.1, but the former seems niche, and the latter unlikely. So, editon = 2018 optimizes for the common case. I think the main point here is not saving two characters, but avoiding "2018", which just looks weird: why represent a number as a string if you can represent it as a number?

Note that we already support mixed typed in other places in Cargo.toml, for example, opt-level can be either of 1, "2", "s".

As for consistency with versions/version requirements I agree with @est31: we conceptually use SemVer for package versions and integers for edition versions, so, to me, it doesn't seem that these are the same thing.

[withoutboats]

I think its fairly likely we will someday have an edition which we want to give a non-integral name. Early versions of the preview proposal used the syntax edition = "2018-preview" instead of turning on a lint, and I could see us using that in a future edition preview. And who knows what other names we will use as the concept evolves over the history of Rust.

Unless the type can encode all forms, I'd rather not support it. If I agreed that editions would always be an integral number, I'd be for this (well, really, I'd question the choice to support strings in that case). But I think editions can be named arbitrary strings.

The biggest problem for me is for a user who is new to TOML, and the way this could confuse them into thinking TOML supports bare words in value position (since it does support them in key position). Even more generally, I'd rather keep our use of TOML as "simply typed" as possible and avoid supporting multiple types without strong motivation (the support for both strings and objects for dependencies members is an example that pulls its weight).

[est31]

Hrmmm I've tried doing the #[serde(deserialize_with="string_or_int")] version with something like:

fn string_or_int<'de, T, D>(deserializer: D) -> Result<T, D::Error>
where
    T: Deserialize<'de> + FromStr<Err = ()>,
    D: Deserializer<'de>,
{

and keeping edition: Option<String>,. But it turns out that Option<String> does not implement FromStr. Changing the trait to From<String> would work for edition, but would have problems for the opt level as the conversion has to be failible (As it only allows special prefixes). I could use TryFrom but that's unstable. So I'd say the current version is the easiest? To get a smaller patch I've removed the u64 function, it seems redundant.

[est31]

To further reduce code size, I could remove the where clauses and instead put stuff into the pointy brackets like:

            fn visit_i64<E: de::Error>(self, u: i64) -> Result<Self::Value, E> {
                Ok(StringOrU32::U32(u as u32))
            }

            fn visit_str<E: de::Error>(self, s: &str) -> Result<Self::Value, E> {
                Ok(StringOrU32::String(s.to_string()))
            }

[matklad]

I'd rather keep our use of TOML as "simply typed" as possible and avoid supporting multiple types without strong motivation

It seems that I personally feel much more comfortable with allowing "elaborately typed" toml values. I'd like to allow specifying integers as integers, so that that the user does not have to think "why I need to quote 2018? It is a number, TOML support numbers".

Note that we already support union types in some lesser-obvious places than dependencies: opt-level, lto, debug, build and publish fields all could have more than one type.

Let's talk about this briefly during the next Cargo meeting? I don't have super strong feelings here, but I do find number-represented-as-string weird and I now totally see that this is not as clear-cut as I've initially thought :)

[est31]

@matklad you have added the nominated label. Has this PR been talked about in the cargo team?

[matklad]

Nope, we've run out of time :(

[joshtriplett]

Personally, I'm entirely in favor of this. Current editions use numbers, future editions will likely use numbers, and this makes Cargo accept numbers so people don't have to give advice like "just put edition equals 2018 in your Cargo.toml, but remember to quote the 2018". (Which then leads to the obvious question of "why?".)

[est31]

The period of my contributions to Rust upstream has reached an end. Thus I'm unable to continue my work on this. I still think something like this is a great addition. I urge anyone interested in this change to adopt and continue it from here on. Thanks.

[joshtriplett]

Reopening this as I think it remains valuable.

Thank you for your contributions, @est31.

[alexcrichton]

I'm gonna close this for now as we haven't had a chance to talk about this in rust-lang/cargo for awhile now, but we can always reopen if this comes back up!

[est31]

@alexcrichton please don't reopen this, as I won't maintain it any more, sorry.

If you still want this please open a separate PR! thanks.