time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.13k stars 281 forks source link

UTC_OFFSET_FORMAT: use const blocks around function calls #704

Closed RalfJung closed 2 months ago

RalfJung commented 2 months ago

Implicit promotion (where &expr terms implicitly create a new constant with value expr and a borrow with lifetime 'static to such a constant) has a long and troubled history. Right now we are in a state that's at least reasonably well-behaved, but it does make for some rather strange behaviors, as is tracked in https://github.com/rust-lang/rust/issues/124328. I wanted to do a crater experiment to see just how much code out there relies on this, by making rustc do longer do promotion of function calls, and it turns out not even cargo builds any more then. :joy:

The crate that failed to build is time. So I wonder if it would be okay to patch time to make it work under this more strict, more sane promotion regime. The main point in the new regime is to use explicit const { ... } blocks to make things into constants, instead of doing that implicitly. So here we wrap all function calls in const blocks, so that the entire outer expression is just a bunch of array constructors, enum constructors, &, and consts -- which is trivially promotable.

Landing this will raise the MSRV to 1.79. I realize it's too early to do that under your MSRV policy. 1.81 will be released tomorrow so then it would be covered by the policy, but I'm also completely fine with delaying this until later. I'm in no rush here. :)

RalfJung commented 2 months ago

Strange, when I format this file I get a diff in unrelated parts of the file... I'll have to manually only format the part that I changed then.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.6%. Comparing base (4a74924) to head (a4daf75). Report is 44 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #704 +/- ## ======================================= - Coverage 97.8% 97.6% -0.2% ======================================= Files 81 83 +2 Lines 9378 9008 -370 ======================================= - Hits 9169 8790 -379 - Misses 209 218 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

RalfJung commented 2 months ago

Also to be clear, this code will not be rejected on the current edition ever. If we do any kind of change here, it will be over an edition. But it would still be interesting to know just how wide-spread this pattern is -- last time we tried this, several years ago, it was only 2 or 3 crates that were affected.

jhpratt commented 2 months ago

Would the same behavior result if using separate const items rather than inline const? That would avoid the need for an MSRV bump (which would actually need to wait another 12 weeks as it's an internal change).

Strange, when I format this file I get a diff in unrelated parts of the file... I'll have to manually only format the part that I changed then.

Try cargo +nightly fmt — a number of options are used that aren't stable, and that may be the cause of that.

Also to be clear, this code will not be rejected on the current edition ever. If we do any kind of change here, it will be over an edition. But it would still be interesting to know just how wide-spread this pattern is -- last time we tried this, several years ago, it was only 2 or 3 crates that were affected.

As the primary intent here is to get a crater run, is it possible to apply a patch (in Cargo.toml) to cargo? It seems like that would solve the issue without a release. Given RustConf prep, I probably won't get around to a release for at least another week.

RalfJung commented 2 months ago

Would the same behavior result if using separate const items rather than inline const? That would avoid the need for an MSRV bump (which would actually need to wait another 12 weeks as it's an internal change).

Ah I see. Yes using const items is also possible, just a bit less nice.

As the primary intent here is to get a crater run, is it possible to apply a patch (in Cargo.toml) to cargo?

Cargo is a submodule, so it can't be easily patched from the rustc repo. Also that crater run would then show tons of failures from time-rs failing.

As I said I am in no rush here, waiting a few weeks is fine.

jhpratt commented 2 months ago

Ah I see. Yes using const items is also possible, just a bit less nice.

Let's do that for the time being. It would avoid the MSRV update and having to wait another 12 weeks to be permitted to do it (per policy). Feel free to include a TODO saying to use inline const once able.

Cargo is a submodule, so it can't be easily patched from the rustc repo. Also that crater run would then show tons of failures from time-rs failing.

I considered the latter but am not familiar with the inner workings of crater.

As I said I am in no rush here, waiting a few weeks is fine.

Sounds good. With the requested update, I might be able to get it out this weekend (or Monday) depending on if I have time to review the changes since the most recent release.

RalfJung commented 2 months ago

Done :)

jhpratt commented 2 months ago

Thanks!