Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 12, 2024

Conversation

augustelalande
Copy link
Contributor

Summary

Fixes #10351

It seems the bug was caused by this section of code

// 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

Copy link

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

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

@charliermarsh
Copy link
Member

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

@augustelalande
Copy link
Contributor Author

This was the original implementation

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

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

@charliermarsh charliermarsh merged commit dacec73 into astral-sh:main Mar 12, 2024
17 checks passed
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indexer fails to identify continuation preceded by newline
2 participants