str::lines and BufRead::lines: document trailing bare cr behaviour #91051

ijackson · 2021-11-19T19:11:21Z

Apropos discussion here #91047 (comment)

Sadly, str::lines gets this wrong. I think it is probably too late to
fix this, so document it instead.

Apropos discussion here rust-lang#91047 (comment) Sadly, str::lines gets this wrong. I think it is probably too late to fix this, so document it instead.

rust-highfive · 2021-11-19T19:11:24Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

wooster0

Can we keep it neutral and not have it sound so negative? I don't think giving the "we messed up" vibe in the official docs adds a lot of value.
And instead of referring to the two ways as "correct" and "incorrect" you could be more explicit.

library/core/src/str/mod.rs

library/std/src/io/mod.rs

library/core/src/str/mod.rs

library/std/src/io/mod.rs

hkratz · 2021-11-20T09:46:35Z

I think we should just fix this and make str::lines like BufRead::lines. I don't think that much (if any) code relies on this (faulty) behavior.

ijackson · 2021-11-20T13:03:15Z

I think we should just fix this and make str::lines like BufRead::lines. I don't think that much (if any) code relies on this (faulty) behavior.

I think this would need an FCP, but it is indeed our other option. I will work up an MR to do this, so we can see what it looks like (and what kind of justification I find I am able to write).

I think we should choose between fixing it, and acknowledging that it is, at the very least, anomalous. I don't think we should pretend (even implicitly) in the docs that we think the current behaviour of str::lines is correct.

shirshak55 · 2021-11-20T14:03:01Z

I don't think many libs rely on this behaviour so fixing would be a wise decision rather than mitigating it by adding docs. Not many of us are going to read the documentation and this is not expected too. So it would be better if we fix it. Maybe next time when we do a crater run we can investigate how much churn it would cause in the ecosystem.

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>

Co-authored-by: r00ster <r00ster91@protonmail.com>

rust-log-analyzer · 2022-04-12T01:51:20Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
 Documenting core v0.0.0 (/checkout/library/core)
error: unresolved link to `std::io::BufRead::lines`
   --> library/core/src/str/mod.rs:996:18
    |
996 |     /// Unlike [`std::io::BufRead::lines`], trailing bare CR is handled as a newline:
    |                  ^^^^^^^^^^^^^^^^^^^^^^^ no item named `std` in scope
    |
    = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
error: could not document `core`

Caused by:
Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2021 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/doc --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --markdown-css rust.css --markdown-no-toc -Z unstable-options --resource-suffix 1.62.0 --index-page /checkout/src/doc/index.md -C metadata=4599449636879869 -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --cfg=bootstrap -Csymbol-mangling-version=legacy -Zunstable-options -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.62.0-nightly
  (ce412f469
  2022-04-11)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")'` (exit status: 1)

Dylan-DPC · 2022-05-10T09:37:24Z

@ijackson if you can ~~rebase~~ fix the CI we can push this forward

ChrisDenton · 2022-05-13T18:43:38Z

I think we should just fix this and make str::lines like BufRead::lines.

I don't think many libs rely on this behaviour so fixing would be a wise decision rather than mitigating it by adding docs.

Given these comments, ping @rust-lang/libs-api

joshtriplett · 2022-05-13T20:38:59Z

I do think we could try a crater run fixing the two to be consistent.

m-ou-se · 2022-05-16T09:52:41Z

I agree, these two being inconsistent seems like a mistake we should fix.

JohnCSimon · 2022-07-23T23:32:45Z

Triage:
@ijackson What is the status of this PR?

JohnCSimon · 2022-07-23T23:56:07Z

@ijackson
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

Previously "bare\r" was split into ["bare"] even though the documentation said that only LF and CRLF count as newlines. 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()`. This is an alternative to rust-lang#91051, which proposes to document rather than fix the behaviour. Fixes rust-lang#94435. Co-authored-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

str::lines and BufRead::lines: document trailing bare cr behaviour

d346dfd

Apropos discussion here rust-lang#91047 (comment) Sadly, str::lines gets this wrong. I think it is probably too late to fix this, so document it instead.

rust-highfive assigned m-ou-se Nov 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2021

wooster0 suggested changes Nov 20, 2021

View reviewed changes

library/core/src/str/mod.rs Outdated Show resolved Hide resolved

library/std/src/io/mod.rs Outdated Show resolved Hide resolved

library/core/src/str/mod.rs Outdated Show resolved Hide resolved

library/std/src/io/mod.rs Outdated Show resolved Hide resolved

apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 22, 2021

ijackson mentioned this pull request Nov 24, 2021

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

Closed

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2022

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2022

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022

Apply suggestions from code review

9b1f6c5

Co-authored-by: r00ster <r00ster91@protonmail.com>

Dylan-DPC 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 May 10, 2022

JohnCSimon closed this Jul 23, 2022

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

str::lines and BufRead::lines: document trailing bare cr behaviour #91051

str::lines and BufRead::lines: document trailing bare cr behaviour #91051

ijackson commented Nov 19, 2021

rust-highfive commented Nov 19, 2021

wooster0 left a comment

hkratz commented Nov 20, 2021

ijackson commented Nov 20, 2021

shirshak55 commented Nov 20, 2021

rust-log-analyzer commented Apr 12, 2022

Dylan-DPC commented May 10, 2022 •

edited

Loading

ChrisDenton commented May 13, 2022

joshtriplett commented May 13, 2022

m-ou-se commented May 16, 2022

JohnCSimon commented Jul 23, 2022

JohnCSimon commented Jul 23, 2022

str::lines and BufRead::lines: document trailing bare cr behaviour #91051

str::lines and BufRead::lines: document trailing bare cr behaviour #91051

Conversation

ijackson commented Nov 19, 2021

rust-highfive commented Nov 19, 2021

wooster0 left a comment

Choose a reason for hiding this comment

hkratz commented Nov 20, 2021

ijackson commented Nov 20, 2021

shirshak55 commented Nov 20, 2021

rust-log-analyzer commented Apr 12, 2022

Dylan-DPC commented May 10, 2022 • edited Loading

ChrisDenton commented May 13, 2022

joshtriplett commented May 13, 2022

m-ou-se commented May 16, 2022

JohnCSimon commented Jul 23, 2022

JohnCSimon commented Jul 23, 2022

Dylan-DPC commented May 10, 2022 •

edited

Loading