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 String::leak #109814

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Stabilize String::leak #109814

merged 1 commit into from
Jun 14, 2023

Conversation

est31
Copy link
Member

@est31 est31 commented Mar 31, 2023

Stabilizes the following API:

impl String {
    pub fn leak(self) -> &'static mut str;
}

closes #102929

blocked by having an FCP for stabilization.

@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 31, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@est31
Copy link
Member Author

est31 commented Mar 31, 2023

Stabilization proposal

I propose stabilizing the following API:

impl String {
    pub fn leak(self) -> &'static mut str;
}

The ACP was in rust-lang/libs-team#109 .

Implementation history

It was implemented by #103280 . An earlier attempt was at #102941.

Design considerations

The added function is similar to already existing Vec::leak, and Box::leak. Those functions are generic on the lifetime however while this function always returns 'static references. This is due to two reasons:

  • Vec and Box are generic containers and can only live as long as their contained type lives. If that type is not : 'static, then obviously the returned reference can't be either.
  • Same goes for allocators, as Box is also generic on the allocator implementation, and the returned data can only live as long as the allocator does.

Strings are always over bytes/chars so the first point isn't a concern for us, nor are allocators one because Strings don't implement custom allocators (yet). This explains the difference why we return a 'static reference here.

If/when we add an explicit allocator to String, then in order for the String::leak function to be sound, it has to make the returned lifetime dependent on the lifetime on the allocator, and thus will require an explicit lifetime parameter similar to how there is one for Box::leak or Vec::leak. The breakage caused by the addition of such a parameter should be very minimal however, and be dwarved by the breakage caused by adding a type parameter to the String type.

cc @finnbear @dtolnay as you were involved in the original ACP.

@Noratrieb
Copy link
Member

Noratrieb commented Mar 31, 2023

nor are allocators one because Strings don't implement custom allocators (yet)

Does this API make them impossible in the future?

I think it doesn't as going to a unbounded lifetime should be backwards compatible but it would be good to mention that here more explicitly.

@est31
Copy link
Member Author

est31 commented Mar 31, 2023

nor are allocators one because Strings don't implement custom allocators (yet)

Does this API make them impossible in the future?

@Nilstrieb I hope not. I made my comment with the assumption there isn't. The ACP had a generic lifetime and the PR had a 'static one but I couldn't find any discussion towards around this so I mentioned it here. IF adding a lifetime parameter is semver breaking, then now is an excellent opportunity to change it. The cargo book doesn't list lifetime parameter additions for functions but even for type parameter additions it says that adding such parameters isn't an issue in general.

@est31
Copy link
Member Author

est31 commented Apr 1, 2023

@rustbot label +T-libs-api -T-libs

@rustbot rustbot 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 Apr 1, 2023
@est31
Copy link
Member Author

est31 commented Apr 3, 2023

@Nilstrieb I've added a paragraph about the breakage concerns of explicit lifetime parameters.

@finnbear finnbear mentioned this pull request Apr 3, 2023
4 tasks
@est31
Copy link
Member Author

est31 commented Apr 14, 2023

@m-ou-se friendly ping! It would be nice to have an FCP for stabilization. A stabilization proposal can be found above.

@jyn514 jyn514 added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Apr 26, 2023
@est31
Copy link
Member Author

est31 commented Apr 29, 2023

r? libs-api

@rustbot rustbot assigned Amanieu and unassigned m-ou-se Apr 29, 2023
@dtolnay
Copy link
Member

dtolnay commented Apr 29, 2023

@rust-lang/libs-api:
@rfcbot fcp merge

Stabilization report: #109814 (comment)

@rfcbot
Copy link

rfcbot commented Apr 29, 2023

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

Concerns:

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. labels Apr 29, 2023
@dtolnay dtolnay assigned dtolnay and unassigned Amanieu Apr 29, 2023
@BurntSushi
Copy link
Member

Is it easy to just use an unbounded lifetime here and avoid the question of whether adding it later is a breaking change?

@Amanieu
Copy link
Member

Amanieu commented May 16, 2023

Yes, this should return an unbounded lifetime instead of &'static mut. Later when allocator support is added the lifetime can be bound to the lifetime of the allocator.

@rfcbot concern unbounded lifetime

@est31
Copy link
Member Author

est31 commented May 27, 2023

@BurntSushi @m-ou-se @joshtriplett could you maybe take a look at the FCP above? Also, it would be great for someone to review #111656, it should address @Amanieu and @BurntSushi 's concerns. I have asked for more docs in that PR but I can also just add them to this one once #111656 is merged.

@Amanieu
Copy link
Member

Amanieu commented May 27, 2023

Consider the author of #111656 hasn't responded in over a week, I think it's fine to make the doc changes here.

@Amanieu
Copy link
Member

Amanieu commented May 27, 2023

@rfcbot resolve unbounded lifetime

@finnbear
Copy link
Contributor

Consider the author of #111656 hasn't responded in over a week, I think it's fine to make the doc changes here.

Apologies for forgetting to respond. I think the wording "This is mainly useful for data that lives for the remainder of the program's life." and the &'static reference in the example imply and show, respectively, the possibility of &'static references. However, I would welcome any improvements to the wording in a future PR.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 27, 2023
…etime, r=Amanieu

Use an unbounded lifetime in `String::leak`.

Using `'a` instead of `'static` is predicted to make the process of making `String` generic over an allocator easier/less of a breaking change.

See:
- rust-lang#109814 (comment)
- rust-lang#109814 (comment)

ACP: rust-lang/libs-team#109
@Amanieu
Copy link
Member

Amanieu commented May 28, 2023

I'd just add something like this similar to what Vec::leak has:

This lifetime may be chosen to be 'static.

@est31
Copy link
Member Author

est31 commented May 28, 2023

Re improving docs here vs in #111656, I guess given that I have a PR for String::leak open already (this one), it's easiest for me to just modify my PR, especially as it was me who brought up the concern. I have pushed a proposed wording:

The caller has free choice over the returned lifetime, including `'static`.

The reason why I asked for docs is that when I looked up the docs for Box::leak, I wondered if I could make a function return a 'static reference and it helped me a lot that it was mentioned not just in the example but also in the text. Ideally, the more important information doesn't just occur once in docs but multiple times.

@rfcbot rfcbot added 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 Jun 4, 2023
@rfcbot
Copy link

rfcbot commented Jun 4, 2023

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

@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 Jun 14, 2023
@rfcbot
Copy link

rfcbot commented Jun 14, 2023

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.

@Amanieu
Copy link
Member

Amanieu commented Jun 14, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 14, 2023

📌 Commit 3ab0d90 has been approved by Amanieu

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 Jun 14, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#98202 (Implement `TryFrom<&OsStr>` for `&str`)
 - rust-lang#107619 (Specify behavior of HashSet::insert)
 - rust-lang#109814 (Stabilize String::leak)
 - rust-lang#111974 (Update runtime guarantee for `select_nth_unstable`)
 - rust-lang#112109 (Don't print unsupported split-debuginfo modes with `-Zunstable-options`)
 - rust-lang#112506 (Properly check associated consts for infer placeholders)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b8f71ea into rust-lang:master Jun 14, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 14, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 15, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Jul 18, 2023
…Amanieu

Use an unbounded lifetime in `String::leak`.

Using `'a` instead of `'static` is predicted to make the process of making `String` generic over an allocator easier/less of a breaking change.

See:
- rust-lang/rust#109814 (comment)
- rust-lang/rust#109814 (comment)

ACP: rust-lang/libs-team#109
joseluis added a commit to andamira/textos that referenced this pull request Aug 29, 2023
- misc. updates.

----
# improve unsafe

- add #Safety section to unsafe functions
- add unsafe sub-features, like in devela THINK

-----------
- Add some pixel fonts a short selection, and option to import
  - (receiving some kind of slice, typed or interpreted
    - (array from one type to another as long as they both implement X
    - (this pattern can be reused throughout all libera

---
- implement From/To Char enum from devela (TODO)

---

- [ ] bring functions from devela
  - strcount
  - buffer fmt
  - strings …

----

- WIP TRY add TryFrom/ From?
  - DESIGN Decide whether to fail, (TryFrom) or return From converting up until capacity.

------
- add new search methods using memchr

----
- add new NulTerminatedString types
  - crate
  - https://github.com/CAD97/cstr8/

- take ideas from other crates
  - https://crates.io/crates/fstr (conversion of just the right size?)

-----

update Egc

- [ ] impl TryFrom Egc (using string) for both static
- … ops

----------

impl From<CharXX> for given string type aliases.

----
update unicode strings

- new type `StaticNonSpecificString`.
- make `StaticNonNulString` a type alias of `StaticNonSpecificString`.

- [ ] TODO generalize strings even more:
  - `StaticNonSpecificString<const CHAR: &str, const CAP: usize> {}`
    - specialize StaticNonNulString as a type alias:
      `pub type StaticNonNulString<const CAP: usize> = StaticNonSpecificString<'\x00', CAP>;`

----
- pop_egc
- push_egc (convenience)

- push_utf8_bytes (from a byte slice) (impl from functions from_utf8?
- pop utf8_bytes?

------
- graphemes? &[egc] ?

----
## IDEAS

chars:

- [ ] new constructor for `Chars` trait and for concrete types.

- add TryFrom<T: Chars>? or as a methods in chars? try_to_XXX

- new char types e.g.: CharMath, CharBoxDrawing
  - those would be... enums :)
    - macro to generate an enum from a list of correspondences.

- [ ] TODO: impl partialEq & partialOrd between all 4 char types

- [ ] TODO: add more constants?
  - [ ] NUL

strings:

- add more alias string sizes? 96 (32×3), 192 (64×3) 384 (128 * 3)

- [ ] MAYBE make counter_string a CounterString trait
  - [ ] impl over String, str, StaticStringU8, StaticNonNullString.

- add non-unicode strings (sixbit?) less bit? with custom translation
  using custom tables. (link to a slice of the specific const-generic size)
- generalize over unicode strings and non-unicode strings… single trait?

------
- TODO: add more methods?
    - escape_unicode
    - escape_debug
    - escape_default

- wrap fns that returns iterators: to_lowercase, to_uppercase
  - https://doc.rust-lang.org/nightly/std/primitive.char.html#method.to_lowercase

----
- make tests, and examples.
- impl push_str & try_push_str & try_push_str_complete for StaticStringU8

----

add indent format

- add own `Align` enum, & impl from fmt::Alignmnent, instead of using that.

-------

- TextCell
- TextGrid (via ladata/Grid2?) to libera type grid2d?? (recuperar cuadra como grid2d?
- TextLine (rows, or
- TextRope
- traits
  - Ascii
  - Utf8
  - Utf16
- struct Str<const BYTES: usize> {} // owns its buffer
-

---

alignment functions WIP examples

- think about using cow

-----
- THINK
  - no_std functions (how to group them)
  - a trait over String?

-------

- THINK alternatives
  - dirty utf versions?
  - stateful struct to customize these things?

-------
## improve box drawing chars

- separate lines
  - add dotted
  - add half
  - add thin to heavy
  - eight vertical & horizontal lines (U 15)
  - diagonal lines (U15)

- add block elements
  - shades
    - half block shades
  - quadrants
  - sextants
  - eight vertical & horizontal blocks
  - eight vertical & horizontal blocks upper & right (u15)
  - corners
  - diagonal blocks (U15)

- add a binary encoding

----

-----
## alt. unicode-width
- add an opinionated wrapper over unicode-width
  - maybe create traits depending on UnicodeWidthStr / UnicodeWidthChar
    - save custom tables for special symbols
    - for the rest of symbols, derive to unicode-width
  - deal with >2 width chars
  - deal with https://github.com/unicode-rs/unicode-width/issues/

----

- From rational to select unicode char on filling

--------------

I want to provide scaffolding for apunta bin + revela lib)
a text buffer that refreshes, that can be analized, strings positioned…
areas detected, layers... analisys at certain frequency / Hz

---

- refactor cargo manifest

----
# functions beneficial to implement in a Rust library for text manipulation

- https://chat.openai.com/c/17b13959-8073-42ef-bcf4-0b9ef59eb97c

-----------
# rope
- example crop

alternatives
- https://docs.rs/crop
- https://docs.rs/jumprope
- https://docs.rs/ropey/*/ropey/struct.Rope.html
- https://docs.rs/any-rope/*/any_rope

----

# TODO
- unicode columns based on width + override specially long characters
  - (perfect ad-hoc hash-map associated)
  - https://github.com/AldaronLau/unicode-columns/blob/stable/src/lib.rs

-----

# UNICODE BLOCKS
- https://docs.rs/unicode-blocks/0.1.5/unicode_blocks/struct.UnicodeBlock.html

- `block` module. re-export crate.
- desired API:
  'a'.block() -> UnicodeBlock
  UnicodeBlock.name()
  UnicodeBlock.start()
  UnicodeBlock.end()
  UnicodeBlock.contains('a')

------

# UNICODE SEGMENTATION
- https://unicode-rs.github.io/unicode-segmentation/unicode_segmentation/index.html

------

# mixed blitter
- dankamongmen/notcurses#1223

----

add collections of unicode characters with metadata

- allow to support pixel ttf fonts, etc...

- e.g.: dots: … ‥ . · etc…

codepages
- https://crates.io/crates/codepage-strings
- https://crates.io/crates/oem-cp

# IDEAS
- recreate sixbit, in a more general way… choosing bw 64 & 128-bit codepages
  - https://crates.io/crates/sixbit/

--------
- no_std formatting?
  - https://doc.rust-lang.org/core/macro.format_args.html
  - https://doc.rust-lang.org/core/fmt/struct.Arguments.html

--------
- ASCII utilities
  - see: https://doc.rust-lang.org/std/ascii/trait.AsciiExt.html#tymethod.eq_ignore_ascii_case

---
- CASE utilities
  - https://stackoverflow.com/questions/38406793/why-is-capitalizing-the-first-letter-of-a-string-so-convoluted-in-rust

# ISSUES
- "Available on non-crate feature `safe` only."
  rust-lang/rust#43781 (comment)

## FONTS (TODO move to `trazas` crate?)
- https://news.ycombinator.com/item?id=34689213

# LEARN FFI cstring, etc.
- https://www.reddit.com/r/rust/comments/s3x1e3/const_const_u8/
  - u8 is always 8-bits, but it's a mistake to use u8 rather than std::os::raw::c_char

# LEARN unicode
- https://unicode.org/glossary/#byte_order_mark
- https://en.wikipedia.org/wiki/Homoglyph#Unicode_homoglyphs

# CRATES

- https://github.com/bcmyers/num-format (also for numera, ladata…)
- > https://docs.rs/memchr/2.5.0/memchr/

- https://crates.io/crates/texcore
- https://crates.io/crates/tectonic  :) a complete TeX/LaTeX engine

- https://crates.io/crates/character-set (for inspiration) (source not in github)
  - https://docs.rs/crate/character-set/0.4.0/source/src/builtin/general_category.rs

# WAITING for
- https://doc.rust-lang.org/nightly/core/ascii/enum.Char.html
  - rust-lang/rust#110998

- rust-lang/rust#109814 string.leak for 1.72.0

## DISCARDED IDEAS

- [ ] remove Char types, move to devela?
  - better not, because I need good trait coupling…
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. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue for String::leak