F401 Deletes import, cause EOF error

[jasikpark]

With this program:

##d####
\
import ost

(possibly importantly w/ the lack of a return character after ost

outputs this when run w/ --fix:

ruff artifacts/ruff_fix_validity/minimized-from-crash-dd75211301269fe8 --fix
warning: Detected debug build without --no-cache.
error: Autofix introduced a syntax error in `artifacts/ruff_fix_validity/minimized-from-crash-dd75211301269fe8` with rule codes F401: unexpected EOF while parsing at byte offset 11
---
##d####
\

---
artifacts/ruff_fix_validity/minimized-from-crash-dd75211301269fe8:3:8: F401 `ost` imported but unused
Found 1 error.

Found via #4822, run against main (be107da)

[charliermarsh]

Yeah the general problem here is that a file can't end in a backslash, I think (even a newline is sufficient).

[jasikpark]

Ah, thanks for the info, I was pretty bewildered by this xd

[jasikpark]

I wonder what the right thing to do is when a simple change ends up with an incorrect program?

[charliermarsh]

We should of course fix it, but it's worth noting that this is a fairly unlikely case to hit in the wild. Continuations themselves are relatively rare, and most editors don't even let you save a file without a trailing newline :)

[charliermarsh]

I actually thought we did handle this case already:

pub(crate) fn delete_stmt(
    stmt: &Stmt,
    parent: Option<&Stmt>,
    locator: &Locator,
    indexer: &Indexer,
    stylist: &Stylist,
) -> Edit {
    if parent
        .map(|parent| is_lone_child(stmt, parent))
        .unwrap_or_default()
    {
        // If removing this node would lead to an invalid syntax tree, replace
        // it with a `pass`.
        Edit::range_replacement("pass".to_string(), stmt.range())
    } else {
        if let Some(semicolon) = trailing_semicolon(stmt, locator) {
            let next = next_stmt_break(semicolon, locator);
            Edit::deletion(stmt.start(), next)
        } else if helpers::has_leading_content(stmt, locator) {
            Edit::range_deletion(stmt.range())
        } else if helpers::preceded_by_continuation(stmt, indexer, locator) {
            if is_end_of_file(stmt, locator) && locator.is_at_start_of_line(stmt.start()) {
                // Special-case: a file can't end in a continuation.
                Edit::range_replacement(stylist.line_ending().to_string(), stmt.range())
            } else {
                Edit::range_deletion(stmt.range())
            }
        } else {
            let range = locator.full_lines_range(stmt.range());
            Edit::range_deletion(range)
        }
    }
}

Note the branch that says // Special-case: a file can't end in a continuation.. Need to look into that...

[charliermarsh]

I think the safest and most general fix is to extend the deletion to include the continuation character itself.

[evanrittenhouse]

Yeah the general problem here is that a file can't end in a backslash, I think (even a newline is sufficient).

I'm pretty sure that the trailing newline is what's causing a similar issue #4404. We attempt to strip the newline, then on the next iteration, the file ends in a \. I think the fix will rely on some combination of physical lines (Line) and Stmt, but not quite sure how to unify them yet.

[dhruvmanila]

Note the branch that says // Special-case: a file can't end in a continuation.. Need to look into that...

Did a quick debug, it turns out that specific continuation offset is not being stored here:

ruff/crates/ruff_python_ast/src/source_code/indexer.rs

Lines 14 to 15 in 653a0eb

	/// Stores the start offset of continuation lines.
	continuation_lines: Vec<TextSize>,

This is happening as the logic is not considering a possibility of having a continuation character before the first token in file.

Edit: It seems to be happening whenever the continuation character is at the start of the line because these continuation offsets are not being considered:

\
import sys

\
import os

ruff/crates/ruff_python_ast/src/source_code/indexer.rs

Lines 51 to 57 in 653a0eb

	// Newlines after a newline never form a continuation.
	if !matches!(
	prev_token,
	Some(Tok::Newline | Tok::NonLogicalNewline) | None
	) {
	continuation_lines.push(line_start);
	}

The logic says that if the previous token is not a newline or not at the start, only then consider this for the continuation offset.

[charliermarsh]

Is that only a start-of-file problem? I wonder if that match should allow None?

[dhruvmanila]

Is that only a start-of-file problem? I wonder if that match should allow None?

No, sorry I updated the comment. It's occurring whenever the continuation character is at the start of line. This means the previous token was either a newline or None. The opposite of this condition is used to check whether to add the offset or not.