Add String<A> type with custom allocator parameter#149328
Conversation
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang This PR modifies Some changes occurred in src/tools/clippy cc @rust-lang/clippy rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
rustbot
added
A-rustc-dev-guide
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
|
maybe r? @Amanieu as the reviewer of the previous PR has more context edit: whoops saw previous reviewer actually was Mark-Simulcrum, Amanieu was just last to leave a review. If it makes sense to swap it back, please do! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
T-release
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #147799) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors
Bot
added
S-waiting-on-author
|
for some reason, when rustdoc is building the reference docs, it thinks |
This comment has been minimized.
This comment has been minimized.
cc @rust-lang/lang-docs |
|
The links are generated directly by rustdoc. If rustdoc is generating the wrong link for something like And if the link for https://doc.rust-lang.org/std/string/struct.String.html is going away, that seems especially concerning since that very common type probably has links to it from all over the internet. |
|
It's strange that rustdoc seems to do this correctly everywhere else, eg when building doc comments, it's only when generating the mdbook for |
Why was this not necessary for Vec? All stable functions that construct a String still force the global allocator to be used, right? That should be enough to guide type inference I would think when combined with a default for the type parameter for when you explicitly write String as type. |
Note that using See also my comments from my attempt at doing this: #101551 (comment) #101551 (comment)
|
This does seem concerning. Is there anything we could do here? E.g., could we use |
|
My attempts at #[cfg(not(doc))]
mod global { struct String<A> { ...} }
#[cfg(not(doc))]
type String = global::String<Global>;
#[cfg(doc)]
struct String<A> { ... }looks like it should work, but fails trying to document methods on impl String {
/// see [`str::is_foo`]
/// [`str::is_foo`]: ../../../primitive.str.html#is_foo
pub fn ...
}specifically, the path needs either One suggestion was to somehow insert a page redirect from Hopefully, I can figure out some |
This comment has been minimized.
This comment has been minimized.
This is a third attempt at implementing adding Allocator support to the std lib's `String`. Still stuck on the same issue with type inference failing on the newly generic `String<A>`, but I opted to do even less than the previous WIP work, and have added no new functions (`String<A>` can be constructed via `Vec<u8, A>` or `Box<str, A>`), and have moved only the struct definition to its own mod to make rebasing a bit easier if/when main changes underneath me.
This adds a diagnostic name `string_in_global` so that clippy can easily check for the alloc::string::String type alias.
added some code in linkchecker to check the generic::String docs when trying to resolve links to alloc::string::String type alias. There's some lazy-loading that the browser does, but linkchecker doesn't, so maybe some general-purpose solution would be better, but this seemed better than a big list of exceptions.
Yes, you could just use `unsafe { from_utf8_unchecked }``, but people get
antsy about `unsafe`, so add some Vec ctor/dtor equivalents.
|
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #156473) made this pull request unmergeable. Please resolve the merge conflicts. |
15 participants
View all comments
This change is part of the
allocator_apifeature set #32838 (also see wg-allocators roadmap or libs-team ACP).The previous attempts at adding an allocator parameter to string are at rust-lang/rust#101551, and rust-lang/rust#79500 (I think those authors should get much of the credit here, I am re-writing what they worked out in those threads).
workaround
There is a type inference ambiguity introduced when adding a generic parameter to a type which previously had none, even when that parameter has a default value (more details in rust-lang/rust#98931). I've done the same workaround as rust-lang/rust#101551, which is to make
alloc::string::Stringa type alias toString<Global>, but I've arranged the modules a bit differently to make rebase/merges a bit easier.This workaround unfortunately changes the type name of the
Stringlanguage item, and that would be user-facing in error or diagnostic output. I understand from this comment that this change is acceptable.changes to existing API
Most of the methods on the original
Stringhave been implemented for the generic version instead. I don't foresee anything more moving fromString<Global>toString<A>, as the remaining methods are all constructors which implicitly use theGlobalallocator.There are three general types of changes:
impl Foo for Stringtoimpl<A: Allocator> Foo for String<A>Vec<u8, A>andBox<str, A>: here we can use the existing allocator from those types.String<A>,String<Global>, orString<impl Allocator + Default>, etc; in general I try to leave these out of this change, but where some analogous change was made toVec<T, A>I follow that.new methods
Some methods have been added to
String<A>which are not strictly necessary to land this change, but are considered helpful enough to users, and close enough to existing precedent inVec<T, A>. Specifically, 4 new constructors (new_in,with_capacity_in,try_with_capacity_in,from_raw_parts_in), 1 destructor (into_raw_parts_with_alloc), and 1 getter (allocator). Technically, the updatedfrom_utf8_uncheckedshould be sufficient for constructing, but we can add some safe constructors so users don't have to sully themselves.not implemented
Variants of
from_utf{8,16}*which internally allocate or useCowhave been punted on this PR, maybe a followup would make sense to either rewrite them, or add some*_invariant.