Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTC_OFFSET_FORMAT: use const blocks around function calls #704

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Sep 4, 2024

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 rust-lang/rust#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. 😂

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

RalfJung commented Sep 4, 2024

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.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RalfJung
Copy link
Contributor Author

RalfJung commented Sep 5, 2024

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

jhpratt commented Sep 5, 2024

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

RalfJung commented Sep 5, 2024

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

jhpratt commented Sep 5, 2024

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

RalfJung commented Sep 5, 2024

Done :)

@jhpratt
Copy link
Member

jhpratt commented Sep 6, 2024

Thanks!

@jhpratt jhpratt merged commit 2e68a36 into time-rs:main Sep 6, 2024
22 checks passed
@RalfJung RalfJung deleted the promotion branch September 6, 2024 06:57
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.

2 participants