Skip to content

Commit

Permalink
Centralize the parsing of string annotations in a method on Checker
Browse files Browse the repository at this point in the history
  • Loading branch information
AlexWaygood committed Aug 29, 2024
1 parent fdde658 commit 87a0c9c
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 23 deletions.
20 changes: 17 additions & 3 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,22 @@ impl<'a> Checker<'a> {
.map(|node_id| IsolationLevel::Group(node_id.into()))
.unwrap_or_default()
}

/// A thin wrapper over [`ruff_python_parser::typing::parse_type_annotation`]
/// that returns `Some(annotation)` only if the annotation is valid Python syntax.
///
/// In the future this function could possibly add caching to improve performance.
pub(crate) fn parse_string_annotation(
&self,
annotation: &ast::ExprStringLiteral,
) -> Option<(Parsed<ast::ModExpression>, AnnotationKind)> {
let (parsed_annotation, kind) =
parse_type_annotation(annotation, self.locator.contents()).ok()?;
if !parsed_annotation.errors().is_empty() {
return None;
}
Some((parsed_annotation, kind))
}
}

impl<'a> Visitor<'a> for Checker<'a> {
Expand Down Expand Up @@ -2176,9 +2192,7 @@ impl<'a> Checker<'a> {
while !self.visit.string_type_definitions.is_empty() {
let type_definitions = std::mem::take(&mut self.visit.string_type_definitions);
for (string_expr, snapshot) in type_definitions {
if let Ok((parsed_annotation, kind)) =
parse_type_annotation(string_expr, self.locator.contents())
{
if let Some((parsed_annotation, kind)) = self.parse_string_annotation(string_expr) {
let parsed_annotation = allocator.alloc(parsed_annotation);
self.parsed_type_annotation = Some(parsed_annotation);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub(crate) fn any_eq_ne_annotation(checker: &mut Checker, name: &str, parameters
return;
}

if !match_maybe_stringized_annotation(annotation, checker.locator(), |expr| {
if !match_maybe_stringized_annotation(annotation, checker, |expr| {
semantic.match_typing_expr(expr, "Any")
}) {
return;
Expand Down
12 changes: 4 additions & 8 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_python_ast as ast;
use ruff_python_parser::typing::parse_type_annotation;
use ruff_source_file::Locator;

use crate::checkers::ast::Checker;

/// Apply a test to an annotation expression,
/// abstracting over the fact that the annotation expression might be "stringized".
Expand All @@ -9,18 +9,14 @@ use ruff_source_file::Locator;
/// `foo: "typing.Any"` means the same thing to a type checker as `foo: typing.Any`.
pub(super) fn match_maybe_stringized_annotation(
expr: &ast::Expr,
locator: &Locator,
checker: &Checker,
match_fn: impl FnOnce(&ast::Expr) -> bool,
) -> bool {
if let ast::Expr::StringLiteral(string_annotation) = expr {
let Ok((parsed_annotation, _)) =
parse_type_annotation(string_annotation, locator.contents())
let Some((parsed_annotation, _)) = checker.parse_string_annotation(string_annotation)
else {
return false;
};
if !parsed_annotation.errors().is_empty() {
return false;
}
let ast::ModExpression {
body: annotation, ..
} = parsed_annotation.into_syntax();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ use std::fmt;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_semantic::SemanticModel;
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use super::helpers::match_maybe_stringized_annotation;
Expand Down Expand Up @@ -67,7 +65,7 @@ pub(crate) fn no_return_argument_annotation(checker: &mut Checker, parameters: &
.iter()
.filter_map(ast::AnyParameterRef::annotation)
{
if is_no_return(annotation, checker.semantic(), checker.locator()) {
if is_no_return(annotation, checker) {
checker.diagnostics.push(Diagnostic::new(
NoReturnArgumentAnnotationInStub {
module: if checker.settings.target_version >= Py311 {
Expand All @@ -82,9 +80,9 @@ pub(crate) fn no_return_argument_annotation(checker: &mut Checker, parameters: &
}
}

fn is_no_return(expr: &ast::Expr, semantic: &SemanticModel, locator: &Locator) -> bool {
match_maybe_stringized_annotation(expr, locator, |expr| {
semantic.match_typing_expr(expr, "NoReturn")
fn is_no_return(expr: &ast::Expr, checker: &Checker) -> bool {
match_maybe_stringized_annotation(expr, checker, |expr| {
checker.semantic().match_typing_expr(expr, "NoReturn")
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use ruff_python_ast::identifier::Identifier;
use ruff_python_semantic::analyze;
use ruff_python_semantic::analyze::visibility::{is_abstract, is_final, is_overload};
use ruff_python_semantic::{ScopeKind, SemanticModel};
use ruff_source_file::Locator;

use super::helpers::match_maybe_stringized_annotation;
use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -158,7 +157,7 @@ pub(crate) fn non_self_return_type(

// In-place methods that are expected to return `Self`.
if is_inplace_bin_op(name) {
if !is_self(returns, semantic, checker.locator()) {
if !is_self(returns, checker) {
checker.diagnostics.push(Diagnostic::new(
NonSelfReturnType {
class_name: class_def.name.to_string(),
Expand Down Expand Up @@ -243,9 +242,10 @@ fn is_name(expr: &Expr, name: &str) -> bool {
id.as_str() == name
}

fn is_self(expr: &Expr, semantic: &SemanticModel, locator: &Locator) -> bool {
match_maybe_stringized_annotation(expr, locator, |expr| {
semantic.match_typing_expr(expr, "Self")
/// Return `true` if the given expression resolves to `typing.Self`.
fn is_self(expr: &Expr, checker: &Checker) -> bool {
match_maybe_stringized_annotation(expr, checker, |expr| {
checker.semantic().match_typing_expr(expr, "Self")
})
}

Expand Down

0 comments on commit 87a0c9c

Please sign in to comment.