Skip to content

Commit

Permalink
Disallow access to Parsed output, use the API instead (#11741)
Browse files Browse the repository at this point in the history
## Summary

This PR is a follow-up to #11740 to restrict access to the `Parsed`
output by replacing the `parsed` API function with a more specific one.
Currently, that is `comment_ranges` but the linked PR exposes a `tokens`
method.

The main motivation is so that there's no way to get an incorrect
information from the checker. And, it also encapsulates the source of
the comment ranges and the tokens itself. This way it would become
easier to just update the checker if the source for these information
changes in the future.

## Test Plan

`cargo insta test`
  • Loading branch information
dhruvmanila authored Jun 5, 2024
1 parent b021b5b commit 2e0a975
Show file tree
Hide file tree
Showing 36 changed files with 44 additions and 70 deletions.
7 changes: 4 additions & 3 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ use ruff_python_semantic::{
StarImport, SubmoduleImport,
};
use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS};
use ruff_python_trivia::CommentRanges;
use ruff_source_file::{Locator, OneIndexed, SourceRow};
use ruff_text_size::{Ranged, TextRange, TextSize};

Expand Down Expand Up @@ -326,9 +327,9 @@ impl<'a> Checker<'a> {
}
}

/// The [`Parsed`] output for the current file, which contains the tokens, AST, and more.
pub(crate) const fn parsed(&self) -> &'a Parsed<ModModule> {
self.parsed
/// Returns the [`CommentRanges`] for the parsed source code.
pub(crate) fn comment_ranges(&self) -> &'a CommentRanges {
self.parsed.comment_ranges()
}

/// Returns the [`Tokens`] for the parsed type annotation if the checker is in a typing context
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub(crate) fn zip_without_explicit_strict(checker: &mut Checker, call: &ast::Exp
add_argument(
"strict=False",
&call.arguments,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
),
// If the function call contains `**kwargs`, mark the fix as unsafe.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::Expr
let range = parenthesized_range(
argument.into(),
(&call.arguments).into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(argument.range());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ pub(crate) fn unnecessary_dict_kwargs(checker: &mut Checker, call: &ast::ExprCal
parenthesized_range(
value.into(),
dict.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(value.range())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ fn generate_fix(
let insertion = add_argument(
locator.slice(generic_base),
arguments,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
source,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,15 +284,15 @@ pub(crate) fn unittest_assertion(
// the assertion is part of a larger expression.
if checker.semantic().current_statement().is_expr_stmt()
&& checker.semantic().current_expression_parent().is_none()
&& !checker.parsed().comment_ranges().intersects(expr.range())
&& !checker.comment_ranges().intersects(expr.range())
{
if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
checker.generator().stmt(&stmt),
parenthesized_range(
expr.into(),
checker.semantic().current_statement().into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(expr.range()),
Expand Down Expand Up @@ -385,7 +385,6 @@ pub(crate) fn unittest_raises_assertion(
call.func.range(),
);
if !checker
.parsed()
.comment_ranges()
.has_comments(call, checker.locator())
{
Expand Down Expand Up @@ -745,7 +744,7 @@ pub(crate) fn composite_condition(
let mut diagnostic = Diagnostic::new(PytestCompositeAssertion, stmt.range());
if matches!(composite, CompositionKind::Simple)
&& msg.is_none()
&& !checker.parsed().comment_ranges().intersects(stmt.range())
&& !checker.comment_ranges().intersects(stmt.range())
&& !checker
.indexer()
.in_multi_statement_line(stmt, checker.locator())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ fn check_names(checker: &mut Checker, decorator: &Decorator, expr: &Expr) {
let name_range = get_parametrize_name_range(
decorator,
expr,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(expr.range());
Expand Down Expand Up @@ -388,7 +388,7 @@ fn check_names(checker: &mut Checker, decorator: &Decorator, expr: &Expr) {
let name_range = get_parametrize_name_range(
decorator,
expr,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(expr.range());
Expand Down Expand Up @@ -681,7 +681,7 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) {
let element_end =
trailing_comma(element, checker.locator().contents(), values_end);
let deletion_range = TextRange::new(previous_end, element_end);
if !checker.parsed().comment_ranges().intersects(deletion_range) {
if !checker.comment_ranges().intersects(deletion_range) {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_deletion(deletion_range)));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,6 @@ pub(crate) fn compare_with_tuple(checker: &mut Checker, expr: &Expr) {

// Avoid removing comments.
if checker
.parsed()
.comment_ranges()
.has_comments(expr, checker.locator())
{
Expand Down Expand Up @@ -779,7 +778,7 @@ fn is_short_circuit(
parenthesized_range(
furthest.into(),
expr.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(furthest.range())
Expand Down Expand Up @@ -807,7 +806,7 @@ fn is_short_circuit(
parenthesized_range(
furthest.into(),
expr.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(furthest.range())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ pub(crate) fn if_expr_with_true_false(
parenthesized_range(
test.into(),
expr.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(test.range()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@ pub(crate) fn multiple_with_statements(
TextRange::new(with_stmt.start(), colon.end()),
);
if !checker
.parsed()
.comment_ranges()
.intersects(TextRange::new(with_stmt.start(), with_stmt.body[0].start()))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ pub(crate) fn nested_if_statements(
);
// The fixer preserves comments in the nested body, but removes comments between
// the outer and inner if statements.
if !checker.parsed().comment_ranges().intersects(TextRange::new(
if !checker.comment_ranges().intersects(TextRange::new(
nested_if.start(),
nested_if.body()[0].start(),
)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ pub(crate) fn if_else_block_instead_of_dict_get(checker: &mut Checker, stmt_if:
stmt_if.range(),
);
if !checker
.parsed()
.comment_ranges()
.has_comments(stmt_if, checker.locator())
{
Expand Down Expand Up @@ -300,7 +299,6 @@ pub(crate) fn if_exp_instead_of_dict_get(
expr.range(),
);
if !checker
.parsed()
.comment_ranges()
.has_comments(expr, checker.locator())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ pub(crate) fn if_else_block_instead_of_if_exp(checker: &mut Checker, stmt_if: &a
stmt_if.range(),
);
if !checker
.parsed()
.comment_ranges()
.has_comments(stmt_if, checker.locator())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,11 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) {

// ...and the same comments
let first_comments = checker
.parsed()
.comment_ranges()
.comments_in_range(body_range(&current_branch, checker.locator()))
.iter()
.map(|range| checker.locator().slice(*range));
let second_comments = checker
.parsed()
.comment_ranges()
.comments_in_range(body_range(following_branch, checker.locator()))
.iter()
Expand All @@ -99,7 +97,7 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) {
&current_branch,
following_branch,
checker.locator(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
)
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ fn key_in_dict(
let left_range = parenthesized_range(
left.into(),
parent,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(left.range());
let right_range = parenthesized_range(
right.into(),
parent,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(right.range());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {

// Generate the replacement condition.
let condition = if checker
.parsed()
.comment_ranges()
.has_comments(&range, checker.locator())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ pub(crate) fn suppressible_exception(
stmt.range(),
);
if !checker
.parsed()
.comment_ranges()
.has_comments(stmt, checker.locator())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ pub(crate) fn inplace_argument(checker: &mut Checker, call: &ast::ExprCall) {
call,
keyword,
statement,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator(),
) {
diagnostic.set_fix(fix);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ pub(crate) fn literal_comparisons(checker: &mut Checker, compare: &ast::ExprComp
&ops,
&compare.comparators,
compare.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator(),
);
for diagnostic in &mut diagnostics {
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/pycodestyle/rules/not_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) {
&[CmpOp::NotIn],
comparators,
unary_op.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator(),
),
unary_op.range(),
Expand All @@ -125,7 +125,7 @@ pub(crate) fn not_tests(checker: &mut Checker, unary_op: &ast::ExprUnaryOp) {
&[CmpOp::IsNot],
comparators,
unary_op.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator(),
),
unary_op.range(),
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,15 @@ pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) {
parenthesized_range(
dict.value(i - 1).into(),
dict.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or_else(|| dict.value(i - 1).range())
.end(),
parenthesized_range(
dict.value(i).into(),
dict.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or_else(|| dict.value(i).range())
Expand All @@ -201,15 +201,15 @@ pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) {
parenthesized_range(
dict.value(i - 1).into(),
dict.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or_else(|| dict.value(i - 1).range())
.end(),
parenthesized_range(
dict.value(i).into(),
dict.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or_else(|| dict.value(i).range())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ fn remove_unused_variable(binding: &Binding, checker: &Checker) -> Option<Fix> {
let start = parenthesized_range(
target.into(),
statement.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(target.range())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) {
parenthesized_range(
body_target.into(),
body.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents()
)
.unwrap_or(body_target.range())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,6 @@ pub(crate) fn nested_min_max(
}) {
let mut diagnostic = Diagnostic::new(NestedMinMax { func: min_max }, expr.range());
if !checker
.parsed()
.comment_ranges()
.has_comments(expr, checker.locator())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub(crate) fn subprocess_run_without_check(checker: &mut Checker, call: &ast::Ex
add_argument(
"check=False",
&call.arguments,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
),
// If the function call contains `**kwargs`, mark the fix as unsafe.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ fn generate_keyword_fix(checker: &Checker, call: &ast::ExprCall) -> Fix {
}))
),
&call.arguments,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
))
}
Expand All @@ -190,7 +190,7 @@ fn generate_import_fix(checker: &Checker, call: &ast::ExprCall) -> Result<Fix> {
let argument_edit = add_argument(
&format!("encoding={binding}(False)"),
&call.arguments,
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
);
Ok(Fix::unsafe_edits(import_edit, [argument_edit]))
Expand Down
5 changes: 1 addition & 4 deletions crates/ruff_linter/src/rules/pyupgrade/rules/f_strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,10 +512,7 @@ pub(crate) fn f_strings(checker: &mut Checker, call: &ast::ExprCall, summary: &F
// 0, # 0
// )
// ```
let has_comments = checker
.parsed()
.comment_ranges()
.intersects(call.arguments.range());
let has_comments = checker.comment_ranges().intersects(call.arguments.range());

if !has_comments {
if contents.is_empty() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ pub(crate) fn yield_in_for_loop(checker: &mut Checker, stmt_for: &ast::StmtFor)
parenthesized_range(
iter.as_ref().into(),
stmt_for.into(),
checker.parsed().comment_ranges(),
checker.comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(iter.range()),
Expand Down
Loading

0 comments on commit 2e0a975

Please sign in to comment.