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

Change FromStr for String to use Infallible directly #67925

Merged
merged 1 commit into from
Feb 20, 2020

Conversation

petertodd
Copy link
Contributor

Fixes the confusing documentation on ParseError by making it irrelevant.

It might be fine to mark it as depreciated right now too - I can't imagine much code uses ParseError directly.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(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 Jan 6, 2020
src/liballoc/string.rs Outdated Show resolved Hide resolved
#[stable(feature = "str_parse_error", since = "1.5.0")]
pub type ParseError = core::convert::Infallible;

#[stable(feature = "rust1", since = "1.0.0")]
impl FromStr for String {
type Err = core::convert::Infallible;
#[inline]
fn from_str(s: &str) -> Result<String, ParseError> {
fn from_str(s: &str) -> Result<String, core::convert::Infallible> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this make ParseError unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think the obvious thing for downstream code that needs to specify ParseError for some reason to do is replace it with Infallible. Of course, that's a backwards compatibility issue: your code won't compile with older versions of Rust.

Though I'd expect it to be a fairly rare one, as I doubt much downstream code needs to actually mention ParseError directly.

Copy link
Member

Choose a reason for hiding this comment

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

This might as well be -> Result<String, Self::Err> to prevent any future churn.

/// defined, but, given that a [`String`] can always be made into a new
/// [`String`] without error, this type will never actually be returned. As
/// such, it is only here to satisfy said signature, and is useless otherwise.
/// This alias exists for backwards compatibility, and will be eventually deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

Why will it be deprecated? Says who?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the never type works on all editions I don't see why people would ever use Infalliable long-term. Deprecation doesn't prevent its usage, just discourage it.

@JohnCSimon
Copy link
Member

ping from triage:
@petertodd @shepmaster What is the status of this PR, it's sat idle for a few weeks.

@petertodd
Copy link
Contributor Author

@JohnCSimon @shepmaster I changed it to return Self::Err as suggested; should be fine to merge so long as others' think it's a good idea.

@shepmaster
Copy link
Member

The code change itself looks fine, but I'm not cool enough to vet the changes about deprecation.

r? @LukasKalbertodt

@LukasKalbertodt
Copy link
Member

I think this is fine as is. The only non-doc changes are definitely non-breaking. And mentioning ParseError will be deprecated eventually is fine, as that was part of the initial plan (implemented in #58302). Just to be sure: @SimonSapin does this seem fine to you? (as you've been heavily involved in this discussion.)

@JohnCSimon
Copy link
Member

Ping from triage: @SimonSapin - can you please review this?
@LukasKalbertodt is this PR ready to be merged?

src/liballoc/string.rs Outdated Show resolved Hide resolved
Fixes the confusing documentation on `ParseError` by making it
irrelevant.
@LukasKalbertodt
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2020

📌 Commit 883e69d has been approved by LukasKalbertodt

@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 Feb 19, 2020
@bors
Copy link
Contributor

bors commented Feb 20, 2020

⌛ Testing commit 883e69d with merge de362d8...

@bors
Copy link
Contributor

bors commented Feb 20, 2020

☀️ Test successful - checks-azure
Approved by: LukasKalbertodt
Pushing de362d8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2020
@bors bors merged commit de362d8 into rust-lang:master Feb 20, 2020
@petertodd petertodd deleted the 2020-fromstr-infallible branch June 2, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants