Lokathor / tinyvec

Just, really the littlest Vec you could need. So smol.
https://docs.rs/tinyvec
Apache License 2.0
648 stars 49 forks source link

Use `rustversion` to detect available Rust version #165

Closed 8573 closed 2 years ago

8573 commented 2 years ago

tinyvec currently uses crate features rustc_1_40, rustc_1_55, and rustc_1_57 to enable functions and optimizations that require Rust versions higher than the library's base MSRV of 1.34.0.

This patch replaces the uses of these crate features with dtolnay's macro rustversion, which automatically detects the version of rustc with which the code is being compiled, thus allowing the optimizations enabled by newer Rust versions to be applied regardless of whether a dependent requests any of the rustc_* crate features. This may be especially useful for dependents whose own MSRVs would bar them from using those crate features without gating them behind crate features of their own, potentially requiring a proliferation of such crate features through a dependency tree.

I would have limited this patch to using rustversion to enable optimizations automatically and not to replace the use of the crate features to gate functions that require Rust versions newer than 1.34.0, because I thought that using rustversion rather than the crate features to gate such functions might have been undesirable because it would mean losing the "Available on crate feature xyz only" hints on Docs.rs, but I see that Docs.rs doesn't apply those hints to functions anyway, so no such hints are lost by switching the gating mechanism to rustversion.

This patch further uses rustversion to add compilation errors in case one of the rustc_* crate features is requested and the available Rust version is too old, such that the rustc_* crate features now function simply as static assertions that the running rustc supports the indicated Rust version.

This patch, of course, adds a dependency on rustversion, which becomes this library's only non-optional dependency. Its MSRV is 1.31.0 and so does not raise the MSRV of this library. If having a non-optional dependency is unacceptable, an alternative could be to have rustversion be an optional, on-by-default dependency and to rely on the rustc_* crate features as before if rustversion is disabled. Rather than

#[rustversion::since(1.57)]

the conditional compilation clauses would look like

#[cfg(any(feature = "rustversion", feature = "rustc_1_57"))]
#[cfg_attr(feature = "rustversion", rustversion::since(1.57))]

which is verbose enough that I suspect that rejecting rustversion altogether would be preferred.

I admit that I do not understand why the comment in Cargo.toml on the crate feature rustc_1_40 seems to say that "us[ing] Vec::append if possible in TinyVec::append" and overriding DoubleEndedIterator::nth_back require Rust 1.37 and Rust 1.40 respectively, when the standard library documentation says that Vec::append and DoubleEndedIterator::nth_back were stabilized in Rust 1.4.0 and Rust 1.37.0 respectively.

8573 commented 2 years ago

This change was tested in my fork: https://github.com/8573/tinyvec/actions/runs/2640914561

Shnatsel commented 2 years ago

I believe this would tie tinyvec to Cargo, and make it difficult to build with other build systems that do not necessarily support build.rs

I understand it also ties the crate to rustc, making it difficult to build with alternative implementations such as gcc-rust or mrustc.

I am not strictly opposed to this change, but I wanted to note this to make sure we understand the trade-offs.

8573 commented 2 years ago

I hadn't thought of that. It looks like mrustc emulates rustc --version in a User-Agent-ish way, printing "rustc 1.Y.100 (mrustc ...)" when it's emulating rustc 1.Y. With rustversion already supporting similar since https://github.com/dtolnay/rustversion/pull/29, I don't suppose it would be too difficult for rustversion to support ignoring "(mrustc ...)" as well as "(gentoo)", although I'm not sure how many such accommodations would be welcome.

gccrs seems more complex and I don't know where to find how its version ultimately would be printed. I think this is a version being printed but not necessarily that of the Rust compiler. I'll ask on IRC. I suspect the version output from gccrs is less compatible with rustc's than mrustc's is.

How much of a concern is a build system not supporting build scripts, given the necessity(?) of supporting Autotools?

Lokathor commented 2 years ago

I don't think a non-optional dependency is a good fit for this crate.

Though separately it is interesting that a few things have the wrong minimum version. I think what probably happened is that a 1.40 change added several things, only one of which truly required 1.40, and they just got bundled up together.

8573 commented 2 years ago

gccrs gives version output like

gccrs (<build info...>) 12.0.1 20220118 (experimental)
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I was advised by @dkm

2022-07-09 13:26:20 <dkm> Not sure what is your intent here, but we are not
                    ready (nor do we want) to have ifdef for gccrs. We clearly
                    don't want to create a "version-space" for gccrs
2022-07-09 13:26:46 <dkm> I guess maybe in the future we'll have a version that
                    gives the targeted rustc version
2022-07-09 13:28:59 <dkm> So please, don't try to detect gccrs by any means, at
                    least not for now :)
2022-07-09 13:30:24 <c74d> Should I quote that in the ticket I linked?
2022-07-09 13:31:41 <c74d> (I'm really more asking for permission to quote you
                    than whether I 'should')
2022-07-09 13:33:02 <dkm> Yeah no worry, you can also add @philberty and/or
                    @dkm in the comment :)
2022-07-09 13:33:40 <dkm> that's a sensible subject as some people are afraid
                    of gccrs splitting the ecosystem
2022-07-09 13:34:30 <dkm> so extracting a rustversion from gccrs: OK,
                    extracting that you are using gccrs in your rustversion: KO
                    :)
2022-07-09 13:38:35 <c74d> The question that would be attempted to answer from
                    the version string is "what version of Rust does this
                    compiler support", regardless of what compiler it is.  I
                    acknowledge that there might end up being no simple answer
                    (maybe it supports most but not all of Rust 1.123), for
                    which I have no solution.
2022-07-09 13:38:40 <c74d> Thanks for your advice
2022-07-09 13:39:24 <dkm> Ok, so that's OK. The goal of gccrs is to target a
                    given version and fully support it.
2022-07-09 13:39:41 <dkm> If it's partially supporting it, then it's a bug, or
                    the compiler is not ready
2022-07-09 13:39:54 <dkm> so, not your problem.
2022-07-09 13:40:14 <dkm> And currently, we don't advertise any version, and
                    even if we did, it would be far from working :)
2022-07-09 13:40:26 <dkm> So maybe it's a bit early for you to take gccrs into
                    account :)
2022-07-09 13:41:05 <c74d> Okay, thanks
8573 commented 2 years ago

I don't think a non-optional dependency is a good fit for this crate.

I'll close the ticket on the assumption that having it as an optional dependency is also not desired (e.g., the extra verbosity is seen as not worth the benefit).