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

library: core::str::lines: Fix handling of trailing bare CR #91191

Closed
wants to merge 5 commits into from
Closed

library: core::str::lines: Fix handling of trailing bare CR #91191

wants to merge 5 commits into from

Conversation

ijackson
Copy link
Contributor

E.g., split "bare\r" into the single line "bare\r", not "bare".

The documentation for this function says that only LF or CR-LF count as newlines. So a bare CR is not a line ending, and must not be stripped.

This fix is a behavioural change, even though it brings the behaviour into line with the documentation, and into line with that of std::io::BufRead:;lines().

It seems unlikely that anyone is relying on this bug, but I'm not sure how to rule it out. Perhaps this should have an FCP or a crater run or something. It should definitely be in the release notes.

This is an alternative to #91051, which proposes to document rather than fix the behaviour.

Also add some tests of edge cases. I felt they were useful for the docs so I made them doctests.

As for the implementation: the current version doesn't give the map closure the right information, so we need to use split_inclusive. After that, we can reuse the logic in the new str::trim_newline.

Currently that is not merged yet, so this branch is on top of #91047.

@ijackson
str: Implement str::strip_newline
be13a03
@ijackson
str::trim_newline: delinkify link to BufRead::lines
60cb3e6 
Sadly there doesn't seem to be a way to do this right now.
@ijackson
str::trim_newline: Add a test case demoing handling of LF-CR
ca04e2b
@ijackson
library: line splitting: Add some edge case doctests
825eab4 
Currently, there is one bug demonstrated here.

Signed-off-by: Ian Jackson <[email protected]>
@ijackson
library: core::str::lines: Fix handling of trailing bare CR
cd42365 
E.g., split "bare\r" into the single line "bare\r", not "bare".

The documentation for this function says that only LF or CR-LF count
as newlines.  So a bare CR is not a line ending, and must not be
stripped.

This fix is a behavioural change, even though it brings the behaviour
into line with the documentation, and into line with that of
`std::io::BufRead:;lines()`.

It seems unlikely that anyone is relying on this bug, but I'm not sure
how to rule it out. Perhaps this should have an FCP or a crater run or
something.  It should definitely be in the release notes.

This is an alternative to rust-lang#91051, which proposes to document rather
than fix the behaviour.

As for the implementation: the current version doesn't give the map
closure the right information, so we need to use split_inclusive.
After that, we can reuse the logic in the new `str::trim_newline`.

Signed-off-by: Ian Jackson <[email protected]>
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Nov 24, 2021
@ijackson
Copy link
Contributor Author

@rustbot modify labels +T-libs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 24, 2021
@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 10, 2021
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Dec 10, 2021
dtolnay
dtolnay requested changes Dec 10, 2021
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.

I'd be happy to propose FCP to T-libs-api, but I would prefer not to bundle a new public trim_newline method into this fix. Could that please be removed from this PR?

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2021
@ijackson
Copy link
Contributor Author

I'd be happy to propose FCP to T-libs-api, but I would prefer not to bundle a new public trim_newline method into this fix. Could that please be removed from this PR?

I had expected #91047 (trim_newline) to be uncontroversial, and therefore to be merged fairly quickly (ie, before this MR). That's why this branch is on top of that one: if #91047 had been merged then this MR would no longer contain the addition of trim_newline.

I'm not sure precisely why #91047 is stalled but if you prefer I can remove the pub from trim_newline here, and then if this MR goes in first, #91047's branch can be turned into "make this internal function pub". Doing it this way would mean #91047 ought to wait further, on the FCP here, to because otherwise merging #91047 would generate a merge conflict here.

@bors
Copy link
Contributor

bors commented Feb 6, 2022

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

@Dylan-DPC
Copy link
Member

@ijackson whats the update on this?

@hkratz hkratz mentioned this pull request Feb 28, 2022
Closed
dtolnay
dtolnay reviewed Mar 18, 2022
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.

I'll close this so that anyone else interested in resolving #94435 isn't deterred by there already being an old existing PR.

We'll need a PR that fixes the issue while exposing no novel standard library APIs.

@dtolnay dtolnay closed this Mar 18, 2022
@KamilaBorowska KamilaBorowska mentioned this pull request Aug 9, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 3, 2022
@matthiaskrgr
Rollup merge of rust-lang#100311 - xfix:lines-fix-handling-of-bare-cr…
acde5df 
…, r=joshtriplett

Fix handling of trailing bare CR in str::lines

Continuing from rust-lang#91191.

Fixes rust-lang#94435.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 22, 2023
@Dylan-DPC
Rollup merge of rust-lang#100311 - xfix:lines-fix-handling-of-bare-cr…
f592849 
…, r=ChrisDenton

Fix handling of trailing bare CR in str::lines

Continuing from rust-lang#91191.

Fixes rust-lang#94435.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 22, 2023
@Dylan-DPC
Rollup merge of rust-lang#100311 - xfix:lines-fix-handling-of-bare-cr…
d694f47 
…, r=ChrisDenton

Fix handling of trailing bare CR in str::lines

Continuing from rust-lang#91191.

Fixes rust-lang#94435.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay requested changes

Assignees

@dtolnay dtolnay

Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ijackson @rust-highfive @bors @Dylan-DPC @dtolnay @m-ou-se @rustbot