Skip to content

Conversation

@8573
Copy link

@8573 8573 commented Jul 9, 2022

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
Copy link
Author

8573 commented Jul 9, 2022

`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 8573 force-pushed the 8573/feat/rustversion/1 branch from 73f2787 to 45a8d4f Compare July 9, 2022 11:36
@Shnatsel
Copy link
Collaborator

Shnatsel commented Jul 9, 2022

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 8573 mentioned this pull request Jul 9, 2022
@8573
Copy link
Author

8573 commented Jul 9, 2022

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 dtolnay/rustversion#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
Copy link
Owner

Lokathor commented Jul 9, 2022

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
Copy link
Author

8573 commented Jul 9, 2022

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
Copy link
Author

8573 commented Jul 11, 2022

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).

@8573 8573 closed this Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants