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

Suggest lazy initialization when calling a non-const function in a static initializer #100410

Closed
jyn514 opened this issue Aug 11, 2022 · 9 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Aug 11, 2022

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=67155cb72a24e0a080d9dee88483fbec

use std::sync::Mutex;

static MUTEX: Mutex<i32> = Mutex::new(1);

fn main() {
    println!("{}", MUTEX.lock().unwrap());
}

The current output is:

error[E0015]: cannot call non-const fn `Mutex::<i32>::new` in statics
 --> src/main.rs:3:28
  |
3 | static MUTEX: Mutex<i32> = Mutex::new(1);
  |                            ^^^^^^^^^^^^^
  |
  = note: calls in statics are limited to constant functions, tuple structs and tuple variants

I have a friend learning rust who concluded, reasonably, that global mutexes are not allowed and you have to declare them in main and pass them down to functions with an argument. It would be great if the compiler could suggest the common alternative approach instead, which is lazy initialization:

Ideally the output should look like:

error[E0015]: cannot call non-const fn `Mutex::<i32>::new` in statics
 --> src/main.rs:3:28
  |
3 | static MUTEX: Mutex<i32> = Mutex::new(1);
  |                            ^^^^^^^^^^^^^
  |
  = note: calls in statics are limited to constant functions, tuple structs and tuple variants
  = help: consider using the `once_cell` crate: https://crates.io/crates/once_cell

I know the compiler generally has a policy of not favoring individual crates, but once_cell is literally becoming part of the standard library so I think this is reasonable and doesn't show favoritism.

(I'm aware Mutex::new will be a const fn in 1.63, but this same diagnostic would be helpful in any other case where the initializer isn't const.)

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Aug 11, 2022
@jyn514
Copy link
Member Author

jyn514 commented Aug 11, 2022

also, in this particular case it could have suggested using AtomicI32 instead; not sure if that's a good suggestion to make though.

@jyn514
Copy link
Member Author

jyn514 commented Aug 11, 2022

Mentoring instructions: add that note immediately after

err.note(&format!(
"calls in {}s are limited to constant functions, \
tuple structs and tuple variants",
ccx.const_kind(),
));

@jyn514 jyn514 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Aug 11, 2022
@cameron1024
Copy link
Contributor

@rustbot claim

@mqudsi
Copy link
Contributor

mqudsi commented Aug 12, 2022

Can't we just punt this until after #74465 lands?

@jyn514
Copy link
Member Author

jyn514 commented Aug 12, 2022

@mqudsi I don't understand why it has to be blocked on that? we can certainly update the message to suggest std::once after that lands, but I don't think it needs to block the suggestion.

@mqudsi
Copy link
Contributor

mqudsi commented Aug 12, 2022

I guess it's just being conservative when evaluating the costs vs benefits?

once_cell (and before it, lazy_static) have been around for a long time and it would have been a great tradeoff to suggest either years ago (cost: suggesting an external crate, benefit: years of people benefiting from it), but if we've not suggested an external crate for this long and a native solution is just around the corner, it feels like it would be prudent to just wait it out at this point (cost: same, benefit: capped to however long it takes for std's equivalent to land).

I'm not a zero-dependency zealot (but I do appreciate the distinction between cargo and rustc), but my concern is that in some ways, this suggestion isn't something we can take back (it's not like we'll emit a diagnostic message to stop using once_cell once std gets support for the same). Every time someone interacts with the rust compiler and gets an officially sanctioned message coming from rustc itself saying "use a 3rd party lib" that could change the reaction from "I need to do this some other way" to "rust can't even do basic things without using a 3rd party lib, and even the compiler knows it." As you mentioned, there are workarounds that don't have dependencies on 3rd party crates, and they're technically more idiomatic even if they're less ergonomic.

I could be mistaken, but I've been following it and I don't think any serious issues have been raised with the core/std port of once_cell and there's no reason to expect the proposal will languish for years to come like some other, more controversial changes; all that just makes me think "let sleeping dogs lie"... but that's just my two cents and I'm obviously not going to complain if this does get implemented - I just thought I'd share my perspective, for what little it is worth.

(I use once_cell myself and even plug it in my own crate's docs.)

@jyn514
Copy link
Member Author

jyn514 commented Aug 12, 2022

but my concern is that in some ways, this suggestion isn't something we can take back (it's not like we'll emit a diagnostic message to stop using once_cell once std gets support for the same). Every time someone interacts with the rust compiler and gets an officially sanctioned message coming from rustc itself saying "use a 3rd party lib" that could change the reaction from "I need to do this some other way" to "rust can't even do basic things without using a 3rd party lib, and even the compiler knows it."

tbh people who feel like that probably already feel that way regardless of what the compiler says

We already suggest async_trait for async functions in traits, I don't think this is that much of a stretch. If std::once is just around the corner like you say, then we won't suggest an external crate for long anyway :)

@mqudsi
Copy link
Contributor

mqudsi commented Aug 12, 2022

Fair enough!

ChrisDenton added a commit to ChrisDenton/rust that referenced this issue Aug 20, 2022
…er-errors

suggest `once_cell::Lazy` for non-const statics

Addresses rust-lang#100410

Some questions:
 - removing the `if` seems to include too many cases (e.g. calls to non-const functions inside a `const fn`), but this code excludes the following case:
```rust
const FOO: Foo = non_const_fn();
```
Should we suggest `once_cell` in this case as well?
 - The original issue mentions suggesting `AtomicI32` instead of `Mutex<i32>`, should this PR address that as well?
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 20, 2022
…er-errors

suggest `once_cell::Lazy` for non-const statics

Addresses rust-lang#100410

Some questions:
 - removing the `if` seems to include too many cases (e.g. calls to non-const functions inside a `const fn`), but this code excludes the following case:
```rust
const FOO: Foo = non_const_fn();
```
Should we suggest `once_cell` in this case as well?
 - The original issue mentions suggesting `AtomicI32` instead of `Mutex<i32>`, should this PR address that as well?
@Noratrieb
Copy link
Member

#100507 fixed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants