Implement DerefMut for String #26241

SimonSapin · 2015-06-12T09:22:06Z

rust-highfive · 2015-06-12T09:22:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

SimonSapin · 2015-06-12T09:40:08Z

Is it also time to look at either deprecating or stabilizing OwnedAsciiExt and make_ascii_*case?

bluss · 2015-06-12T12:35:19Z

Those casing ascii methods are pretty broken in unicode context, but I guess we could allow arbitrary ascii -> ascii mappings. Since a grapheme can be multiple codepoints, this also edits / transforms graphemes that use at least one ascii codepoint, for example é (depending on normalization).

SimonSapin · 2015-06-12T13:22:16Z

@bluss Agreed, the AsciiExt methods are for very narrow use case like CSS property names (which are entirely within ASCII), which is why they’re not inherent or in the prelude. We also have to_lowercase and to_uppercase on char and str that do use the full Unicode mappings.

alexcrichton · 2015-06-12T17:15:38Z

I would be more uncomfortable with stabilizing make_ascii_{upper,lower}case (primarily in terms of naming), but I'm ok with this. I'm going to tag this with T-libs and I-needs-decision to ensure it gets some visibility.

SimonSapin · 2015-06-12T17:36:52Z

OK. make_ascii_*case is a motivation for this change, but changing or stabilising would be a separate decision.

Gankra · 2015-06-12T17:38:47Z

I say we just land DerefMut and call it a day. Other stuff is easy to impl outside std.

bluss · 2015-06-13T11:01:18Z

I don't want to complicate this issue, but we might want to add slicing of &mut str and .split_at_mut(mid), the latter being an important crutch for slices already to avoid needing unsafe sections.

SimonSapin · 2015-06-15T16:41:48Z

Added a commit:

Implement IndexMut for String and str.

... matching the existing Index impls.
There is no reason not to if String implement DerefMut.

The code removed in src/librustc/middle/effect.rs was added in #9750
to prevent things like s[0] = 0x80 where s: String,
but I belive became unnecessary when the Index(Mut) traits were introduced.

aturon · 2015-06-16T17:25:16Z

I'm 👍 on this change in general, but are we OK landing this insta-stable? (Since it's just filling out APIs according to vetted/stable convention, seems fine to me, but I wanted to check.)

SimonSapin · 2015-06-16T17:34:42Z

str::split_at_mut is unstable (like the existing str::split_at) but yes, the rest (DerefMut and IndexMut impls as well as str::slice_mut_unchecked) is insta-stable in the current form of this PR. I don’t mind changing it.

aturon · 2015-06-16T17:36:16Z

Hm, I was also forgetting that implementations of stable traits are instantly stable regardless. I'm happy with the PR as it stands.

alexcrichton · 2015-06-16T21:59:35Z

This PR is now entering its final comment period. There was some discussion at triage today about this and there was some hesitation about the power that DerefMut is adding, so we decided to FCP it instead of merging it immediately!

SimonSapin · 2015-06-16T22:43:41Z

After seeing #9750 again I’ve been pondering on whether this PR introduces a way to break the UTF-8 invariant in safe code. I believe it does not:

With x: &mut T, *T can only be assigned to directly when T is sized. Since str: !Sized, &mut str can not be assigned to directly.
The IndexMut impls in this PR all have Target=str, so from a &mut str they can only give another &mut str.
Although Index<usize, Target=u8> for str would make sense (and did sort of exist in Rust once upon a time: Disallow modifications of strings #9750) but is not implemented. The corresponding IndexMut would allow string mutation, but it’s not implemented either.
The above leaves as the only way to mutate &mut str from safe code using safe functions/methods like AsciiExt::make_ascii_lowercase that use unsafe code (such as transmute::<&mut str, &mut [u8]>) internally. It’s up to these functions/methods to preserve the invariant in the safe abstraction they provide.

bors · 2015-06-18T21:35:41Z

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

SimonSapin · 2015-06-19T00:03:48Z

I’ll rebase once a decision is made on what to include or not.

alexcrichton · 2015-07-01T02:17:40Z

src/libcollections/str.rs

Can you tag this with an explicit unstable tag?

alexcrichton · 2015-07-01T02:18:19Z

The consensus of the libs team is to move ahead with this PR, with a rebase and a few nits addressed this should be good to go, thanks @SimonSapin!

SimonSapin · 2015-07-01T05:47:03Z

Rebased and added a commit to address comments. This commits also adds #[unstable] to str::split_at, which existed before this PR but did not have any stability marker.

I’m getting an LLVM assertion error when building locally :( Could this be specific to stage1? I haven’t tried stage2 yet.

../../projects/rust/src/libstd/lib.rs:1:1: 1:1 note: expansion site rustc: ../../../../../../../projects/rust/src/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp:525: bool isUnsignedDIType(llvm::DwarfDebug*, const llvm::DIType*): Assertion `T == dwarf::DW_TAG_typedef || T == dwarf::DW_TAG_const_type || T == dwarf::DW_TAG_volatile_type || T == dwarf::DW_TAG_restrict_type || T == dwarf::DW_TAG_enumeration_type' failed. /home/simon/projects/rust/mk/tests.mk:394: recipe for target 'x86_64-unknown-linux-gnu/stage1/test/stdtest-x86_64-unknown-linux-gnu' failed make: *** [x86_64-unknown-linux-gnu/stage1/test/stdtest-x86_64-unknown-linux-gnu] Aborted (core dumped)

SimonSapin · 2015-07-07T13:58:03Z

I’m still getting the same LLVM error after rebasing on today’s Rust master. Any suggestion what to do here?

alexcrichton · 2015-07-07T21:12:32Z

Unfortunately I don't know how to get past that error.

`&mut str` is rarely useful, but it is for e.g. `AsciiExt::make_ascii_lowercase`.

... matching the existing Index impls. There is no reason not to if String implement DerefMut. The code removed in `src/librustc/middle/effect.rs` was added in rust-lang#9750 to prevent things like `s[0] = 0x80` where `s: String`, but I belive became unnecessary when the Index(Mut) traits were introduced.

pnkfelix · 2015-07-13T14:55:24Z

This seems like it has been blocked for 6 days due to #26484. However this is unnecessary, assuming it does not add any new breakage to --enable-debug --enable-optimize, since we have no bot that tests that configuration (see also #27010).

pnkfelix · 2015-07-13T14:56:21Z

After discussion with @SimonSapin I am interpreting this statement from @alexcrichton 👍

The consensus of the libs team is to move ahead with this PR, with a rebase and a few nits addressed this should be good to go, thanks @SimonSapin!

as an implicit r=me for the PR.

pnkfelix · 2015-07-13T14:56:46Z

@bors r=alexcrichton 0375d9c

bors · 2015-07-13T14:56:49Z

🙀 0375d9c is not a valid commit SHA. Please try again with 23daca3.

SimonSapin · 2015-07-13T14:58:32Z

doener on IRC suggested that I might be hitting #26484. After running configure again with default flags (I’m not sure what I had before), make check-stage1-std succeeded locally.

SimonSapin · 2015-07-13T15:08:34Z

I just pushed a rebase.

pnkfelix · 2015-07-13T15:31:51Z

@bors r=alexcrichton eb99f0e

bors · 2015-07-13T15:46:57Z

⌛ Testing commit eb99f0e with merge 13aed07...

bors · 2015-07-13T16:31:04Z

💔 Test failed - auto-mac-64-opt

SimonSapin · 2015-07-13T21:30:08Z

@alexcrichton With that last commit make check-stage1 (not just check-stage1-std) passes locally.

alexcrichton · 2015-07-13T22:51:21Z

@bors: r+ 3226858

bors · 2015-07-13T23:47:07Z

⌛ Testing commit 3226858 with merge dd46cf8...

See rust-lang/rfcs#1157

bors · 2015-07-14T01:19:19Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

rust-highfive assigned aturon Jun 12, 2015

alexcrichton added I-needs-decision Issue: In need of a decision. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 12, 2015

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 12, 2015

SimonSapin force-pushed the derefmut-for-string branch from a61076a to ef8ac71 Compare June 15, 2015 16:42

alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-needs-decision Issue: In need of a decision. labels Jun 16, 2015

alexcrichton reviewed Jul 1, 2015
View reviewed changes

src/libcollections/str.rs Outdated

Copy link

Member

alexcrichton Jul 1, 2015

Choose a reason for hiding this comment

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

Can you tag this with an explicit unstable tag?

alexcrichton removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 1, 2015

SimonSapin force-pushed the derefmut-for-string branch from 285feb6 to 28357e5 Compare July 1, 2015 05:44

SimonSapin force-pushed the derefmut-for-string branch from 28357e5 to 23daca3 Compare July 1, 2015 05:48

SimonSapin added 6 commits July 13, 2015 16:21

Implement DerefMut for String

2d34a53

`&mut str` is rarely useful, but it is for e.g. `AsciiExt::make_ascii_lowercase`.

Add tests for AsciiExt::make_ascii_*case, including on String.

304f545

Add more tests for AsciiExt::is_ascii

90d61d8

Add str::split_at_mut

7469914

Mark some new things as unstable.

eb99f0e

SimonSapin force-pushed the derefmut-for-string branch from 23daca3 to eb99f0e Compare July 13, 2015 15:06

Fix tests for changes in rust-lang#26241.

3226858

SimonSapin force-pushed the derefmut-for-string branch from 88fe063 to 3226858 Compare July 13, 2015 21:29

bors added a commit that referenced this pull request Jul 13, 2015

Auto merge of #26241 - SimonSapin:derefmut-for-string, r=alexcrichton

dd46cf8

See rust-lang/rfcs#1157

bors merged commit 3226858 into rust-lang:master Jul 14, 2015

bors mentioned this pull request Jul 14, 2015

Document Unicode complications when iterating "characters" #27012

Merged

SimonSapin deleted the derefmut-for-string branch July 18, 2015 19:01

bluss mentioned this pull request Sep 15, 2015

1.3 relnotes #28408

Merged

Implement DerefMut for String #26241

Implement DerefMut for String #26241

Uh oh!

Conversation

SimonSapin commented Jun 12, 2015

rust-highfive commented Jun 12, 2015

SimonSapin commented Jun 12, 2015

bluss commented Jun 12, 2015

SimonSapin commented Jun 12, 2015

alexcrichton commented Jun 12, 2015

SimonSapin commented Jun 12, 2015

Gankra commented Jun 12, 2015

bluss commented Jun 13, 2015

SimonSapin commented Jun 15, 2015

aturon commented Jun 16, 2015

SimonSapin commented Jun 16, 2015

aturon commented Jun 16, 2015

alexcrichton commented Jun 16, 2015

SimonSapin commented Jun 16, 2015

bors commented Jun 18, 2015

SimonSapin commented Jun 19, 2015

alexcrichton Jul 1, 2015

Choose a reason for hiding this comment

alexcrichton commented Jul 1, 2015

SimonSapin commented Jul 1, 2015

SimonSapin commented Jul 7, 2015

alexcrichton commented Jul 7, 2015

pnkfelix commented Jul 13, 2015

pnkfelix commented Jul 13, 2015

pnkfelix commented Jul 13, 2015

bors commented Jul 13, 2015

SimonSapin commented Jul 13, 2015

SimonSapin commented Jul 13, 2015

pnkfelix commented Jul 13, 2015

bors commented Jul 13, 2015

bors commented Jul 13, 2015

SimonSapin commented Jul 13, 2015

alexcrichton commented Jul 13, 2015

bors commented Jul 13, 2015

bors commented Jul 14, 2015

Labels

9 participants