[FastAPI] Update Annotated fixes (FAST002)

crates/ruff_linter/resources/test/fixtures/fastapi/FAST002.py → crates/ruff_linter/resources/test/fixtures/fastapi/FAST002_0.py

@@ -29,13 +29,13 @@ def get_items(
 
 @app.post("/stuff/")
 def do_stuff(
-    some_query_param: str | None = Query(default=None),
     some_path_param: str = Path(),
-    some_body_param: str = Body("foo"),
     some_cookie_param: str = Cookie(),
-    some_header_param: int = Header(default=5),
     some_file_param: UploadFile = File(),
     some_form_param: str = Form(),
+    some_query_param: str | None = Query(default=None),
+    some_body_param: str = Body("foo"),
+    some_header_param: int = Header(default=5),
 ):
     # do stuff
     pass

crates/ruff_linter/resources/test/fixtures/fastapi/FAST002_1.py

@@ -0,0 +1,21 @@
+"""Test that FAST002 doesn't suggest invalid Annotated fixes with default
+values. See #15043 for more details."""
+
+from fastapi import FastAPI, Query
+
+app = FastAPI()
+
+
+@app.get("/test")
+def handler(echo: str = Query("")):
+    return echo
+
+
+@app.get("/test")
+def handler2(echo: str = Query(default="")):
+    return echo
+
+
+@app.get("/test")
+def handler3(echo: str = Query("123", min_length=3, max_length=50)):
+    return echo

crates/ruff_linter/src/rules/fastapi/mod.rs

@@ -14,7 +14,8 @@ mod tests {
     use crate::{assert_messages, settings};
 
     #[test_case(Rule::FastApiRedundantResponseModel, Path::new("FAST001.py"))]
-    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002.py"))]
+    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_0.py"))]
+    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_1.py"))]
     #[test_case(Rule::FastApiUnusedPathParameter, Path::new("FAST003.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
@@ -28,7 +29,8 @@ mod tests {
 
     // FAST002 autofixes use `typing_extensions` on Python 3.8,
     // since `typing.Annotated` was added in Python 3.9
-    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002.py"))]
+    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_0.py"))]
+    #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_1.py"))]
     fn rules_py38(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}_py38", rule_code.as_ref(), path.to_string_lossy());
         let diagnostics = test_path(

crates/ruff_linter/src/rules/fastapi/rules/fastapi_non_annotated_dependency.rs

@@ -97,10 +97,9 @@ pub(crate) fn fastapi_non_annotated_dependency(
         return;
     }
 
-    let mut updatable_count = 0;
-    let mut has_non_updatable_default = false;
-    let total_params =
-        function_def.parameters.args.len() + function_def.parameters.kwonlyargs.len();
+    // `create_diagnostic` needs to know if a default argument has been seen to
+    // avoid emitting fixes that would remove defaults and cause a syntax error.
+    let mut seen_default = false;
 
     for parameter in function_def
         .parameters
@@ -114,15 +113,9 @@ pub(crate) fn fastapi_non_annotated_dependency(
         );
 
         if needs_update {
-            updatable_count += 1;
-            // Determine if it's safe to update this parameter:
-            // - if all parameters are updatable its safe.
-            // - if we've encountered a non-updatable parameter with a default value, it's no longer
-            //   safe. (https://github.com/astral-sh/ruff/issues/12982)
-            let safe_to_update = updatable_count == total_params || !has_non_updatable_default;
-            create_diagnostic(checker, parameter, safe_to_update);
+            seen_default = create_diagnostic(checker, parameter, seen_default);
         } else if parameter.default.is_some() {
-            has_non_updatable_default = true;
+            seen_default = true;
         }
     }
 }
@@ -153,44 +146,94 @@ fn is_fastapi_dependency(checker: &Checker, expr: &ast::Expr) -> bool {
 fn create_diagnostic(
     checker: &mut Checker,
     parameter: &ast::ParameterWithDefault,
-    safe_to_update: bool,
-) {
+    mut seen_default: bool,
+) -> bool {
     let mut diagnostic = Diagnostic::new(
         FastApiNonAnnotatedDependency {
             py_version: checker.settings.target_version,
         },
         parameter.range,
     );
 
-    if safe_to_update {
-        if let (Some(annotation), Some(default)) =
-            (&parameter.parameter.annotation, &parameter.default)
-        {
-            diagnostic.try_set_fix(|| {
-                let module = if checker.settings.target_version >= PythonVersion::Py39 {
-                    "typing"
-                } else {
-                    "typing_extensions"
+    if let (Some(annotation), Some(default)) = (&parameter.parameter.annotation, &parameter.default)
+    {
+        diagnostic.try_set_optional_fix(|| {
+            let module = if checker.settings.target_version >= PythonVersion::Py39 {
+                "typing"
+            } else {
+                "typing_extensions"
+            };
+            let (import_edit, binding) = checker.importer().get_or_import_symbol(
+                &ImportRequest::import_from(module, "Annotated"),
+                parameter.range.start(),
+                checker.semantic(),
+            )?;
+
+            // Refine the match from `is_fastapi_dependency` to exclude Depends
+            // and Security, which don't have the same argument structure. The
+            // others need to be converted from `q: str = Query("")` to `q:
+            // Annotated[str, Query()] = ""` for example, but Depends and
+            // Security need to stay like `Annotated[str, Depends(callable)]`
+            let is_dep = checker
+                .semantic()
+                .resolve_qualified_name(map_callable(default))
+                .is_some_and(|qualified_name| {
+                    !matches!(
+                        qualified_name.segments(),
+                        ["fastapi", "Depends" | "Security"]
+                    )
+                });
+
+            // Each of these classes takes a single, optional default
+            // argument, followed by kw-only arguments
+            let default_arg = default
+                .as_call_expr()
+                .and_then(|args| args.arguments.find_argument("default", 0));
+
+            let kwarg_list: Option<Vec<_>> = default.as_call_expr().map(|args| {
+                args.arguments
+                    .keywords
+                    .iter()
+                    .filter_map(|kwarg| match kwarg.arg.as_ref() {
+                        None => None,
+                        Some(name) if name == "default" => None,
+                        Some(_) => Some(checker.locator().slice(kwarg.range())),
+                    })
+                    .collect()
+            });
+
+            let content = if is_dep && default_arg.is_some() {
+                let default_arg = default_arg.unwrap();
+                let kwarg_list = match kwarg_list {
+                    Some(v) => v.join(", "),
+                    None => String::new(),
                 };
-                let (import_edit, binding) = checker.importer().get_or_import_symbol(
-                    &ImportRequest::import_from(module, "Annotated"),
-                    parameter.range.start(),
-                    checker.semantic(),
-                )?;
-                let content = format!(
+                seen_default = true;
+                format!(
+                    "{}: {}[{}, {}({kwarg_list})] = {}",
+                    &parameter.parameter.name.id,
+                    binding,
+                    checker.locator().slice(annotation.range()),
+                    checker.locator().slice(map_callable(default).range()),
+                    checker.locator().slice(default_arg.value().range()),
+                )
+            } else if !seen_default {
+                format!(
                     "{}: {}[{}, {}]",
                     parameter.parameter.name.id,
                     binding,
                     checker.locator().slice(annotation.range()),
                     checker.locator().slice(default.range())
-                );
-                let parameter_edit = Edit::range_replacement(content, parameter.range);
-                Ok(Fix::unsafe_edits(import_edit, [parameter_edit]))
-            });
-        }
-    } else {
-        diagnostic.fix = None;
+                )
+            } else {
+                return Ok(None);
+            };
+            let parameter_edit = Edit::range_replacement(content, parameter.range);
+            Ok(Some(Fix::unsafe_edits(import_edit, [parameter_edit])))
+        });
     }
 
     checker.diagnostics.push(diagnostic);
+
+    seen_default
 }