From 2fb6b320d8a756390a2cf6ec9289e141af1a0ad2 Mon Sep 17 00:00:00 2001 From: TomerBin Date: Sat, 21 Dec 2024 16:45:28 +0200 Subject: [PATCH] Use TypeChecker for detecting fastapi routes (#15093) --- .../test/fixtures/fastapi/FAST001.py | 15 +++++++++ .../resources/test/fixtures/ruff/RUF029.py | 11 ++++++- .../src/rules/fastapi/rules/mod.rs | 16 ++++----- ...i-redundant-response-model_FAST001.py.snap | 24 ++++++++++++-- .../src/analyze/typing.rs | 33 +++++++++++++++++++ 5 files changed, 88 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST001.py b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST001.py index 0563e5c5f9..b425a5de91 100644 --- a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST001.py +++ b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST001.py @@ -108,3 +108,18 @@ app = None @app.post("/items/", response_model=Item) async def create_item(item: Item) -> Item: return item + + +# Routes might be defined inside functions + + +def setup_app(app_arg: FastAPI, non_app: str) -> None: + # Error + @app_arg.get("/", response_model=str) + async def get_root() -> str: + return "Hello World!" + + # Ok + @non_app.get("/", response_model=str) + async def get_root() -> str: + return "Hello World!" diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF029.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF029.py index 47651079fa..c65ef354b0 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF029.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF029.py @@ -80,11 +80,20 @@ async def test() -> str: return ",".join(vals) +# FastApi routes can be async without actually using await + from fastapi import FastAPI app = FastAPI() @app.post("/count") -async def fastapi_route(): # Ok: FastApi routes can be async without actually using await +async def fastapi_route(): return 1 + + +def setup_app(app_arg: FastAPI, non_app: str) -> None: + @app_arg.get("/") + async def get_root() -> str: + return "Hello World!" + diff --git a/crates/ruff_linter/src/rules/fastapi/rules/mod.rs b/crates/ruff_linter/src/rules/fastapi/rules/mod.rs index 43dd60cecc..034737fa12 100644 --- a/crates/ruff_linter/src/rules/fastapi/rules/mod.rs +++ b/crates/ruff_linter/src/rules/fastapi/rules/mod.rs @@ -7,7 +7,7 @@ mod fastapi_redundant_response_model; mod fastapi_unused_path_parameter; use ruff_python_ast as ast; -use ruff_python_semantic::analyze::typing::resolve_assignment; +use ruff_python_semantic::analyze::typing; use ruff_python_semantic::SemanticModel; /// Returns `true` if the function is a FastAPI route. @@ -41,11 +41,11 @@ pub(crate) fn is_fastapi_route_call(call_expr: &ast::ExprCall, semantic: &Semant ) { return false; } - - resolve_assignment(value, semantic).is_some_and(|qualified_name| { - matches!( - qualified_name.segments(), - ["fastapi", "FastAPI" | "APIRouter"] - ) - }) + let Some(name) = value.as_name_expr() else { + return false; + }; + let Some(binding_id) = semantic.resolve_name(name) else { + return false; + }; + typing::is_fastapi_route(semantic.binding(binding_id), semantic) } diff --git a/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-redundant-response-model_FAST001.py.snap b/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-redundant-response-model_FAST001.py.snap index 9d630c9a8f..79953ce698 100644 --- a/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-redundant-response-model_FAST001.py.snap +++ b/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-redundant-response-model_FAST001.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/fastapi/mod.rs -snapshot_kind: text --- FAST001.py:17:22: FAST001 [*] FastAPI route with redundant `response_model` argument | @@ -172,4 +171,25 @@ FAST001.py:53:24: FAST001 [*] FastAPI route with redundant `response_model` argu 53 |+@router.get("/items/") 54 54 | async def create_item(item: Item) -> Item: 55 55 | return item -56 56 | +56 56 | + +FAST001.py:118:23: FAST001 [*] FastAPI route with redundant `response_model` argument + | +116 | def setup_app(app_arg: FastAPI, non_app: str) -> None: +117 | # Error +118 | @app_arg.get("/", response_model=str) + | ^^^^^^^^^^^^^^^^^^ FAST001 +119 | async def get_root() -> str: +120 | return "Hello World!" + | + = help: Remove argument + +ℹ Unsafe fix +115 115 | +116 116 | def setup_app(app_arg: FastAPI, non_app: str) -> None: +117 117 | # Error +118 |- @app_arg.get("/", response_model=str) + 118 |+ @app_arg.get("/") +119 119 | async def get_root() -> str: +120 120 | return "Hello World!" +121 121 | diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index b346390a89..ecd59aa002 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -786,6 +786,35 @@ impl TypeChecker for PathlibPathChecker { } } +pub struct FastApiRouteChecker; + +impl FastApiRouteChecker { + fn is_fastapi_route_constructor(semantic: &SemanticModel, expr: &Expr) -> bool { + let Some(qualified_name) = semantic.resolve_qualified_name(expr) else { + return false; + }; + + matches!( + qualified_name.segments(), + ["fastapi", "FastAPI" | "APIRouter"] + ) + } +} + +impl TypeChecker for FastApiRouteChecker { + fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool { + Self::is_fastapi_route_constructor(semantic, annotation) + } + + fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool { + let Expr::Call(ast::ExprCall { func, .. }) = initializer else { + return false; + }; + + Self::is_fastapi_route_constructor(semantic, func) + } +} + pub struct TypeVarLikeChecker; impl TypeVarLikeChecker { @@ -914,6 +943,10 @@ pub fn is_pathlib_path(binding: &Binding, semantic: &SemanticModel) -> bool { check_type::(binding, semantic) } +pub fn is_fastapi_route(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) +} + /// Test whether the given binding is for an old-style `TypeVar`, `TypeVarTuple` or a `ParamSpec`. pub fn is_type_var_like(binding: &Binding, semantic: &SemanticModel) -> bool { check_type::(binding, semantic)