From 0b99611384a47a3654467654f32093342ca437da Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Thu, 9 Jan 2025 02:55:51 +0000 Subject: [PATCH 01/11] [`fastapi`] Handle parameters with `Depends` correctly (`FAST003`) --- .../test/fixtures/fastapi/FAST003.py | 34 +++- .../rules/fastapi_unused_path_parameter.rs | 160 +++++++++++++++++- ...-api-unused-path-parameter_FAST003.py.snap | 63 ++++++- 3 files changed, 248 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py index 6a3b14838d93ee..c04c2ecb5ad476 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 dependable(thing_id): ... +def not_so_dependable(lorem): ... + +from foo import unknown_imported +unknown_not_function = unknown_imported() + + +### Errors +@app.get("/things/{thing_id}") +async def single(other: Annotated[str, Depends(not_so_dependable)]): ... +@app.get("/things/{thing_id}") +async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... +@app.get("/things/{thing_id}") +async def default(other: str = Depends(not_so_dependable)): ... + + +### No errors +@app.get("/things/{thing_id}") +async def single(other: Annotated[str, Depends(dependable)]): ... +@app.get("/things/{thing_id}") +async def double(other: Annotated[str, Depends(dependable), Depends(not_so_dependable)]): ... +@app.get("/things/{thing_id}") +async def default(other: str = Depends(dependable)): ... +@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 779fa9835eaf35..725fba9a270017 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}; @@ -136,12 +136,26 @@ pub(crate) fn fastapi_unused_path_parameter( // Extract the path parameters from the route path. let path_params = PathParamIterator::new(path.to_str()); + let dependables = keywordable_parameters(function_def) + .filter_map(|parameter_with_default| { + match Dependable::from_parameter(parameter_with_default, checker.semantic()) { + Dependable::None => None, + dependable => Some(dependable), + } + }) + .collect::>(); + + if dependables.iter().any(Dependable::is_unknown) { + return; + } + + let dependables_parameter_names: Vec<_> = dependables + .into_iter() + .flat_map(Dependable::parameter_names) + .collect(); + // Extract the arguments from the function signature - let named_args: Vec<_> = function_def - .parameters - .args - .iter() - .chain(&function_def.parameters.kwonlyargs) + let named_args: Vec<_> = keywordable_parameters(function_def) .map(|ParameterWithDefault { parameter, .. }| { parameter_alias(parameter, checker.semantic()) .unwrap_or_else(|| parameter.name.as_str()) @@ -161,6 +175,10 @@ pub(crate) fn fastapi_unused_path_parameter( continue; } + if dependables_parameter_names.contains(&path_param.to_string()) { + continue; + } + // Determine whether the path parameter is used as a positional-only argument. In this case, // the path parameter injection won't work, but we also can't fix it (yet), since we'd need // to make the parameter non-positional-only. @@ -194,6 +212,134 @@ pub(crate) fn fastapi_unused_path_parameter( checker.diagnostics.extend(diagnostics); } +fn keywordable_parameters( + function_def: &ast::StmtFunctionDef, +) -> impl Iterator { + function_def + .parameters + .args + .iter() + .chain(&function_def.parameters.kwonlyargs) +} + +#[derive(Debug, is_macro::Is)] +enum Dependable { + /// `Depends` call not found or invalid. + None, + /// 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 Dependable { + fn parameter_names(self) -> Vec { + match self { + Self::None | Self::Unknown => vec![], + Self::Function(parameter_names) => parameter_names, + } + } +} + +impl Dependable { + /// 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, + ) -> Self { + let ParameterWithDefault { + parameter, default, .. + } = parameter_with_default; + + if let Some(default) = default { + let dependable = Self::from_call(default.as_ref(), semantic); + + if !matches!(dependable, Self::None) { + return dependable; + } + } + + let Some(annotation) = ¶meter.annotation else { + return Dependable::None; + }; + let Expr::Subscript(ExprSubscript { value, slice, .. }) = annotation.as_ref() else { + return Dependable::None; + }; + + if !semantic.match_typing_expr(value, "Annotated") { + return Dependable::None; + } + + match slice.as_ref() { + Expr::Tuple(tuple) => tuple + .elts + .iter() + .skip(1) + .find_map(|metadata| match Self::from_call(metadata, semantic) { + Self::None => None, + dependable => Some(dependable), + }) + .unwrap_or(Self::None), + _ => Dependable::None, + } + } + + fn from_call(expr: &Expr, semantic: &SemanticModel) -> Self { + let Expr::Call(ExprCall { + func, arguments, .. + }) = expr + else { + return Self::None; + }; + + if !is_fastapi_depends(func.as_ref(), semantic) { + return Self::None; + } + + let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { + return Self::None; + }; + + let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else { + return Self::Unknown; + }; + + let BindingKind::FunctionDefinition(scope_id) = binding.kind else { + return Self::Unknown; + }; + + let scope = &semantic.scopes.raw[scope_id.as_u32() as usize]; + + let ScopeKind::Function(function_def) = scope.kind else { + return Self::Unknown; + }; + + let parameter_names = keywordable_parameters(function_def) + .map(|ParameterWithDefault { parameter, .. }| parameter.name.to_string()) + .collect::>(); + + 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 9178c4c878cd01..a236c67e380b29 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(not_so_dependable)]): ... +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(not_so_dependable)]): ... + 159 |+async def single(other: Annotated[str, Depends(not_so_dependable)], thing_id): ... +160 160 | @app.get("/things/{thing_id}") +161 161 | async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)]): ... +160 | @app.get("/things/{thing_id}") + | ^^^^^^^^^^ FAST003 +161 | async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)]): ... +160 160 | @app.get("/things/{thing_id}") +161 |-async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... + 161 |+async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)], thing_id): ... +162 162 | @app.get("/things/{thing_id}") +163 163 | async def default(other: str = Depends(not_so_dependable)): ... +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(not_so_dependable), Depends(dependable)]): ... +162 | @app.get("/things/{thing_id}") + | ^^^^^^^^^^ FAST003 +163 | async def default(other: str = Depends(not_so_dependable)): ... + | + = 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(not_so_dependable), Depends(dependable)]): ... +162 162 | @app.get("/things/{thing_id}") +163 |-async def default(other: str = Depends(not_so_dependable)): ... + 163 |+async def default(thing_id, other: str = Depends(not_so_dependable)): ... +164 164 | +165 165 | +166 166 | ### No errors From c32894ba4e269cdcc7b07da3f923a00b13630695 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Thu, 9 Jan 2025 12:21:21 +0000 Subject: [PATCH 02/11] Per review --- .../rules/fastapi_unused_path_parameter.rs | 89 ++++++++----------- 1 file changed, 38 insertions(+), 51 deletions(-) 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 725fba9a270017..631c71b2f8ea3d 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 @@ -136,23 +136,19 @@ pub(crate) fn fastapi_unused_path_parameter( // Extract the path parameters from the route path. let path_params = PathParamIterator::new(path.to_str()); - let dependables = keywordable_parameters(function_def) - .filter_map(|parameter_with_default| { - match Dependable::from_parameter(parameter_with_default, checker.semantic()) { - Dependable::None => None, - dependable => Some(dependable), - } - }) - .collect::>(); + let dependencies = keywordable_parameters(function_def).filter_map(|parameter_with_default| { + Dependency::from_parameter(parameter_with_default, checker.semantic()) + }); - if dependables.iter().any(Dependable::is_unknown) { - return; - } + let mut dependencies_parameter_names = vec![]; - let dependables_parameter_names: Vec<_> = dependables - .into_iter() - .flat_map(Dependable::parameter_names) - .collect(); + for dependency in dependencies { + if let Some(parameter_names) = dependency.parameter_names() { + dependencies_parameter_names.extend(parameter_names); + } else { + return; + } + } // Extract the arguments from the function signature let named_args: Vec<_> = keywordable_parameters(function_def) @@ -175,7 +171,7 @@ pub(crate) fn fastapi_unused_path_parameter( continue; } - if dependables_parameter_names.contains(&path_param.to_string()) { + if dependencies_parameter_names.contains(&path_param.to_string()) { continue; } @@ -223,25 +219,23 @@ fn keywordable_parameters( } #[derive(Debug, is_macro::Is)] -enum Dependable { - /// `Depends` call not found or invalid. - None, +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 Dependable { - fn parameter_names(self) -> Vec { +impl Dependency { + fn parameter_names(self) -> Option> { match self { - Self::None | Self::Unknown => vec![], - Self::Function(parameter_names) => parameter_names, + Self::Unknown => None, + Self::Function(parameter_names) => Some(parameter_names), } } } -impl Dependable { +impl Dependency { /// Return `foo` in the first metadata `Depends(foo)`, /// or that of the default value: /// @@ -256,28 +250,25 @@ impl Dependable { fn from_parameter( parameter_with_default: &ParameterWithDefault, semantic: &SemanticModel, - ) -> Self { + ) -> Option { let ParameterWithDefault { parameter, default, .. } = parameter_with_default; if let Some(default) = default { - let dependable = Self::from_call(default.as_ref(), semantic); - - if !matches!(dependable, Self::None) { - return dependable; - } + if let Some(dependency) = Self::from_call(default.as_ref(), semantic) { + return Some(dependency); + }; } - let Some(annotation) = ¶meter.annotation else { - return Dependable::None; - }; - let Expr::Subscript(ExprSubscript { value, slice, .. }) = annotation.as_ref() else { - return Dependable::None; + let Expr::Subscript(ExprSubscript { value, slice, .. }) = + ¶meter.annotation.as_deref()? + else { + return None; }; if !semantic.match_typing_expr(value, "Annotated") { - return Dependable::None; + return None; } match slice.as_ref() { @@ -285,50 +276,46 @@ impl Dependable { .elts .iter() .skip(1) - .find_map(|metadata| match Self::from_call(metadata, semantic) { - Self::None => None, - dependable => Some(dependable), - }) - .unwrap_or(Self::None), - _ => Dependable::None, + .find_map(|metadata| Self::from_call(metadata, semantic)), + _ => None, } } - fn from_call(expr: &Expr, semantic: &SemanticModel) -> Self { + fn from_call(expr: &Expr, semantic: &SemanticModel) -> Option { let Expr::Call(ExprCall { func, arguments, .. }) = expr else { - return Self::None; + return None; }; if !is_fastapi_depends(func.as_ref(), semantic) { - return Self::None; + return None; } let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { - return Self::None; + return None; }; let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else { - return Self::Unknown; + return Some(Self::Unknown); }; let BindingKind::FunctionDefinition(scope_id) = binding.kind else { - return Self::Unknown; + return Some(Self::Unknown); }; - let scope = &semantic.scopes.raw[scope_id.as_u32() as usize]; + let scope = &semantic.scopes[scope_id]; let ScopeKind::Function(function_def) = scope.kind else { - return Self::Unknown; + return Some(Self::Unknown); }; let parameter_names = keywordable_parameters(function_def) .map(|ParameterWithDefault { parameter, .. }| parameter.name.to_string()) .collect::>(); - Self::Function(parameter_names) + Some(Self::Function(parameter_names)) } } From 28b065e67e1b902c4cf7ff4fb779738a089d63f9 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Thu, 9 Jan 2025 12:36:44 +0000 Subject: [PATCH 03/11] Per review --- .../rules/fastapi_unused_path_parameter.rs | 61 ++++++++++--------- 1 file changed, 31 insertions(+), 30 deletions(-) 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 631c71b2f8ea3d..c486f44ef5abb8 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 @@ -136,28 +136,31 @@ pub(crate) fn fastapi_unused_path_parameter( // Extract the path parameters from the route path. let path_params = PathParamIterator::new(path.to_str()); - let dependencies = keywordable_parameters(function_def).filter_map(|parameter_with_default| { - Dependency::from_parameter(parameter_with_default, checker.semantic()) - }); + // Extract the arguments from the function signature + let mut named_args = vec![]; - let mut dependencies_parameter_names = 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; + }; - for dependency in dependencies { if let Some(parameter_names) = dependency.parameter_names() { - dependencies_parameter_names.extend(parameter_names); + named_args.extend(parameter_names); } else { return; } } - // Extract the arguments from the function signature - let named_args: Vec<_> = keywordable_parameters(function_def) - .map(|ParameterWithDefault { parameter, .. }| { - parameter_alias(parameter, checker.semantic()) - .unwrap_or_else(|| parameter.name.as_str()) - }) - .collect(); - // Check if any of the path parameters are not in the function signature. let mut diagnostics = vec![]; for (path_param, range) in path_params { @@ -166,12 +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) { - continue; - } - - if dependencies_parameter_names.contains(&path_param.to_string()) { + // 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; } @@ -208,7 +208,7 @@ pub(crate) fn fastapi_unused_path_parameter( checker.diagnostics.extend(diagnostics); } -fn keywordable_parameters( +fn non_posonly_non_variadic_parameters( function_def: &ast::StmtFunctionDef, ) -> impl Iterator { function_def @@ -271,14 +271,15 @@ impl Dependency { return None; } - match slice.as_ref() { - Expr::Tuple(tuple) => tuple - .elts - .iter() - .skip(1) - .find_map(|metadata| Self::from_call(metadata, semantic)), - _ => 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 { @@ -311,7 +312,7 @@ impl Dependency { return Some(Self::Unknown); }; - let parameter_names = keywordable_parameters(function_def) + let parameter_names = non_posonly_non_variadic_parameters(function_def) .map(|ParameterWithDefault { parameter, .. }| parameter.name.to_string()) .collect::>(); From d5fd7c5019744f0a70b2b3fa8082ac9e9958181f Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Thu, 9 Jan 2025 14:52:49 +0000 Subject: [PATCH 04/11] Per review --- .../test/fixtures/fastapi/FAST003.py | 16 +++++----- ...-api-unused-path-parameter_FAST003.py.snap | 30 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py index c04c2ecb5ad476..912ecdda5cbb77 100644 --- a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py +++ b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py @@ -147,8 +147,8 @@ async def read_thing(query: str): # https://github.com/astral-sh/ruff/issues/13657 -def dependable(thing_id): ... -def not_so_dependable(lorem): ... +def takes_thing_id(thing_id): ... +def something_else(lorem): ... from foo import unknown_imported unknown_not_function = unknown_imported() @@ -156,20 +156,20 @@ def not_so_dependable(lorem): ... ### Errors @app.get("/things/{thing_id}") -async def single(other: Annotated[str, Depends(not_so_dependable)]): ... +async def single(other: Annotated[str, Depends(something_else)]): ... @app.get("/things/{thing_id}") -async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... +async def double(other: Annotated[str, Depends(something_else), Depends(takes_thing_id)]): ... @app.get("/things/{thing_id}") -async def default(other: str = Depends(not_so_dependable)): ... +async def default(other: str = Depends(something_else)): ... ### No errors @app.get("/things/{thing_id}") -async def single(other: Annotated[str, Depends(dependable)]): ... +async def single(other: Annotated[str, Depends(takes_thing_id)]): ... @app.get("/things/{thing_id}") -async def double(other: Annotated[str, Depends(dependable), Depends(not_so_dependable)]): ... +async def double(other: Annotated[str, Depends(takes_thing_id), Depends(something_else)]): ... @app.get("/things/{thing_id}") -async def default(other: str = Depends(dependable)): ... +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}") 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 a236c67e380b29..aa603dad30fdc5 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 @@ -347,7 +347,7 @@ FAST003.py:158:19: FAST003 [*] Parameter `thing_id` appears in route path, but n 157 | ### Errors 158 | @app.get("/things/{thing_id}") | ^^^^^^^^^^ FAST003 -159 | async def single(other: Annotated[str, Depends(not_so_dependable)]): ... +159 | async def single(other: Annotated[str, Depends(something_else)]): ... 160 | @app.get("/things/{thing_id}") | = help: Add `thing_id` to function signature @@ -356,49 +356,49 @@ FAST003.py:158:19: FAST003 [*] Parameter `thing_id` appears in route path, but n 156 156 | 157 157 | ### Errors 158 158 | @app.get("/things/{thing_id}") -159 |-async def single(other: Annotated[str, Depends(not_so_dependable)]): ... - 159 |+async def single(other: Annotated[str, Depends(not_so_dependable)], 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(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)]): ... +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(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)]): ... +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(not_so_dependable), Depends(dependable)]): ... - 161 |+async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)], 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(not_so_dependable)): ... +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(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)): ... +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(not_so_dependable), Depends(dependable)]): ... +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(not_so_dependable)): ... - 163 |+async def default(thing_id, other: str = Depends(not_so_dependable)): ... +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 From 7f4aa9f729cd025fa0ae0a2d3de0a136ee07ebdc Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 12 Jan 2025 23:07:45 +0000 Subject: [PATCH 05/11] Ignore parameters with multiple `Depends` --- .../test/fixtures/fastapi/FAST003.py | 6 +- .../rules/fastapi_unused_path_parameter.rs | 57 +++++++++++++------ ...-api-unused-path-parameter_FAST003.py.snap | 40 +++---------- 3 files changed, 53 insertions(+), 50 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py index 912ecdda5cbb77..ef808e733600de 100644 --- a/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py +++ b/crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py @@ -158,15 +158,17 @@ def something_else(lorem): ... @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 +# A parameter with multiple `Depends()` has undefined behaviour. +# https://github.com/astral-sh/ruff/pull/15364#discussion_r1912551710 @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(something_else), 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)): ... 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 c486f44ef5abb8..cba321a0676781 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,7 +6,7 @@ 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, ExprCall, ExprSubscript, Parameter, ParameterWithDefault}; +use ruff_python_ast::{Arguments, 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}; @@ -224,12 +224,15 @@ enum Dependency { Unknown, /// A function defined in the same file, whose parameter names are as given. Function(Vec), + /// There are multiple `Depends()` calls. + Multiple, } impl Dependency { fn parameter_names(self) -> Option> { match self { Self::Unknown => None, + Self::Multiple => None, Self::Function(parameter_names) => Some(parameter_names), } } @@ -256,7 +259,7 @@ impl Dependency { } = parameter_with_default; if let Some(default) = default { - if let Some(dependency) = Self::from_call(default.as_ref(), semantic) { + if let Some(dependency) = Self::from_default(default, semantic) { return Some(dependency); }; } @@ -275,25 +278,30 @@ impl Dependency { return None; }; - tuple - .elts - .iter() - .skip(1) - .find_map(|metadata| Self::from_call(metadata, semantic)) - } + let mut dependency = None; - fn from_call(expr: &Expr, semantic: &SemanticModel) -> Option { - let Expr::Call(ExprCall { - func, arguments, .. - }) = expr - else { - return None; - }; + for metadata_element in tuple.elts.iter().skip(1) { + let Some(arguments) = depends_arguments(metadata_element, semantic) else { + continue; + }; - if !is_fastapi_depends(func.as_ref(), semantic) { - return None; + if dependency.is_some() { + return Some(Self::Multiple); + } else { + dependency = Self::from_depends_call(arguments, semantic); + } } + dependency + } + + fn from_default(expr: &Expr, semantic: &SemanticModel) -> Option { + let arguments = depends_arguments(expr, semantic)?; + + Self::from_depends_call(arguments, semantic) + } + + fn from_depends_call(arguments: &Arguments, semantic: &SemanticModel) -> Option { let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { return None; }; @@ -320,6 +328,21 @@ impl Dependency { } } +fn depends_arguments<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Arguments> { + let Expr::Call(ExprCall { + func, arguments, .. + }) = expr + else { + return None; + }; + + if !is_fastapi_depends(func.as_ref(), semantic) { + return None; + } + + Some(arguments) +} + fn is_fastapi_depends(expr: &Expr, semantic: &SemanticModel) -> bool { let Some(qualified_name) = semantic.resolve_qualified_name(expr) else { return false; 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 aa603dad30fdc5..9cb210585fd387 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 @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/fastapi/mod.rs -snapshot_kind: text --- FAST003.py:9:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature | @@ -359,17 +358,16 @@ FAST003.py:158:19: FAST003 [*] Parameter `thing_id` appears in route path, but n 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}") +161 161 | async def default(other: str = Depends(something_else)): ... +162 162 | -FAST003.py:160:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `double` signature +FAST003.py:160:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `default` 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}") +161 | async def default(other: str = Depends(something_else)): ... | = help: Add `thing_id` to function signature @@ -377,28 +375,8 @@ FAST003.py:160:19: FAST003 [*] Parameter `thing_id` appears in route path, but n 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 +161 |-async def default(other: str = Depends(something_else)): ... + 161 |+async def default(thing_id, other: str = Depends(something_else)): ... +162 162 | +163 163 | +164 164 | ### No errors From 57747416820b42226e1cca3ee7438670a52156ff Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 12 Jan 2025 23:17:30 +0000 Subject: [PATCH 06/11] Clippy --- .../src/rules/fastapi/rules/fastapi_unused_path_parameter.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 cba321a0676781..d4fbe6977c8955 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 @@ -287,9 +287,9 @@ impl Dependency { if dependency.is_some() { return Some(Self::Multiple); - } else { - dependency = Self::from_depends_call(arguments, semantic); } + + dependency = Self::from_depends_call(arguments, semantic); } dependency From ec8ea3681e0599b04680c73abb84797b87ec0192 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 12 Jan 2025 20:30:35 -0500 Subject: [PATCH 07/11] Remove allocations --- .../rules/fastapi_unused_path_parameter.rs | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) 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 d4fbe6977c8955..bd545da02c1da3 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,7 +6,9 @@ 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::{Arguments, Expr, ExprCall, ExprSubscript, Parameter, ParameterWithDefault}; +use ruff_python_ast::{ + Arguments, Expr, ExprCall, ExprSubscript, Identifier, Parameter, ParameterWithDefault, +}; use ruff_python_semantic::{BindingKind, Modules, ScopeKind, SemanticModel}; use ruff_python_stdlib::identifiers::is_identifier; use ruff_text_size::{Ranged, TextSize}; @@ -143,9 +145,9 @@ pub(crate) fn fastapi_unused_path_parameter( let ParameterWithDefault { parameter, .. } = parameter_with_default; if let Some(alias) = parameter_alias(parameter, checker.semantic()) { - named_args.push(alias.to_string()); + named_args.push(alias); } else { - named_args.push(parameter.name.to_string()); + named_args.push(parameter.name.as_str()); } let Some(dependency) = @@ -155,8 +157,9 @@ pub(crate) fn fastapi_unused_path_parameter( }; if let Some(parameter_names) = dependency.parameter_names() { - named_args.extend(parameter_names); + named_args.extend(parameter_names.iter().copied().map(Identifier::as_str)); } else { + // If we can't determine the dependency, we can't really do anything. return; } } @@ -171,7 +174,7 @@ pub(crate) fn fastapi_unused_path_parameter( // 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()) { + if named_args.contains(&path_param) { continue; } @@ -208,6 +211,7 @@ pub(crate) fn fastapi_unused_path_parameter( checker.diagnostics.extend(diagnostics); } +/// Returns an iterator over the non-positional-only, non-variadic parameters of a function. fn non_posonly_non_variadic_parameters( function_def: &ast::StmtFunctionDef, ) -> impl Iterator { @@ -219,26 +223,26 @@ fn non_posonly_non_variadic_parameters( } #[derive(Debug, is_macro::Is)] -enum Dependency { +enum Dependency<'a> { /// 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), + Function(Vec<&'a Identifier>), /// There are multiple `Depends()` calls. Multiple, } -impl Dependency { - fn parameter_names(self) -> Option> { +impl<'a> Dependency<'a> { + fn parameter_names(&self) -> Option<&[&'a Identifier]> { match self { Self::Unknown => None, Self::Multiple => None, - Self::Function(parameter_names) => Some(parameter_names), + Self::Function(parameter_names) => Some(parameter_names.as_slice()), } } } -impl Dependency { +impl<'a> Dependency<'a> { /// Return `foo` in the first metadata `Depends(foo)`, /// or that of the default value: /// @@ -251,8 +255,8 @@ impl Dependency { /// ): ... /// ``` fn from_parameter( - parameter_with_default: &ParameterWithDefault, - semantic: &SemanticModel, + parameter_with_default: &'a ParameterWithDefault, + semantic: &'a SemanticModel, ) -> Option { let ParameterWithDefault { parameter, default, .. @@ -295,13 +299,13 @@ impl Dependency { dependency } - fn from_default(expr: &Expr, semantic: &SemanticModel) -> Option { + fn from_default(expr: &'a Expr, semantic: &'a SemanticModel) -> Option { let arguments = depends_arguments(expr, semantic)?; Self::from_depends_call(arguments, semantic) } - fn from_depends_call(arguments: &Arguments, semantic: &SemanticModel) -> Option { + fn from_depends_call(arguments: &'a Arguments, semantic: &'a SemanticModel) -> Option { let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { return None; }; @@ -321,8 +325,8 @@ impl Dependency { }; let parameter_names = non_posonly_non_variadic_parameters(function_def) - .map(|ParameterWithDefault { parameter, .. }| parameter.name.to_string()) - .collect::>(); + .map(|ParameterWithDefault { parameter, .. }| ¶meter.name) + .collect(); Some(Self::Function(parameter_names)) } From c061301766fc2241cf02a8edbff71f439b61d3bf Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 13 Jan 2025 09:36:05 +0100 Subject: [PATCH 08/11] Use 'proper' lifetimes --- .../rules/fastapi/rules/fastapi_unused_path_parameter.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 bd545da02c1da3..e3b4b5ec5c2b7b 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 @@ -256,7 +256,7 @@ impl<'a> Dependency<'a> { /// ``` fn from_parameter( parameter_with_default: &'a ParameterWithDefault, - semantic: &'a SemanticModel, + semantic: &SemanticModel<'a>, ) -> Option { let ParameterWithDefault { parameter, default, .. @@ -299,13 +299,13 @@ impl<'a> Dependency<'a> { dependency } - fn from_default(expr: &'a Expr, semantic: &'a SemanticModel) -> Option { + fn from_default(expr: &'a Expr, semantic: &SemanticModel<'a>) -> Option { let arguments = depends_arguments(expr, semantic)?; Self::from_depends_call(arguments, semantic) } - fn from_depends_call(arguments: &'a Arguments, semantic: &'a SemanticModel) -> Option { + fn from_depends_call(arguments: &'a Arguments, semantic: &SemanticModel<'a>) -> Option { let Some(Expr::Name(name)) = arguments.find_argument_value("dependency", 0) else { return None; }; @@ -332,7 +332,7 @@ impl<'a> Dependency<'a> { } } -fn depends_arguments<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Arguments> { +fn depends_arguments<'a>(expr: &'a Expr, semantic: &SemanticModel) -> Option<&'a Arguments> { let Expr::Call(ExprCall { func, arguments, .. }) = expr From b548cf1fec3635f4f1e6266b17dd730cd6613504 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 13 Jan 2025 09:38:35 +0100 Subject: [PATCH 09/11] Store `&str` instead of `Identifier --- .../fastapi/rules/fastapi_unused_path_parameter.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) 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 e3b4b5ec5c2b7b..b86e11ceecf62e 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,9 +6,7 @@ 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::{ - Arguments, Expr, ExprCall, ExprSubscript, Identifier, Parameter, ParameterWithDefault, -}; +use ruff_python_ast::{Arguments, 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}; @@ -157,7 +155,7 @@ pub(crate) fn fastapi_unused_path_parameter( }; if let Some(parameter_names) = dependency.parameter_names() { - named_args.extend(parameter_names.iter().copied().map(Identifier::as_str)); + named_args.extend_from_slice(parameter_names); } else { // If we can't determine the dependency, we can't really do anything. return; @@ -227,13 +225,13 @@ enum Dependency<'a> { /// 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<&'a Identifier>), + Function(Vec<&'a str>), /// There are multiple `Depends()` calls. Multiple, } impl<'a> Dependency<'a> { - fn parameter_names(&self) -> Option<&[&'a Identifier]> { + fn parameter_names(&self) -> Option<&[&'a str]> { match self { Self::Unknown => None, Self::Multiple => None, @@ -325,7 +323,7 @@ impl<'a> Dependency<'a> { }; let parameter_names = non_posonly_non_variadic_parameters(function_def) - .map(|ParameterWithDefault { parameter, .. }| ¶meter.name) + .map(|ParameterWithDefault { parameter, .. }| &*parameter.name) .collect(); Some(Self::Function(parameter_names)) From 2a036e33de322c512a845e668a04448f13076c37 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 13 Jan 2025 09:44:32 +0100 Subject: [PATCH 10/11] Some smaller nits --- .../rules/fastapi_unused_path_parameter.rs | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) 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 b86e11ceecf62e..282372fdf3c16a 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 @@ -260,10 +260,11 @@ impl<'a> Dependency<'a> { parameter, default, .. } = parameter_with_default; - if let Some(default) = default { - if let Some(dependency) = Self::from_default(default, semantic) { - return Some(dependency); - }; + if let Some(dependency) = default + .as_deref() + .and_then(|default| Self::from_default(default, semantic)) + { + return Some(dependency); } let Expr::Subscript(ExprSubscript { value, slice, .. }) = @@ -280,21 +281,19 @@ impl<'a> Dependency<'a> { return None; }; - let mut dependency = None; + let mut dependencies = tuple.elts.iter().skip(1).filter_map(|metadata_element| { + let arguments = depends_arguments(metadata_element, semantic)?; - for metadata_element in tuple.elts.iter().skip(1) { - let Some(arguments) = depends_arguments(metadata_element, semantic) else { - continue; - }; + Self::from_depends_call(arguments, semantic) + }); - if dependency.is_some() { - return Some(Self::Multiple); - } + let dependency = dependencies.next()?; - dependency = Self::from_depends_call(arguments, semantic); + if dependencies.next().is_some() { + Some(Self::Multiple) + } else { + Some(dependency) } - - dependency } fn from_default(expr: &'a Expr, semantic: &SemanticModel<'a>) -> Option { From 5624fdc1383907d25b007ba408463fe596066c4f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 13 Jan 2025 09:46:15 +0100 Subject: [PATCH 11/11] Improve doc --- .../rules/fastapi/rules/fastapi_unused_path_parameter.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 282372fdf3c16a..d1ea93756cdc43 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 @@ -224,9 +224,14 @@ fn non_posonly_non_variadic_parameters( enum Dependency<'a> { /// 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<&'a str>), + /// There are multiple `Depends()` calls. + /// + /// Multiple `Depends` annotations aren't supported by fastapi and the exact behavior is + /// [unspecified](https://github.com/astral-sh/ruff/pull/15364#discussion_r1912551710). Multiple, } @@ -246,7 +251,7 @@ impl<'a> Dependency<'a> { /// /// ```python /// def _( - /// a: Annotated[str, Depends(foo), Depends(bar)], + /// a: Annotated[str, Depends(foo)], /// # ^^^^^^^^^^^^ /// a: str = Depends(foo), /// # ^^^^^^^^^^^^