Fix various bugs found via running the test suite by dhruvmanila · Pull Request #11611 · astral-sh/ruff

dhruvmanila · 2024-05-30T06:36:04Z

Summary

This PR fixes various bugs found when running the test suite. Note that it doesn't update the code to make it compile which is done in a separate PR.

Refer to review comments for each individual bug.

dhruvmanila · 2024-05-30T10:07:19Z

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

                }
                if checker.enabled(Rule::PrintfStringFormatting) {
-                    pyupgrade::rules::printf_string_formatting(checker, format_string, right);
+                    pyupgrade::rules::printf_string_formatting(checker, bin_op, format_string);


The function requires the range and the right side of the binary expression so we let's just pass the binary expression as a whole.

dhruvmanila · 2024-05-30T10:07:42Z

crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/implicit.rs

        let (a_range, b_range) = match (a_token.kind(), b_token.kind()) {
            (TokenKind::String, TokenKind::String) => (a_token.range(), b_token.range()),
            (TokenKind::String, TokenKind::FStringStart) => {
-                match indexer.fstring_ranges().innermost(a_token.start()) {
+                match indexer.fstring_ranges().innermost(b_token.start()) {
                    Some(b_range) => (a_token.range(), b_range),
                    None => continue,
                }


Must've been a copy paste mistake.

That must have been annoying to find.

dhruvmanila · 2024-05-30T10:10:23Z

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

-                    TokenKind::Async if matches!(self.tokens.peek(), Some((TokenKind::Def, _))) => {
-                        LogicalLineKind::Function
+                        continue;
                    }
-                    TokenKind::Import => LogicalLineKind::Import,
-                    TokenKind::From => LogicalLineKind::FromImport,
-                    _ => LogicalLineKind::Other,
-                };

-                first_logical_line_token = Some((logical_line_kind, range));
+                    is_docstring = kind == TokenKind::String;
+
+                    let logical_line_kind = match kind {
+                        TokenKind::Class => LogicalLineKind::Class,
+                        TokenKind::Comment => LogicalLineKind::Comment,
+                        TokenKind::At => LogicalLineKind::Decorator,
+                        TokenKind::Def => LogicalLineKind::Function,
+                        // Lookahead to distinguish `async def` from `async with`.
+                        TokenKind::Async
+                            if self
+                                .tokens
+                                .peek()
+                                .is_some_and(|token| token.kind() == TokenKind::Def) =>


I think this isn't a bug fix, but mostly a change from:

if matches!(self.tokens.peek(), Some((TokenKind::Def, _)))

To

if self .tokens .peek() .is_some_and(|token| token.kind() == TokenKind::Def)

Yeah, I think this is more a compilation error fix?

dhruvmanila · 2024-05-30T10:12:28Z

crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs

                    diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
                        content,
-                        located_op.range + expr.start(),
+                        located_op.range,
                    )));


The reason this offset exists was because the lexer function used was lex instead of lex_starts_at.

ruff/crates/ruff_linter/src/rules/pyflakes/rules/invalid_literal_comparisons.rs

Lines 152 to 157 in b0a7510

let mut tok_iter = lexer::lex(&parenthesized_contents, Mode::Expression)

.flatten()

.skip(1)

.map(|(tok, range)| (tok, range - TextSize::from(1)))

.filter(|(tok, _)| !matches!(tok, Tok::NonLogicalNewline | Tok::Comment(_)))

.peekable();

dhruvmanila · 2024-05-30T10:14:18Z

crates/ruff_linter/src/rules/pyupgrade/fixes.rs

    // Reconstruct the source code by skipping any names that are in `members`.
    let mut output = String::with_capacity(import_from_stmt.range().len().to_usize());
-    let mut last_pos = TextSize::default();
+    let mut last_pos = import_from_stmt.start();


Now, we use the locator containing the entire source code, so the start offset should be the statement start offset.

dhruvmanila · 2024-05-30T10:19:58Z

crates/ruff_python_parser/src/lexer.rs

-            (_, quote @ ('\'' | '"')) => self.try_single_char_prefix(first).then_some(quote),
+            (_, quote @ ('\'' | '"')) => self.try_single_char_prefix(first).then(|| {
+                self.cursor.bump();
+                quote
+            }),


Oops! Forgot to bump the cursor.

dhruvmanila · 2024-05-30T10:20:14Z

crates/ruff_python_parser/src/lexer.rs

        if self.cursor.eat_char2(quote, quote) {
-            self.current_flags = TokenFlags::TRIPLE_QUOTED_STRING;
+            self.current_flags |= TokenFlags::TRIPLE_QUOTED_STRING;
        }


dhruvmanila · 2024-05-30T10:20:23Z

crates/ruff_python_parser/src/lexer.rs

        if self.cursor.eat_char2(quote, quote) {
-            self.current_flags = TokenFlags::TRIPLE_QUOTED_STRING;
+            self.current_flags |= TokenFlags::TRIPLE_QUOTED_STRING;
        }


Yeah, not sure why I did this xD

dhruvmanila · 2024-05-30T10:21:14Z

crates/ruff_python_parser/src/lexer.rs

    /// Returns `true` if the token is a raw string.
    const fn is_raw_string(self) -> bool {
-        self.contains(TokenFlags::RAW_STRING)
+        self.intersects(TokenFlags::RAW_STRING)
    }


Using contains is incorrect here.

For all other changes, I find intersects easier to reason about.

crates/ruff_python_parser/src/token_source.rs

MichaReiser · 2024-05-30T10:41:11Z

crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/implicit.rs

        let (a_range, b_range) = match (a_token.kind(), b_token.kind()) {
            (TokenKind::String, TokenKind::String) => (a_token.range(), b_token.range()),
            (TokenKind::String, TokenKind::FStringStart) => {
-                match indexer.fstring_ranges().innermost(a_token.start()) {
+                match indexer.fstring_ranges().innermost(b_token.start()) {
                    Some(b_range) => (a_token.range(), b_range),
                    None => continue,
                }


That must have been annoying to find.

MichaReiser · 2024-05-30T10:43:03Z

crates/ruff_linter/src/rules/pycodestyle/rules/blank_lines.rs

-                    TokenKind::Async if matches!(self.tokens.peek(), Some((TokenKind::Def, _))) => {
-                        LogicalLineKind::Function
+                        continue;
                    }
-                    TokenKind::Import => LogicalLineKind::Import,
-                    TokenKind::From => LogicalLineKind::FromImport,
-                    _ => LogicalLineKind::Other,
-                };

-                first_logical_line_token = Some((logical_line_kind, range));
+                    is_docstring = kind == TokenKind::String;
+
+                    let logical_line_kind = match kind {
+                        TokenKind::Class => LogicalLineKind::Class,
+                        TokenKind::Comment => LogicalLineKind::Comment,
+                        TokenKind::At => LogicalLineKind::Decorator,
+                        TokenKind::Def => LogicalLineKind::Function,
+                        // Lookahead to distinguish `async def` from `async with`.
+                        TokenKind::Async
+                            if self
+                                .tokens
+                                .peek()
+                                .is_some_and(|token| token.kind() == TokenKind::Def) =>


Yeah, I think this is more a compilation error fix?

MichaReiser · 2024-05-30T10:44:49Z

crates/ruff_python_formatter/src/verbatim.rs


-impl LogicalLinesIter<'_> {
-    fn new(tokens: Iter<'_, parser::Token>, verbatim_range: TextRange) -> Self {
+impl<'a> LogicalLinesIter<'a> {


Nit: I think '_ should just work fine here but there's no need to change it.

MichaReiser · 2024-05-30T10:45:16Z

crates/ruff_python_index/src/multiline_ranges.rs

    pub(crate) fn visit_token(&mut self, token: &Token) {
-        if matches!(token.kind(), TokenKind::String | TokenKind::FStringStart) {
+        if matches!(token.kind(), TokenKind::String | TokenKind::FStringMiddle) {
            if token.is_triple_quoted_string() {
                self.ranges.push(token.range());
            }


Is this a bug in main too or something you introduced by your refactor?

crates/ruff_python_parser/src/token_source.rs

## Summary This PR fixes various bugs found when running the test suite. Note that it doesn't update the code to make it compile which is done in a separate PR. Refer to review comments for each individual bug.

dhruvmanila added the bug Something isn't working label May 30, 2024

dhruvmanila force-pushed the dhruv/bugfixes branch 2 times, most recently from a40012c to c137200 Compare May 30, 2024 10:04

Base automatically changed from dhruv/tokens-range-query to dhruv/parser-phase-2 May 30, 2024 10:05

Fix various bugs found via running the test suite

b356439

dhruvmanila force-pushed the dhruv/bugfixes branch from c137200 to b356439 Compare May 30, 2024 10:05

dhruvmanila commented May 30, 2024

View reviewed changes

dhruvmanila marked this pull request as ready for review May 30, 2024 10:22

dhruvmanila requested a review from MichaReiser as a code owner May 30, 2024 10:22

MichaReiser approved these changes May 30, 2024

View reviewed changes

Add eof related comment in token source

264dc23

dhruvmanila merged commit ba4d85c into dhruv/parser-phase-2 May 30, 2024

dhruvmanila deleted the dhruv/bugfixes branch May 30, 2024 10:52

	let mut tok_iter = lexer::lex(&parenthesized_contents, Mode::Expression)
	.flatten()
	.skip(1)
	.map(\|(tok, range)\| (tok, range - TextSize::from(1)))
	.filter(\|(tok, _)\| !matches!(tok, Tok::NonLogicalNewline \| Tok::Comment(_)))
	.peekable();

Conversation

dhruvmanila commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dhruvmanila commented May 30, 2024 •

edited

Loading