diff --git a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py index 6a3b14838d93e..912ecdda5cbb7 100644 --- a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py +++ b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py @@ -1,6 +1,6 @@ from typing import Annotated -from fastapi import FastAPI, Path +from fastapi import Depends, FastAPI, Path app = FastAPI() @@ -144,3 +144,35 @@ async def read_thing(query: str): @app.get("/things/{thing_id=}") async def read_thing(query: str): return {"query": query} + + +# https://github.com/astral-sh/ruff/issues/13657 +def takes_thing_id(thing_id): ... +def something_else(lorem): ... + +from foo import unknown_imported +unknown_not_function = unknown_imported() + + +### Errors +@app.get("/things/{thing_id}") +async def single(other: Annotated[str, Depends(something_else)]): ... +@app.get("/things/{thing_id}") +async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... +@app.get("/things/{thing_id}") +async def default(other: str = Depends(something_else)): ... + + +### No errors +@app.get("/things/{thing_id}") +async def single(other: Annotated[str, Depends(takes_thing_id)]): ... +@app.get("/things/{thing_id}") +async def double(other: Annotated[str, Depends(takes_thing_id), Depends(something_else)]): ... +@app.get("/things/{thing_id}") +async def default(other: str = Depends(takes_thing_id)): ... +@app.get("/things/{thing_id}") +async def unknown_1(other: str = Depends(unknown_unresolved)): ... +@app.get("/things/{thing_id}") +async def unknown_2(other: str = Depends(unknown_not_function)): ... +@app.get("/things/{thing_id}") +async def unknown_3(other: str = Depends(unknown_imported)): ... diff --git a/crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs b/crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs index 779fa9835eaf3..c486f44ef5abb 100644 --- a/crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs +++ b/crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs @@ -6,8 +6,8 @@ use ruff_diagnostics::Fix; use ruff_diagnostics::{Diagnostic, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast as ast; -use ruff_python_ast::{Expr, Parameter, ParameterWithDefault}; -use ruff_python_semantic::{Modules, SemanticModel}; +use ruff_python_ast::{Expr, ExprCall, ExprSubscript, Parameter, ParameterWithDefault}; +use ruff_python_semantic::{BindingKind, Modules, ScopeKind, SemanticModel}; use ruff_python_stdlib::identifiers::is_identifier; use ruff_text_size::{Ranged, TextSize}; @@ -137,16 +137,29 @@ pub(crate) fn fastapi_unused_path_parameter( let path_params = PathParamIterator::new(path.to_str()); // Extract the arguments from the function signature - let named_args: Vec<_> = function_def - .parameters - .args - .iter() - .chain(&function_def.parameters.kwonlyargs) - .map(|ParameterWithDefault { parameter, .. }| { - parameter_alias(parameter, checker.semantic()) - .unwrap_or_else(|| parameter.name.as_str()) - }) - .collect(); + let mut named_args = vec![]; + + for parameter_with_default in non_posonly_non_variadic_parameters(function_def) { + let ParameterWithDefault { parameter, .. } = parameter_with_default; + + if let Some(alias) = parameter_alias(parameter, checker.semantic()) { + named_args.push(alias.to_string()); + } else { + named_args.push(parameter.name.to_string()); + } + + let Some(dependency) = + Dependency::from_parameter(parameter_with_default, checker.semantic()) + else { + continue; + }; + + if let Some(parameter_names) = dependency.parameter_names() { + named_args.extend(parameter_names); + } else { + return; + } + } // Check if any of the path parameters are not in the function signature. let mut diagnostics = vec![]; @@ -156,8 +169,9 @@ pub(crate) fn fastapi_unused_path_parameter( continue; } - // If the path parameter is already in the function signature, we don't need to do anything. - if named_args.contains(&path_param) { + // If the path parameter is already in the function or the dependency signature, + // we don't need to do anything. + if named_args.contains(&path_param.to_string()) { continue; } @@ -194,6 +208,126 @@ pub(crate) fn fastapi_unused_path_parameter( checker.diagnostics.extend(diagnostics); } +fn non_posonly_non_variadic_parameters( + function_def: &ast::StmtFunctionDef, +) -> impl Iterator { + function_def + .parameters + .args + .iter() + .chain(&function_def.parameters.kwonlyargs) +} + +#[derive(Debug, is_macro::Is)] +enum Dependency { + /// Not defined in the same file, or otherwise cannot be determined to be a function. + Unknown, + /// A function defined in the same file, whose parameter names are as given. + Function(Vec), +} + +impl Dependency { + fn parameter_names(self) -> Option> { + match self { + Self::Unknown => None, + Self::Function(parameter_names) => Some(parameter_names), + } + } +} + +impl Dependency { + /// Return `foo` in the first metadata `Depends(foo)`, + /// or that of the default value: + /// + /// ```python + /// def _( + /// a: Annotated[str, Depends(foo), Depends(bar)], + /// # ^^^^^^^^^^^^ + /// a: str = Depends(foo), + /// # ^^^^^^^^^^^^ + /// ): ... + /// ``` + fn from_parameter( + parameter_with_default: &ParameterWithDefault, + semantic: &SemanticModel, + ) -> Option { + let ParameterWithDefault { + parameter, default, .. + } = parameter_with_default; + + if let Some(default) = default { + if let Some(dependency) = Self::from_call(default.as_ref(), semantic) { + return Some(dependency); + }; + } + + let Expr::Subscript(ExprSubscript { value, slice, .. }) = + ¶meter.annotation.as_deref()? + else { + return None; + }; + + if !semantic.match_typing_expr(value, "Annotated") { + return None; + } + + let Expr::Tuple(tuple) = slice.as_ref() else { + return None; + }; + + tuple + .elts + .iter() + .skip(1) + .find_map(|metadata| Self::from_call(metadata, semantic)) + } + + fn from_call(expr: &Expr, semantic: &SemanticModel) -> Option { + let Expr::Call(ExprCall { + func, arguments, .. + }) = expr + else { + return None; + }; + + if !is_fastapi_depends(func.as_ref(), semantic) { + return None; + } + + let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { + return None; + }; + + let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else { + return Some(Self::Unknown); + }; + + let BindingKind::FunctionDefinition(scope_id) = binding.kind else { + return Some(Self::Unknown); + }; + + let scope = &semantic.scopes[scope_id]; + + let ScopeKind::Function(function_def) = scope.kind else { + return Some(Self::Unknown); + }; + + let parameter_names = non_posonly_non_variadic_parameters(function_def) + .map(|ParameterWithDefault { parameter, .. }| parameter.name.to_string()) + .collect::>(); + + Some(Self::Function(parameter_names)) + } +} + +fn is_fastapi_depends(expr: &Expr, semantic: &SemanticModel) -> bool { + let Some(qualified_name) = semantic.resolve_qualified_name(expr) else { + return false; + }; + + matches!(qualified_name.segments(), ["fastapi", "Depends"]) +} + /// Extract the expected in-route name for a given parameter, if it has an alias. /// For example, given `document_id: Annotated[str, Path(alias="documentId")]`, returns `"documentId"`. fn parameter_alias<'a>(parameter: &'a Parameter, semantic: &SemanticModel) -> Option<&'a str> { diff --git a/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-unused-path-parameter_FAST003.py.snap b/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-unused-path-parameter_FAST003.py.snap index 9178c4c878cd0..aa603dad30fdc 100644 --- a/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-unused-path-parameter_FAST003.py.snap +++ b/crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-unused-path-parameter_FAST003.py.snap @@ -340,4 +340,65 @@ FAST003.py:87:18: FAST003 [*] Parameter `name` appears in route path, but not in 88 |+async def read_thing(name, *, author: Annotated[str, Path(alias="author_name")], title: str): 89 89 | return {"author": author, "title": title} 90 90 | -91 91 | +91 91 | + +FAST003.py:158:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `single` signature + | +157 | ### Errors +158 | @app.get("/things/{thing_id}") + | ^^^^^^^^^^ FAST003 +159 | async def single(other: Annotated[str, Depends(something_else)]): ... +160 | @app.get("/things/{thing_id}") + | + = help: Add `thing_id` to function signature + +ℹ Unsafe fix +156 156 | +157 157 | ### Errors +158 158 | @app.get("/things/{thing_id}") +159 |-async def single(other: Annotated[str, Depends(something_else)]): ... + 159 |+async def single(other: Annotated[str, Depends(something_else)], thing_id): ... +160 160 | @app.get("/things/{thing_id}") +161 161 | async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... +162 162 | @app.get("/things/{thing_id}") + +FAST003.py:160:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `double` signature + | +158 | @app.get("/things/{thing_id}") +159 | async def single(other: Annotated[str, Depends(something_else)]): ... +160 | @app.get("/things/{thing_id}") + | ^^^^^^^^^^ FAST003 +161 | async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... +162 | @app.get("/things/{thing_id}") + | + = help: Add `thing_id` to function signature + +ℹ Unsafe fix +158 158 | @app.get("/things/{thing_id}") +159 159 | async def single(other: Annotated[str, Depends(something_else)]): ... +160 160 | @app.get("/things/{thing_id}") +161 |-async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... + 161 |+async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)], thing_id): ... +162 162 | @app.get("/things/{thing_id}") +163 163 | async def default(other: str = Depends(something_else)): ... +164 164 | + +FAST003.py:162:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `default` signature + | +160 | @app.get("/things/{thing_id}") +161 | async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... +162 | @app.get("/things/{thing_id}") + | ^^^^^^^^^^ FAST003 +163 | async def default(other: str = Depends(something_else)): ... + | + = help: Add `thing_id` to function signature + +ℹ Unsafe fix +160 160 | @app.get("/things/{thing_id}") +161 161 | async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... +162 162 | @app.get("/things/{thing_id}") +163 |-async def default(other: str = Depends(something_else)): ... + 163 |+async def default(thing_id, other: str = Depends(something_else)): ... +164 164 | +165 165 | +166 166 | ### No errors