Skip to content

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

Closed
ijackson wants to merge 5 commits intorust-lang:masterfrom
ijackson:fix-anomalous-lines
Closed

library: core::str::lines: Fix handling of trailing bare CR#91191
ijackson wants to merge 5 commits intorust-lang:masterfrom
ijackson:fix-anomalous-lines

Conversation

@ijackson
Copy link
Contributor

@ijackson ijackson commented Nov 24, 2021

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 <ijackson@chiark.greenend.org.uk>
@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 <ijackson@chiark.greenend.org.uk>
@rust-highfive
Copy link
Contributor

rust-highfive commented Nov 24, 2021

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

ijackson commented Nov 24, 2021

@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, or significant enough to need a team 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

ijackson commented Dec 10, 2021

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
Collaborator

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

Dylan-DPC commented Feb 19, 2022

@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
@sugar700 sugar700 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, or significant enough to need a team 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