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.

Sign up for GitHub

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

Jump to bottom

Add as_str() to string::Drain. #76525

Merged
merged 6 commits into from
Sep 19, 2020
Merged

Add as_str() to string::Drain. #76525

merged 6 commits into from
Sep 19, 2020

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Sep 9, 2020

Vec's Drain recently had its .as_slice() stabilized, but String's Drain was still missing the analogous .as_str(). This adds that.

Also improves the Debug implementation, which now shows the remaining data instead of just "Drain { .. }".

@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2020
@kennytm
Copy link
Member

kennytm commented Sep 9, 2020

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned kennytm Sep 9, 2020
@kennytm
Copy link
Member

kennytm commented Sep 9, 2020 •
edited
Loading

🤔 Actually those two AsRef impls are insta-stable

@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 9, 2020
@m-ou-se
Mark AsRef impls for String's Drain as stable.
f2a3290 
Trait implementations effectively can't be #[unstable].
@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 9, 2020

thinking Actually those two AsRef impls are insta-stable

Ah, I was unaware of this unstable-trait-impl problem. That's unfortunate. Marked them as stable for now.

@kennytm kennytm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 9, 2020
@kennytm
Copy link
Member

kennytm commented Sep 9, 2020 •
edited
Loading

Or perhaps you could skip the AsRef impls for now so we don't need to invoke FCP on this (similar to the treatment in #58924 → #72584)

@kennytm kennytm removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 9, 2020
@m-ou-se
Disable AsRef implementations for String's Drain.
829019d 
Since trait implementations cannot be unstable, we should only add them
when the as_str feature gets stabilized. Until then, only `.as_str()` is
available (behind a feature gate).
@m-ou-se m-ou-se mentioned this pull request Sep 9, 2020
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 12, 2020
@Dylan-DPC
Rollup merge of rust-lang#76538 - fusion-engineering-forks:check-usel…
a493e97 
…ess-unstable-trait-impl, r=lcnr

Warn for #[unstable] on trait impls when it has no effect.

Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436)

This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning:

```
warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information.
   --> library/std/src/panic.rs:235:1
    |
235 | #[unstable(feature = "integer_atomics", issue = "32976")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

---

It detects three problems in the existing code:

1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236
2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397
3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37

Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2020
@bors
Auto merge of rust-lang#76538 - fusion-engineering-forks:check-useles…
9891908 
…s-unstable-trait-impl, r=lcnr

Warn for #[unstable] on trait impls when it has no effect.

Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436)

This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning:

```
warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information.
   --> library/std/src/panic.rs:235:1
    |
235 | #[unstable(feature = "integer_atomics", issue = "32976")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

---

It detects three problems in the existing code:

1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236
2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397
3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37

Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.
dtolnay
dtolnay approved these changes Sep 19, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. Please file a tracking issue for the new method and mention the pending AsRef impls.

@m-ou-se m-ou-se mentioned this pull request Sep 19, 2020
Closed
@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2020

📌 Commit 15eb638 has been approved by dtolnay

@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 Sep 19, 2020
@kennytm kennytm changed the title Add as_str() and AsRef to string::Drain. Add as_str() to string::Drain. Sep 19, 2020
@RalfJung RalfJung mentioned this pull request Sep 19, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2020
@bors
Auto merge of rust-lang#76912 - RalfJung:rollup-q9ur56h, r=RalfJung
8e9d5db 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#73963 (deny(unsafe_op_in_unsafe_fn) in libstd/path.rs)
 - rust-lang#75099 (lint/ty: move fns to avoid abstraction violation)
 - rust-lang#75502 (Use implicit (not explicit) rules for promotability by default in `const fn`)
 - rust-lang#75580 (Add test for checking duplicated branch or-patterns)
 - rust-lang#76310 (Add `[T; N]: TryFrom<Vec<T>>` (insta-stable))
 - rust-lang#76400 (Clean up vec benches bench_in_place style)
 - rust-lang#76434 (do not inline black_box when building for Miri)
 - rust-lang#76492 (Add associated constant `BITS` to all integer types)
 - rust-lang#76525 (Add as_str() to string::Drain.)
 - rust-lang#76636 (assert ScalarMaybeUninit size)
 - rust-lang#76749 (give *even better* suggestion when matching a const range)
 - rust-lang#76757 (don't convert types to the same type with try_into (clippy::useless_conversion))
 - rust-lang#76796 (Give a better error message when x.py uses the wrong stage for CI)
 - rust-lang#76798 (Build fixes for RISC-V 32-bit Linux support)

Failed merges:

r? `@ghost`
@bors bors merged commit 46bb884 into rust-lang:master Sep 19, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 19, 2020
@m-ou-se m-ou-se deleted the string-drain branch June 23, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
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
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
6 participants
@m-ou-se @rust-highfive @kennytm @dtolnay @bors @rustbot