Avoid introducing syntax errors for FAST003 autofix

Author: charliermarsh

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

@@ -265,3 +265,32 @@ async def get_import():
 @app.get("/debug/{__debug__}")
 async def get_debug():
     ...
+
+
+# https://github.com/astral-sh/ruff/issues/19831
+
+# Errors: vararg-only and kwarg-only functions
+@app.get("/things/{thing_id}")
+async def read_thing_vararg(*query: str): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_kwarg(**query: str): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_vararg_kwarg(*args, **kwargs): ...
+
+# Errors: positional-only parameter edge cases
+@app.get("/things/{thing_id}")
+async def read_thing_posonly(query: str, /): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_posonly_trailing(query: str, /,): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_posonly_default(query: str = "", /): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_posonly_default_trailing(query: str = "", /,): ...
+
+@app.get("/things/{thing_id}")
+async def read_thing_posonly_with_regular(query: str = "", /, x=None): ...

crates/ruff_linter/src/fix/edits.rs

@@ -325,50 +325,75 @@ pub(crate) fn remove_member(elts: &[ast::Expr], index: usize, source: &str) -> R
 }
 
 /// Generic function to add a (regular) parameter to a function definition.
-pub(crate) fn add_parameter(parameter: &str, parameters: &Parameters, source: &str) -> Edit {
+///
+/// Returns `None` if the parameter cannot be added without introducing a syntax error (e.g., a
+/// non-default parameter would follow a positional-only parameter with a default value).
+pub(crate) fn add_parameter(
+    parameter: &str,
+    parameters: &Parameters,
+    source: &str,
+) -> Option<Edit> {
     if let Some(last) = parameters.args.iter().rfind(|arg| arg.default.is_none()) {
-        // Case 1: at least one regular parameter, so append after the last one.
-        Edit::insertion(format!(", {parameter}"), last.end())
-    } else if !parameters.args.is_empty() {
-        // Case 2: no regular parameters, but at least one keyword parameter, so add before the
-        // first.
-        let pos = parameters.start();
-        let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
-        let name = tokenizer
-            .find(|token| token.kind == SimpleTokenKind::Name)
-            .expect("Unable to find name token");
-        Edit::insertion(format!("{parameter}, "), name.start())
+        // Case 1: at least one regular parameter without a default, so append after the last one.
+        Some(Edit::insertion(format!(", {parameter}"), last.end()))
+    } else if let Some(first) = parameters.args.first() {
+        // Case 2: all regular parameters have defaults, so insert before the first one.
+        // However, if any positional-only parameter has a default, inserting a non-default
+        // parameter here would be a syntax error (the "default required" constraint carries
+        // through the `/` separator).
+        if parameters
+            .posonlyargs
+            .last()
+            .is_some_and(|p| p.default.is_some())
+        {
+            return None;
+        }
+        Some(Edit::insertion(format!("{parameter}, "), first.start()))
     } else if let Some(last) = parameters.posonlyargs.last() {
-        // Case 2: no regular parameter, but a positional-only parameter exists, so add after that.
-        // We take care to add it *after* the `/` separator.
-        let pos = last.end();
-        let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
+        // Case 3: no regular parameters, but positional-only parameters exist.
+        // If any positional-only parameter has a default, we can't add a non-default parameter
+        // after the `/` separator — that would be a syntax error.
+        if last.default.is_some() {
+            return None;
+        }
+        // Insert after the `/` separator.
+        let mut tokenizer = SimpleTokenizer::starts_at(last.end(), source);
         let slash = tokenizer
             .find(|token| token.kind == SimpleTokenKind::Slash)
             .expect("Unable to find `/` token");
         // Try to find a comma after the slash.
         let comma = tokenizer.find(|token| token.kind == SimpleTokenKind::Comma);
         if let Some(comma) = comma {
-            Edit::insertion(format!(" {parameter},"), comma.start() + TextSize::from(1))
+            Some(Edit::insertion(format!(" {parameter},"), comma.end()))
         } else {
-            Edit::insertion(format!(", {parameter}"), slash.start())
+            Some(Edit::insertion(format!(", {parameter}"), slash.end()))
         }
-    } else if !parameters.kwonlyargs.is_empty() {
-        // Case 3: no regular parameter, but a keyword-only parameter exist, so add parameter before that.
-        // We need to backtrack to before the `*` separator.
-        // We know there is no non-keyword-only params, so we can safely assume that the `*` separator is the first
+    } else if parameters.vararg.is_some() || !parameters.kwonlyargs.is_empty() {
+        // Case 4: no regular or positional-only parameters, but a vararg (`*args`) or
+        // keyword-only parameters exist. Insert before the `*` separator.
         let pos = parameters.start();
         let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
         let star = tokenizer
             .find(|token| token.kind == SimpleTokenKind::Star)
             .expect("Unable to find `*` token");
-        Edit::insertion(format!("{parameter}, "), star.start())
+        Some(Edit::insertion(format!("{parameter}, "), star.start()))
+    } else if parameters.kwarg.is_some() {
+        // Case 5: only a `**kwargs` parameter exists. Insert before `**`.
+        let pos = parameters.start();
+        let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
+        let double_star = tokenizer
+            .find(|token| token.kind == SimpleTokenKind::DoubleStar)
+            .expect("Unable to find `**` token");
+        Some(Edit::insertion(
+            format!("{parameter}, "),
+            double_star.start(),
+        ))
     } else {
-        // Case 4: no parameters at all, so add parameter after the opening parenthesis.
-        Edit::insertion(
+        // Case 6: no parameters at all, so add parameter after the opening parenthesis.
+        Some(Edit::insertion(
             parameter.to_string(),
             parameters.start() + TextSize::from(1),
-        )
+        ))
     }
 }
 

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

@@ -194,11 +194,13 @@ pub(crate) fn fastapi_unused_path_parameter(
                 .sub_end(TextSize::from((path.len() - range.end + 1) as u32)),
         );
         if !is_positional && is_identifier(path_param) && path_param != "__debug__" {
-            diagnostic.set_fix(Fix::unsafe_edit(add_parameter(
+            if let Some(edit) = add_parameter(
                 path_param,
                 &function_def.parameters,
                 checker.locator().contents(),
-            )));
+            ) {
+                diagnostic.set_fix(Fix::unsafe_edit(edit));
+            }
         }
     }
 }

crates/ruff_linter/src/rules/fastapi/snapshots/ruff_linter__rules__fastapi__tests__fast-api-unused-path-parameter_FAST003.py.snap

@@ -386,3 +386,134 @@ FAST003 Parameter `__debug__` appears in route path, but not in `get_debug` sign
 267 |     ...
     |
 help: Add `__debug__` to function signature
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_vararg` signature
+   --> FAST003.py:273:19
+    |
+272 | # Errors: vararg-only and kwarg-only functions
+273 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+274 | async def read_thing_vararg(*query: str): ...
+    |
+help: Add `thing_id` to function signature
+271 | 
+272 | # Errors: vararg-only and kwarg-only functions
+273 | @app.get("/things/{thing_id}")
+    - async def read_thing_vararg(*query: str): ...
+274 + async def read_thing_vararg(thing_id, *query: str): ...
+275 | 
+276 | @app.get("/things/{thing_id}")
+277 | async def read_thing_kwarg(**query: str): ...
+note: This is an unsafe fix and may change runtime behavior
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_kwarg` signature
+   --> FAST003.py:276:19
+    |
+274 | async def read_thing_vararg(*query: str): ...
+275 |
+276 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+277 | async def read_thing_kwarg(**query: str): ...
+    |
+help: Add `thing_id` to function signature
+274 | async def read_thing_vararg(*query: str): ...
+275 | 
+276 | @app.get("/things/{thing_id}")
+    - async def read_thing_kwarg(**query: str): ...
+277 + async def read_thing_kwarg(thing_id, **query: str): ...
+278 | 
+279 | @app.get("/things/{thing_id}")
+280 | async def read_thing_vararg_kwarg(*args, **kwargs): ...
+note: This is an unsafe fix and may change runtime behavior
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_vararg_kwarg` signature
+   --> FAST003.py:279:19
+    |
+277 | async def read_thing_kwarg(**query: str): ...
+278 |
+279 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+280 | async def read_thing_vararg_kwarg(*args, **kwargs): ...
+    |
+help: Add `thing_id` to function signature
+277 | async def read_thing_kwarg(**query: str): ...
+278 | 
+279 | @app.get("/things/{thing_id}")
+    - async def read_thing_vararg_kwarg(*args, **kwargs): ...
+280 + async def read_thing_vararg_kwarg(thing_id, *args, **kwargs): ...
+281 | 
+282 | # Errors: positional-only parameter edge cases
+283 | @app.get("/things/{thing_id}")
+note: This is an unsafe fix and may change runtime behavior
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_posonly` signature
+   --> FAST003.py:283:19
+    |
+282 | # Errors: positional-only parameter edge cases
+283 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+284 | async def read_thing_posonly(query: str, /): ...
+    |
+help: Add `thing_id` to function signature
+281 | 
+282 | # Errors: positional-only parameter edge cases
+283 | @app.get("/things/{thing_id}")
+    - async def read_thing_posonly(query: str, /): ...
+284 + async def read_thing_posonly(query: str, /, thing_id): ...
+285 | 
+286 | @app.get("/things/{thing_id}")
+287 | async def read_thing_posonly_trailing(query: str, /,): ...
+note: This is an unsafe fix and may change runtime behavior
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_posonly_trailing` signature
+   --> FAST003.py:286:19
+    |
+284 | async def read_thing_posonly(query: str, /): ...
+285 |
+286 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+287 | async def read_thing_posonly_trailing(query: str, /,): ...
+    |
+help: Add `thing_id` to function signature
+284 | async def read_thing_posonly(query: str, /): ...
+285 | 
+286 | @app.get("/things/{thing_id}")
+    - async def read_thing_posonly_trailing(query: str, /,): ...
+287 + async def read_thing_posonly_trailing(query: str, /, thing_id,): ...
+288 | 
+289 | @app.get("/things/{thing_id}")
+290 | async def read_thing_posonly_default(query: str = "", /): ...
+note: This is an unsafe fix and may change runtime behavior
+
+FAST003 Parameter `thing_id` appears in route path, but not in `read_thing_posonly_default` signature
+   --> FAST003.py:289:19
+    |
+287 | async def read_thing_posonly_trailing(query: str, /,): ...
+288 |
+289 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+290 | async def read_thing_posonly_default(query: str = "", /): ...
+    |
+help: Add `thing_id` to function signature
+
+FAST003 Parameter `thing_id` appears in route path, but not in `read_thing_posonly_default_trailing` signature
+   --> FAST003.py:292:19
+    |
+290 | async def read_thing_posonly_default(query: str = "", /): ...
+291 |
+292 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+293 | async def read_thing_posonly_default_trailing(query: str = "", /,): ...
+    |
+help: Add `thing_id` to function signature
+
+FAST003 Parameter `thing_id` appears in route path, but not in `read_thing_posonly_with_regular` signature
+   --> FAST003.py:295:19
+    |
+293 | async def read_thing_posonly_default_trailing(query: str = "", /,): ...
+294 |
+295 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+296 | async def read_thing_posonly_with_regular(query: str = "", /, x=None): ...
+    |
+help: Add `thing_id` to function signature