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

Stabilize LazyCell and LazyLock #121377

Merged
merged 1 commit into from May 26, 2024
Merged

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Feb 21, 2024

Closes #109736

This stabilizes the LazyLock and LazyCell types:

static HASHMAP: LazyLock<HashMap<i32, String>> = LazyLock::new(|| {
    println!("initializing");
    let mut m = HashMap::new();
    m.insert(13, "Spica".to_string());
    m.insert(74, "Hoyten".to_string());
    m
});

let lazy: LazyCell<i32> = LazyCell::new(|| {
    println!("initializing");
    92
});

r? libs-api

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 21, 2024
@pitaj pitaj added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 21, 2024
notgull pushed a commit to rust-windowing/winit that referenced this pull request Feb 25, 2024
Removes the once_cell dependency, instead using std::sync::OnceLock and a
minimal polyfill for std::sync::LazyLock, which may be stabilized soon
(see rust-lang/rust#121377).

This should not require a bump in MSRV, as OnceLock was stabilized in 1.70,
which this crate is using.
@m-ou-se m-ou-se added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Mar 5, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Mar 5, 2024

sidestepping the unresolved questions

It is not fully clear to me which issues this sidesteps. Can you add some clarifications of what is and isn't allowed after this PR, and what a possible upgrade path looks like to a future where LazyCell/LazyLock don't unnecessarily store a function pointer in memory?

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. labels Mar 5, 2024
@pitaj
Copy link
Contributor Author

pitaj commented Mar 5, 2024

@m-ou-se

Unresolved Questions

  1. Default F = fn() -> T in type signature (See comment)

Summary: LazyLock<T, F = fn() -> T> is a hack to make static FOO: Lazy<T> work. (It's also not ideal to store a function pointer in memory unnecessarily). Any alternatives would require new language features.

With the unstable default type parameter, specifying the second type parameter is an error. The only way to get a LazyLock<Thing, {closure}> is via type inference on local variables:

// LazyLock<String, {closure}>
let foo = LazyLock::new(|| "foo".to_string());

// LazyLock<String, fn() -> String>
let bar: LazyLock<String> = LazyLock::new(|| "bar".to_string());

// LazyLock<String, fn() -> String>
static STAT: LazyLock<String> = LazyLock::new(|| "stat".to_string());

Some future possibilities that would allow avoiding the function pointer:

// Just "opaque type inference in statics"
pub static LazyLock<T, F = fn() -> T> { ... }
// lint the missing type parameter
static FOO: LazyLock<String> = ...;
// recommend setting it to infer the closure type
static FOO: LazyLock<String, _> = ...;

// "Opaque type inference in statics" and "type parameter default infer"
pub struct LazyLock<T, F = _> { ... }
static FOO: LazyLock<String> = ...;

// "`impl Trait` in statics" and "type parameter default `impl Trait`"
pub struct LazyLock<T, F = impl FnOnce() -> T> { ... }
static FOO: LazyLock<String> = ...;
  1. Is variance of Lazy correct? (See
    Feature request: Make Lazy<T, F> covariant in F matklad/once_cell#167)

Summary: LazyLock<T, F> is currently invariant in F. Using a type including a lifetime for F can cause "insurmountable" lifetime issues. @matklad agrees that covariance would be better, but it would complicate the implementation (or may be impossible). Nobody has implemented F-covariance for once_cell::*::Lazy yet.

Because the unstable type parameter can't be specified explicitly, it's impossible to set it to a type which can cause those variance issues. The only remotely possible way would be type inference in a local variable, but I can't think of a way that could cause issues, if it's possible at all.

There are only two possible futures here:

  • Lazy* are modified to be covariant in F and the parameter is later stabilized
  • The parameter is stabilized as-is, invariant in F

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 5, 2024
@tgross35
Copy link
Contributor

tgross35 commented Mar 6, 2024

cc @danielhenrymantilla who did a lot of the variance work for once_cell

@tgross35
Copy link
Contributor

tgross35 commented Apr 12, 2024

@m-ou-se is the response in #121377 (comment) sufficient to get this nominated again?

@m-ou-se m-ou-se added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Apr 15, 2024
@pitaj pitaj mentioned this pull request Apr 17, 2024
5 tasks
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Apr 23, 2024
@Amanieu
Copy link
Member

Amanieu commented Apr 30, 2024

We discussed this in a libs-api meeting and decided for a full stabilization of these types as they are right now, including stabilizing the F generic argument.

The reason for also stabilizing F is that even if in the future we get some form of type inference for static, we could just use a lint to suggest using _ to infer F. We expect that if we do get type inference in the future then it is likely that people will end up omitting the type entirely in favor of inference anyways.

Regarding the variance of F, it's not a breaking change to make it co-variant later.

@pitaj
Copy link
Contributor Author

pitaj commented Apr 30, 2024

Great news! I will soon modify this PR to fully stabilize, rather than leaving F unstable.

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label May 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

@pitaj
Copy link
Contributor Author

pitaj commented May 7, 2024

@rustbot ready

@Amanieu
Copy link
Member

Amanieu commented May 15, 2024

@rfcbot merge

@rfcbot
Copy link

rfcbot commented May 15, 2024

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 15, 2024
@rfcbot
Copy link

rfcbot commented May 15, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@bors
Copy link
Contributor

bors commented May 21, 2024

☔ The latest upstream changes (presumably #125379) made this pull request unmergeable. Please resolve the merge conflicts.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 25, 2024
@rfcbot
Copy link

rfcbot commented May 25, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@dtolnay dtolnay removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label May 25, 2024
@dtolnay
Copy link
Member

dtolnay commented May 25, 2024

The purported conflict was from #125310 moving tests/ui/check-static-values-constraints.stderr and #125483 moving compiler/rustc_const_eval/src/transform/check_consts/ops.rs.

Screenshot from 2024-05-25 10-30-57

There was nothing to resolve; git rebase cleanly handles the changes to moved files.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented May 25, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2024

📌 Commit 4913ab8 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2024
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se May 25, 2024
@joshtriplett joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label May 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121377 (Stabilize `LazyCell` and `LazyLock`)
 - rust-lang#122986 (Fix c_char on AIX)
 - rust-lang#123803 (Fix `VecDeque::shrink_to` UB when `handle_alloc_error` unwinds.)
 - rust-lang#124080 (Some unstable changes to where opaque types get defined)
 - rust-lang#124667 (Stabilize `div_duration`)
 - rust-lang#125472 (tidy: validate LLVM component names in tests)
 - rust-lang#125523 (Exit the process a short time after entering our ctrl-c handler)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 890982e into rust-lang:master May 26, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
Rollup merge of rust-lang#121377 - pitaj:lazy_cell_fn_pointer, r=dtolnay

Stabilize `LazyCell` and `LazyLock`

Closes rust-lang#109736

This stabilizes the [`LazyLock`](https://doc.rust-lang.org/stable/std/sync/struct.LazyLock.html) and [`LazyCell`](https://doc.rust-lang.org/stable/std/cell/struct.LazyCell.html) types:

```rust
static HASHMAP: LazyLock<HashMap<i32, String>> = LazyLock::new(|| {
    println!("initializing");
    let mut m = HashMap::new();
    m.insert(13, "Spica".to_string());
    m.insert(74, "Hoyten".to_string());
    m
});

let lazy: LazyCell<i32> = LazyCell::new(|| {
    println!("initializing");
    92
});
```

r? libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for lazy_cell
10 participants