Repository navigation
Deprecate internal function vec::from_elem - #164653
Conversation
|
r? @joboet rustbot has assigned @joboet. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| #[cfg(not(no_global_oom_handling))] | ||
| #[unstable(feature = "liballoc_internals", issue = "none")] | ||
| #[rustc_diagnostic_item = "vec_from_elem"] | ||
| pub fn from_elem_global<T: Clone>(elem: T, n: usize) -> Vec<T> { |
There was a problem hiding this comment.
This might be overkill but I duplicated from_elem so the the standard library can fully act as if it's unstable. And it does avoid needing to allow deprecated.
There was a problem hiding this comment.
Yeah, I dunno if there's an allow_internal_deprecated like allow_internal_unstable has.
There was a problem hiding this comment.
We can hopefully use Vec::from_elem (perhaps named differently) for that soon (see rust-lang/libs-team#912)
There was a problem hiding this comment.
I know it's internal but the name still doesn't seem that great (what is "global" about?). Maybe from_repeated_elem or so?
There was a problem hiding this comment.
This has one use: in the vec! macro. If someone does not know (or can't find out) what global means in the context of allocating a Vec then they really should not be touching the macro at all. And I would really prefer its name not to diverge too far from its diagnostic item name. Especially not in this PR.
Btw, the best place to bikeshed this would be rust-lang/libs-team#912 since it proposes a stable API (though personally I'm not in favour of having a guaranteed specialisation wrapper before specialisation is stable).
|
@rfcbot merge libs Since this is technically an API change, even though this API should have been perma-unstable initially. |
|
@clarfonthey 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! cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns. |
4f09f80 to
07daf37
Compare
|
r=me when FCP completes. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
| #[cfg(not(no_global_oom_handling))] | ||
| #[stable(feature = "rust1", since = "1.0.0")] | ||
| #[rustc_diagnostic_item = "vec_from_elem"] | ||
| #[deprecated(since = "1.0.0", note = "use the `vec!` macro instead")] |
There was a problem hiding this comment.
It does not seem correct to say that this was deprecated since Rust 1.0? This should be CURRENT_RUST_VERSION I think.
There was a problem hiding this comment.
So, it was an internal detail since 1.0, it's only that we never prevented its use.
There was a problem hiding this comment.
I think that it should be CURRENT_RUST_VERSION, so people don't get confused about "why didn't I get a warning earlier?", and so they can figure out how "bad" the deprecation is based on recency.
There was a problem hiding this comment.
Aside from adding the literal attribute, this isn't a recent deprecation. It's been deprecated for as long as it has existed. It was always wrong to use it (and we're only talking about a few crates that ever did).
People thinking it's less bad to use it since it's "only a recent deprecation" is exactly what I'd like to avoid.
There was a problem hiding this comment.
I think it's reasonable to tie the version number to when the attribute was added. It's quite confusing otherwise if code previously was warning free, and now suddenly gets a warning effectively claiming that we warned against this since 1.0.
There was a problem hiding this comment.
I will change because I've already spent way too much time arguing about a warning approximately nobody will ever see. However, I do think it's a bad mistake to treat this at all like a publicly documented function. The #[deprecated] lint is being used out of convenience and I strongly disagree that it should list the version the warning was added. That is not at all actionable information.
There was a problem hiding this comment.
Its previous state is more like what we usually call "soft deprecation".
There was a problem hiding this comment.
Strictly speaking, do we even need to put anything in since? It's technically optional...
There was a problem hiding this comment.
I think that putting in a note why we marked this as deprecated since 1.0 is enough to clear any concerns, but I'm not going to complain too hard.
We could also add a dedicated FCW that gets reported for dependencies too, which would be stronger than a deprecation but get the point across better.
There was a problem hiding this comment.
I will change because I've already spent way too much time arguing about a warning approximately nobody will ever see. However, I do think it's a bad mistake to treat this at all like a publicly documented function. The
#[deprecated]lint is being used out of convenience and I strongly disagree that it should list the version the warning was added. That is not at all actionable information.
For what it's worth, I think 1.0 was probably the right choice: it reflects that this wasn't ever supposed to be usable, which is what users should be caring about. Why would a user ever care about having the error message show the deprecated attribute was added? Out lints don't say "lint emitted since 1.xx". If they're checking with two Rust versions then they've figured it out, if they care more than that then look at the blame 🤷♂️.
But agreed, it's not worth a battle.
07daf37 to
458cde8
Compare
|
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. |
|
@bors r=joboet rollup |
…m, r=joboet Deprecate internal function `vec::from_elem` This is only used internally and is only public because we didn't used to have the ability to gate internal functions used in public macros (other than using `doc(hidden)` to hide it).
|
|
This pull request was unapproved. |
|
Why would we add UI test that uses |
458cde8 to
ddfc252
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Let's try again @bors r=joboet rollup |
|
That is unbelievable timing, but it seems like they may actually want |
|
Hm, if the intent is to test the UI of both |
Rollup of 15 pull requests Successful merges: - #163477 (Fancy paramenv accessors that communicate the access pattern better.) - #160593 (turn ABI-controlling target features into a target modifier) - #164896 (offload: pass the linked device image to the host pass) - #165468 (Remove return type from `BuilderMethods::store`) - #164564 (Re-enable outline-atomics by default on more AArch64 platforms) - #164623 (add next-solver regression tests for two fixed ices) - #164653 (Deprecate internal function `vec::from_elem`) - #164678 (add next-solver regression test for recursive opaque ICE under validate-mir) - #165387 (windows OpenOptionsExt examples: replace winapi by windows crate) - #165429 (Add tests for `-Zcrate-attr` edition and position) - #165430 ([tiny] Show context for found assoc item in E0107) - #165456 (std: Fix build for the L4Re targets) - #165463 (Add BTreeMap::allocator and BTreeSet::allocator) - #165466 (Don't "exit" using `panic` on teeos) - #165491 (Remove unused compiler types and associated helpers)
Rollup merge of #164653 - ChrisDenton:deprecate-vec-from_elem, r=joboet Deprecate internal function `vec::from_elem` This is only used internally and is only public because we didn't used to have the ability to gate internal functions used in public macros (other than using `doc(hidden)` to hide it).
View all comments
This is only used internally and is only public because we didn't used to have the ability to gate internal functions used in public macros (other than using
doc(hidden)to hide it).