Skip to content

Fix Indexer fails to identify continuation preceded by newline #10351#10354

Merged
charliermarsh merged 1 commit intoastral-sh:mainfrom
augustelalande:indexer-bug
Mar 12, 2024
Merged

Fix Indexer fails to identify continuation preceded by newline #10351#10354
charliermarsh merged 1 commit intoastral-sh:mainfrom
augustelalande:indexer-bug

Conversation

@augustelalande
Copy link
Contributor

@augustelalande augustelalande commented Mar 12, 2024

Summary

Fixes #10351

It seems the bug was caused by this section of code

ruff/crates/ruff_python_index/src/indexer.rs

Lines 55 to 58 in b669306

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

It's true that newline tokens cannot be immediately followed by line continuations, but only outside parentheses. e.g. the exception

(
    1
    \
    + 2)

But why was this check put there in the first place? Is it guarding against something else?

Test Plan

New test was added to indexer

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@charliermarsh
Copy link
Member

charliermarsh commented Mar 12, 2024

Can you find anything via git blame about why this is here?

@charliermarsh
Copy link
Member

charliermarsh commented Mar 12, 2024

(It's possible that something changed in the lexer over time that made this not necessary.)

@augustelalande
Copy link
Contributor Author

augustelalande commented Mar 12, 2024

This was the original implementation

ruff/src/source_code/indexer.rs

Lines 22 to 48 in 3791ca7

impl From<&[LexResult]> for Indexer {
fn from(lxr: &[LexResult]) -> Self {
let mut commented_lines = Vec::new();
let mut continuation_lines = Vec::new();
let mut prev: Option<(&Location, &Tok, &Location)> = None;
for (start, tok, end) in lxr.iter().flatten() {
if matches!(tok, Tok::Comment(_)) {
commented_lines.push(start.row());
}
if let Some((.., prev_tok, prev_end)) = prev {
if !matches!(
prev_tok,
Tok::Newline | Tok::NonLogicalNewline | Tok::Comment(..)
) {
for line in prev_end.row()..start.row() {
continuation_lines.push(line);
}
}
}
prev = Some((start, tok, end));
}
Self {
commented_lines,
continuation_lines,
}
}
}

which had slightly different logic and needed the check to avoid flagging every newline. The checks just got kept through the edits.

Note: This implementation would also have failed to parse the code in question correctly.

@charliermarsh
Copy link
Member

charliermarsh commented Mar 12, 2024

Okay, I think it was this change, long ago: RustPython/Parser#27

@charliermarsh charliermarsh merged commit dacec73 into astral-sh:main Mar 12, 2024
@charliermarsh charliermarsh added the bug Something isn't working label Mar 12, 2024
@augustelalande augustelalande deleted the indexer-bug branch March 12, 2024 04:54
@BrewTestBot BrewTestBot mentioned this pull request Mar 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Indexer fails to identify continuation preceded by newline

2 participants

@augustelalande @charliermarsh