[FastAPI] Avoid introducing invalid syntax in fix for fast-api-non-annotated-dependency (FAST002) (#13133)

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

@@ -17,7 +17,7 @@ app = FastAPI()
 router = APIRouter()
 
 
-# Errors
+# Fixable errors
 
 @app.get("/items/")
 def get_items(
@@ -40,6 +40,34 @@ def do_stuff(
     # do stuff
     pass
 
+@app.get("/users/")
+def get_users(
+    skip: int,
+    limit: int,
+    current_user: User = Depends(get_current_user),
+):
+    pass
+
+@app.get("/users/")
+def get_users(
+    current_user: User = Depends(get_current_user),
+    skip: int = 0,
+    limit: int = 10,
+):
+    pass
+
+
+
+# Non fixable errors
+
+@app.get("/users/")
+def get_users(
+    skip: int = 0,
+    limit: int = 10,
+    current_user: User = Depends(get_current_user),
+):
+    pass
+
 
 # Unchanged
 

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

@@ -1,4 +1,4 @@
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast as ast;
 use ruff_python_ast::helpers::map_callable;
@@ -59,14 +59,16 @@ use crate::settings::types::PythonVersion;
 #[violation]
 pub struct FastApiNonAnnotatedDependency;
 
-impl AlwaysFixableViolation for FastApiNonAnnotatedDependency {
+impl Violation for FastApiNonAnnotatedDependency {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
     #[derive_message_formats]
     fn message(&self) -> String {
         format!("FastAPI dependency without `Annotated`")
     }
 
-    fn fix_title(&self) -> String {
+    fn fix_title(&self) -> Option<String> {
-        "Replace with `Annotated`".to_string()
+        Some("Replace with `Annotated`".to_string())
     }
 }
 
@@ -75,64 +77,95 @@ pub(crate) fn fastapi_non_annotated_dependency(
     checker: &mut Checker,
     function_def: &ast::StmtFunctionDef,
 ) {
-    if !checker.semantic().seen_module(Modules::FASTAPI) {
+    if !checker.semantic().seen_module(Modules::FASTAPI)
-        return;
+        || !is_fastapi_route(function_def, checker.semantic())
-    }
+    {
-    if !is_fastapi_route(function_def, checker.semantic()) {
         return;
     }
+
+    let mut updatable_count = 0;
+    let mut has_non_updatable_default = false;
+    let total_params = function_def.parameters.args.len();
+
     for parameter in &function_def.parameters.args {
-        if let (Some(annotation), Some(default)) =
+        let needs_update = matches!(
-            (&parameter.parameter.annotation, &parameter.default)
+            (&parameter.parameter.annotation, &parameter.default),
-        {
+            (Some(_annotation), Some(default)) if is_fastapi_dependency(checker, default)
-            if checker
+        );
-                .semantic()
-                .resolve_qualified_name(map_callable(default))
-                .is_some_and(|qualified_name| {
-                    matches!(
-                        qualified_name.segments(),
-                        [
-                            "fastapi",
-                            "Query"
-                                | "Path"
-                                | "Body"
-                                | "Cookie"
-                                | "Header"
-                                | "File"
-                                | "Form"
-                                | "Depends"
-                                | "Security"
-                        ]
-                    )
-                })
-            {
-                let mut diagnostic =
-                    Diagnostic::new(FastApiNonAnnotatedDependency, parameter.range);
 
-                diagnostic.try_set_fix(|| {
+        if needs_update {
-                    let module = if checker.settings.target_version >= PythonVersion::Py39 {
+            updatable_count += 1;
-                        "typing"
+            // Determine if it's safe to update this parameter:
-                    } else {
+            // - if all parameters are updatable its safe.
-                        "typing_extensions"
+            // - 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 (import_edit, binding) = checker.importer().get_or_import_symbol(
+            let safe_to_update = updatable_count == total_params || !has_non_updatable_default;
-                        &ImportRequest::import_from(module, "Annotated"),
+            create_diagnostic(checker, parameter, safe_to_update);
-                        function_def.start(),
+        } else if parameter.default.is_some() {
-                        checker.semantic(),
+            has_non_updatable_default = true;
-                    )?;
-                    let content = 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]))
-                });
-
-                checker.diagnostics.push(diagnostic);
-            }
         }
     }
 }
+
+fn is_fastapi_dependency(checker: &Checker, expr: &ast::Expr) -> bool {
+    checker
+        .semantic()
+        .resolve_qualified_name(map_callable(expr))
+        .is_some_and(|qualified_name| {
+            matches!(
+                qualified_name.segments(),
+                [
+                    "fastapi",
+                    "Query"
+                        | "Path"
+                        | "Body"
+                        | "Cookie"
+                        | "Header"
+                        | "File"
+                        | "Form"
+                        | "Depends"
+                        | "Security"
+                ]
+            )
+        })
+}
+
+fn create_diagnostic(
+    checker: &mut Checker,
+    parameter: &ast::ParameterWithDefault,
+    safe_to_update: bool,
+) {
+    let mut diagnostic = Diagnostic::new(FastApiNonAnnotatedDependency, 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"
+                };
+                let (import_edit, binding) = checker.importer().get_or_import_symbol(
+                    &ImportRequest::import_from(module, "Annotated"),
+                    parameter.range.start(),
+                    checker.semantic(),
+                )?;
+                let content = 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;
+    }
+
+    checker.diagnostics.push(diagnostic);
+}

crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-non-annotated-dependency_FAST002.py.snap

@@ -261,3 +261,72 @@ FAST002.py:38:5: FAST002 [*] FastAPI dependency without `Annotated`
 39 40 | ):
 40 41 |     # do stuff
 41 42 |     pass
+
+FAST002.py:47:5: FAST002 [*] FastAPI dependency without `Annotated`
+   |
+45 |     skip: int,
+46 |     limit: int,
+47 |     current_user: User = Depends(get_current_user),
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FAST002
+48 | ):
+49 |     pass
+   |
+   = help: Replace with `Annotated`
+
+ℹ Unsafe fix
+12 12 |     Security,
+13 13 | )
+14 14 | from pydantic import BaseModel
+   15 |+from typing import Annotated
+15 16 | 
+16 17 | app = FastAPI()
+17 18 | router = APIRouter()
+--------------------------------------------------------------------------------
+44 45 | def get_users(
+45 46 |     skip: int,
+46 47 |     limit: int,
+47    |-    current_user: User = Depends(get_current_user),
+   48 |+    current_user: Annotated[User, Depends(get_current_user)],
+48 49 | ):
+49 50 |     pass
+50 51 | 
+
+FAST002.py:53:5: FAST002 [*] FastAPI dependency without `Annotated`
+   |
+51 | @app.get("/users/")
+52 | def get_users(
+53 |     current_user: User = Depends(get_current_user),
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FAST002
+54 |     skip: int = 0,
+55 |     limit: int = 10,
+   |
+   = help: Replace with `Annotated`
+
+ℹ Unsafe fix
+12 12 |     Security,
+13 13 | )
+14 14 | from pydantic import BaseModel
+   15 |+from typing import Annotated
+15 16 | 
+16 17 | app = FastAPI()
+17 18 | router = APIRouter()
+--------------------------------------------------------------------------------
+50 51 | 
+51 52 | @app.get("/users/")
+52 53 | def get_users(
+53    |-    current_user: User = Depends(get_current_user),
+   54 |+    current_user: Annotated[User, Depends(get_current_user)],
+54 55 |     skip: int = 0,
+55 56 |     limit: int = 10,
+56 57 | ):
+
+FAST002.py:67:5: FAST002 FastAPI dependency without `Annotated`
+   |
+65 |     skip: int = 0,
+66 |     limit: int = 10,
+67 |     current_user: User = Depends(get_current_user),
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FAST002
+68 | ):
+69 |     pass
+   |
+   = help: Replace with `Annotated`