Skip to content

Commit

Permalink
Remove special pre-visit for module docstrings (#9261)
Browse files Browse the repository at this point in the history
This ensures that we visit the module docstring like any other string.

Closes #9260.
  • Loading branch information
charliermarsh authored Dec 23, 2023
1 parent 506ffad commit 20def33
Show file tree
Hide file tree
Showing 15 changed files with 299 additions and 220 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import a

"""Some other docstring."""

import b

"""Some other docstring."""

import c
5 changes: 5 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyflakes/F404_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
"""Docstring"""

"""Non-docstring"""

from __future__ import absolute_import
36 changes: 17 additions & 19 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP025.py
Original file line number Diff line number Diff line change
@@ -1,30 +1,28 @@
# These should change
x = u"Hello"
u"Hello"

u'world'
x = u"Hello" # UP025

print(u"Hello")
u'world' # UP025

print(u'world')
print(u"Hello") # UP025

import foo

foo(u"Hello", U"world", a=u"Hello", b=u"world")
print(u'world') # UP025

# These should stay quoted they way they are
import foo

x = u'hello'
x = u"""hello"""
x = u'''hello'''
x = u'Hello "World"'
foo(u"Hello", U"world", a=u"Hello", b=u"world") # UP025

# These should not change
u = "Hello"
# Retain quotes when fixing.
x = u'hello' # UP025
x = u"""hello""" # UP025
x = u'''hello''' # UP025
x = u'Hello "World"' # UP025

u = u
u = "Hello" # OK
u = u # OK

def hello():
return"Hello"
return"Hello" # OK

f"foo"u"bar"
f"foo" u"bar"
f"foo"u"bar" # OK
f"foo" u"bar" # OK
26 changes: 15 additions & 11 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,18 @@ where

// Track whether we've seen docstrings, non-imports, etc.
match stmt {
Stmt::Expr(ast::StmtExpr { value, .. })
if !self
.semantic
.flags
.intersects(SemanticModelFlags::MODULE_DOCSTRING)
&& value.is_string_literal_expr() =>
{
self.semantic.flags |= SemanticModelFlags::MODULE_DOCSTRING;
}
Stmt::ImportFrom(ast::StmtImportFrom { module, names, .. }) => {
self.semantic.flags |= SemanticModelFlags::MODULE_DOCSTRING;

// Allow __future__ imports until we see a non-__future__ import.
if let Some("__future__") = module.as_deref() {
if names
Expand All @@ -301,9 +312,11 @@ where
}
}
Stmt::Import(_) => {
self.semantic.flags |= SemanticModelFlags::MODULE_DOCSTRING;
self.semantic.flags |= SemanticModelFlags::FUTURES_BOUNDARY;
}
_ => {
self.semantic.flags |= SemanticModelFlags::MODULE_DOCSTRING;
self.semantic.flags |= SemanticModelFlags::FUTURES_BOUNDARY;
if !(self.semantic.seen_import_boundary()
|| helpers::is_assignment_to_a_dunder(stmt)
Expand Down Expand Up @@ -1435,11 +1448,8 @@ where

impl<'a> Checker<'a> {
/// Visit a [`Module`]. Returns `true` if the module contains a module-level docstring.
fn visit_module(&mut self, python_ast: &'a Suite) -> bool {
fn visit_module(&mut self, python_ast: &'a Suite) {
analyze::module(python_ast, self);

let docstring = docstrings::extraction::docstring_from(python_ast);
docstring.is_some()
}

/// Visit a list of [`Comprehension`] nodes, assumed to be the comprehensions that compose a
Expand Down Expand Up @@ -2006,14 +2016,8 @@ pub(crate) fn check_ast(
);
checker.bind_builtins();

// Check for module docstring.
let python_ast = if checker.visit_module(python_ast) {
&python_ast[1..]
} else {
python_ast
};

// Iterate over the AST.
checker.visit_module(python_ast);
checker.visit_body(python_ast);

// Visit any deferred syntax nodes. Take care to visit in order, such that we avoid adding
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff_linter/src/rules/pycodestyle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ mod tests {
#[test_case(Rule::LineTooLong, Path::new("E501_3.py"))]
#[test_case(Rule::MixedSpacesAndTabs, Path::new("E101.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E40.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402_0.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402_1.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402.ipynb"))]
#[test_case(Rule::MultipleImportsOnOneLine, Path::new("E40.py"))]
#[test_case(Rule::MultipleStatementsOnOneLineColon, Path::new("E70.py"))]
Expand Down Expand Up @@ -65,7 +66,7 @@ mod tests {
}

#[test_case(Rule::IsLiteral, Path::new("constant_literals.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402.py"))]
#[test_case(Rule::ModuleImportNotAtTopOfFile, Path::new("E402_0.py"))]
#[test_case(Rule::TypeComparison, Path::new("E721.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E402.py:25:1: E402 Module level import not at top of file
E402_0.py:25:1: E402 Module level import not at top of file
|
23 | sys.path.insert(0, "some/path")
24 |
Expand All @@ -11,7 +11,7 @@ E402.py:25:1: E402 Module level import not at top of file
27 | import matplotlib
|

E402.py:27:1: E402 Module level import not at top of file
E402_0.py:27:1: E402 Module level import not at top of file
|
25 | import f
26 |
Expand All @@ -21,7 +21,7 @@ E402.py:27:1: E402 Module level import not at top of file
29 | matplotlib.use("Agg")
|

E402.py:31:1: E402 Module level import not at top of file
E402_0.py:31:1: E402 Module level import not at top of file
|
29 | matplotlib.use("Agg")
30 |
Expand All @@ -31,23 +31,23 @@ E402.py:31:1: E402 Module level import not at top of file
33 | __some__magic = 1
|

E402.py:35:1: E402 Module level import not at top of file
E402_0.py:35:1: E402 Module level import not at top of file
|
33 | __some__magic = 1
34 |
35 | import h
| ^^^^^^^^ E402
|

E402.py:45:1: E402 Module level import not at top of file
E402_0.py:45:1: E402 Module level import not at top of file
|
43 | import j
44 |
45 | import k; import l
| ^^^^^^^^ E402
|

E402.py:45:11: E402 Module level import not at top of file
E402_0.py:45:11: E402 Module level import not at top of file
|
43 | import j
44 |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E402_1.py:5:1: E402 Module level import not at top of file
|
3 | """Some other docstring."""
4 |
5 | import b
| ^^^^^^^^ E402
6 |
7 | """Some other docstring."""
|

E402_1.py:9:1: E402 Module level import not at top of file
|
7 | """Some other docstring."""
8 |
9 | import c
| ^^^^^^^^ E402
|


Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
---
source: crates/ruff_linter/src/rules/pycodestyle/mod.rs
---
E402.py:35:1: E402 Module level import not at top of file
E402_0.py:35:1: E402 Module level import not at top of file
|
33 | __some__magic = 1
34 |
35 | import h
| ^^^^^^^^ E402
|

E402.py:45:1: E402 Module level import not at top of file
E402_0.py:45:1: E402 Module level import not at top of file
|
43 | import j
44 |
45 | import k; import l
| ^^^^^^^^ E402
|

E402.py:45:11: E402 Module level import not at top of file
E402_0.py:45:11: E402 Module level import not at top of file
|
43 | import j
44 |
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ mod tests {
#[test_case(Rule::UnusedImport, Path::new("F401_20.py"))]
#[test_case(Rule::ImportShadowedByLoopVar, Path::new("F402.py"))]
#[test_case(Rule::UndefinedLocalWithImportStar, Path::new("F403.py"))]
#[test_case(Rule::LateFutureImport, Path::new("F404.py"))]
#[test_case(Rule::LateFutureImport, Path::new("F404_0.py"))]
#[test_case(Rule::LateFutureImport, Path::new("F404_1.py"))]
#[test_case(Rule::UndefinedLocalWithImportStarUsage, Path::new("F405.py"))]
#[test_case(Rule::UndefinedLocalWithNestedImportStarUsage, Path::new("F406.py"))]
#[test_case(Rule::FutureFeatureNotDefined, Path::new("F407.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F404.py:6:1: F404 `from __future__` imports must occur at the beginning of the file
F404_0.py:6:1: F404 `from __future__` imports must occur at the beginning of the file
|
4 | from collections import namedtuple
5 |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F404_1.py:5:1: F404 `from __future__` imports must occur at the beginning of the file
|
3 | """Non-docstring"""
4 |
5 | from __future__ import absolute_import
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ F404
|


Loading

0 comments on commit 20def33

Please sign in to comment.