Skip to content

Conversation

@eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Feb 7, 2026

LineBuffer::insert_newline() currently inserts CRLF (\r\n) on Windows and LF (\n) elsewhere.
This change makes it always insert LF on all platforms.

Background

I use reedline as the line editor in arf, an R console. R's parser treats \r as an invalid token, so any CR characters in the input buffer cause parse errors.
I currently have to strip CR from the buffer after read_line() returns, but this is a workaround for what appears to be an unnecessary platform distinction in reedline itself.

Why the CRLF insertion is unnecessary

The Windows-specific CRLF was introduced in #399 with the rationale of using the "system newline separator."
However, reedline's own architecture already handles the LF→CRLF conversion at the display layer, making the buffer-level CRLF both redundant and inconsistent with the rest of the codebase:

1. The painting layer already handles CRLF conversion

coerce_crlf() converts bare LF to CRLF right before writing to the terminal.

pub(crate) fn coerce_crlf(input: &str) -> Cow<'_, str> {
let mut result = Cow::Borrowed(input);
let mut cursor: usize = 0;
for (idx, _) in input.match_indices('\n') {
if !(idx > 0 && input.as_bytes()[idx - 1] == b'\r') {
if let Cow::Borrowed(_) = result {
// Best case 1 allocation, worst case 2 allocations
let mut owned = String::with_capacity(input.len() + 1);
// Optimization to avoid the `AddAssign for Cow<str>`
// optimization for `Cow<str>.is_empty` that would replace the
// preallocation
owned.push_str(&input[cursor..idx]);
result = Cow::Owned(owned);
} else {
result += &input[cursor..idx];
}
result += "\r\n";
// Advance beyond the matched LF char (single byte)
cursor = idx + 1;
}
}
if let Cow::Owned(_) = result {
result += &input[cursor..input.len()];
}
result
}

This is applied to all Print() calls in painter.rs. The buffer is meant to store logical content (LF only), and the display layer adapts it for the terminal.

2. All line-counting logic assumes LF only

The rendering code exclusively uses \n for line operations:

  • skip_buffer_lines() uses string.match_indices('\n') and trim_end_matches('\n')
    fn skip_buffer_lines_range(string: &str, skip: usize, offset: Option<usize>) -> (usize, usize) {
    let mut matches = string.match_indices('\n');
  • estimate_required_lines() uses .lines() (which splits on \n)
  • screen_to_buffer_offset() checks if grapheme == "\n"
    if grapheme == "\n" {

A \r in the buffer is invisible to these calculations, which could lead to off-by-one errors in cursor positioning on Windows.

3. Previous fix established that CR in the buffer is harmful

#262 explicitly removed CRLF insertion from the hinter logic, noting:

Carriage returns, that are not introduced at the end of the line as part of the painter or the OS convention, break the painting of multilines, as the cursor position might jump to the start of the multiline continuation prompt and start painting from there!

The same reasoning applies to insert_newline() — it inserts CR into the buffer outside the painting layer.

4. Nushell's parser works around this

Nushell's lexer explicitly ignores standalone \r:
https://github.com/nushell/nushell/blob/10a932e0c9e8e8715ccad006594a70fc83a492d3/crates/nu-parser/src/lex.rs#L633-L637

        } else if c == b'\r' {
            // Ignore a stand-alone carriage return
            curr_offset += 1;
        } else if c == b'\n' {
            // If the next character is a newline, we're looking at an EOL (end of line) token.

This suggests the CRLF in the buffer is not useful to nushell either — it's simply tolerated.

What this changes

  • insert_newline() now always inserts \n, removing the #[cfg(target_os = "windows")] branch
  • No change to the painting layer — coerce_crlf() continues to handle terminal output

@fdncred
Copy link
Contributor

fdncred commented Feb 7, 2026

I'd like to see tests that prove this case works cross-platform. Is that something that can be added?

@eitsupi
Copy link
Contributor Author

eitsupi commented Feb 7, 2026

Added a unit test (insert_newline_inserts_lf_only) that verifies insert_newline() always inserts \n only and never introduces \r into the buffer.
It covers insertion at various positions (start, middle, end, empty buffer, multiline).

Since this is a pure LineBuffer operation with no terminal I/O, the test works on any platform.
The CI Windows runner will confirm that no platform-specific codepath reintroduces CRLF.

The display-side CRLF conversion (coerce_crlf) already has its own test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants