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

Implement DerefMut for String #26241

Merged
merged 7 commits into from
Jul 14, 2015
Merged

Conversation

SimonSapin
Copy link
Contributor

@rust-highfive
Copy link
Collaborator

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
Copy link
Contributor Author

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

@bluss
Copy link
Member

bluss commented Jun 12, 2015

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
Copy link
Contributor Author

@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
Copy link
Member

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.

@alexcrichton 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
@SimonSapin
Copy link
Contributor Author

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

@Gankra
Copy link
Contributor

Gankra commented Jun 12, 2015

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

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

bluss commented Jun 13, 2015

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
Copy link
Contributor Author

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
Copy link
Member

aturon commented Jun 16, 2015

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
Copy link
Contributor Author

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
Copy link
Member

aturon commented Jun 16, 2015

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

@alexcrichton 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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

bors commented Jun 18, 2015

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

@SimonSapin
Copy link
Contributor Author

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

@@ -806,6 +814,12 @@ impl str {
core_str::StrExt::split_at(self, mid)
}

/// Divide one mutable string slice into two at an index.
#[inline]
pub fn split_at_mut(&mut self, mid: usize) -> (&mut str, &mut str) {
Copy link
Member

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
Copy link
Member

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!

@alexcrichton 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@alexcrichton
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@bors r=alexcrichton 0375d9c

@bors
Copy link
Contributor

bors commented Jul 13, 2015

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

@SimonSapin
Copy link
Contributor Author

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
Copy link
Contributor Author

I just pushed a rebase.

@pnkfelix
Copy link
Member

@bors r=alexcrichton eb99f0e

@bors
Copy link
Contributor

bors commented Jul 13, 2015

⌛ Testing commit eb99f0e with merge 13aed07...

@bors
Copy link
Contributor

bors commented Jul 13, 2015

💔 Test failed - auto-mac-64-opt

@SimonSapin
Copy link
Contributor Author

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

@alexcrichton
Copy link
Member

@bors: r+ 3226858

@bors
Copy link
Contributor

bors commented Jul 13, 2015

⌛ Testing commit 3226858 with merge dd46cf8...

@bors bors merged commit 3226858 into rust-lang:master Jul 14, 2015
@SimonSapin SimonSapin deleted the derefmut-for-string branch July 18, 2015 19:01
@bluss bluss mentioned this pull request Sep 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

9 participants