Avoid introducing syntax errors for FAST003 autofix

Author: charliermarsh

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

@@ -257,6 +257,11 @@ async def f4():
 async def f5():
     return locals()
 
+# https://github.com/astral-sh/ruff/issues/19831
+# NFKC-equivalent non-ASCII path parameter should not match (would cause infinite loop)
+@app.get("/queries/{𝑞𝑢𝑒𝑟𝑦}")
+async def read_query_nfkc(query: str): ...
+
 # https://github.com/astral-sh/ruff/issues/20941
 @app.get("/imports/{import}")
 async def get_import():
@@ -265,3 +270,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

@@ -365,24 +365,155 @@ help: Add `id` to function signature
 note: This is an unsafe fix and may change runtime behavior
 
 FAST003 Parameter `import` appears in route path, but not in `get_import` signature
-   --> FAST003.py:261:20
+   --> FAST003.py:266:20
     |
-260 | # https://github.com/astral-sh/ruff/issues/20941
-261 | @app.get("/imports/{import}")
+265 | # https://github.com/astral-sh/ruff/issues/20941
+266 | @app.get("/imports/{import}")
     |                    ^^^^^^^^
-262 | async def get_import():
-263 |     ...
+267 | async def get_import():
+268 |     ...
     |
 help: Add `import` to function signature
 
 FAST003 Parameter `__debug__` appears in route path, but not in `get_debug` signature
-   --> FAST003.py:265:18
+   --> FAST003.py:270:18
     |
-263 |     ...
-264 |
-265 | @app.get("/debug/{__debug__}")
+268 |     ...
+269 |
+270 | @app.get("/debug/{__debug__}")
     |                  ^^^^^^^^^^^
-266 | async def get_debug():
-267 |     ...
+271 | async def get_debug():
+272 |     ...
     |
 help: Add `__debug__` to function signature
+
+FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing_vararg` signature
+   --> FAST003.py:278:19
+    |
+277 | # Errors: vararg-only and kwarg-only functions
+278 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+279 | async def read_thing_vararg(*query: str): ...
+    |
+help: Add `thing_id` to function signature
+276 | 
+277 | # Errors: vararg-only and kwarg-only functions
+278 | @app.get("/things/{thing_id}")
+    - async def read_thing_vararg(*query: str): ...
+279 + async def read_thing_vararg(thing_id, *query: str): ...
+280 | 
+281 | @app.get("/things/{thing_id}")
+282 | 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:281:19
+    |
+279 | async def read_thing_vararg(*query: str): ...
+280 |
+281 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+282 | async def read_thing_kwarg(**query: str): ...
+    |
+help: Add `thing_id` to function signature
+279 | async def read_thing_vararg(*query: str): ...
+280 | 
+281 | @app.get("/things/{thing_id}")
+    - async def read_thing_kwarg(**query: str): ...
+282 + async def read_thing_kwarg(thing_id, **query: str): ...
+283 | 
+284 | @app.get("/things/{thing_id}")
+285 | 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:284:19
+    |
+282 | async def read_thing_kwarg(**query: str): ...
+283 |
+284 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+285 | async def read_thing_vararg_kwarg(*args, **kwargs): ...
+    |
+help: Add `thing_id` to function signature
+282 | async def read_thing_kwarg(**query: str): ...
+283 | 
+284 | @app.get("/things/{thing_id}")
+    - async def read_thing_vararg_kwarg(*args, **kwargs): ...
+285 + async def read_thing_vararg_kwarg(thing_id, *args, **kwargs): ...
+286 | 
+287 | # Errors: positional-only parameter edge cases
+288 | @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:288:19
+    |
+287 | # Errors: positional-only parameter edge cases
+288 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+289 | async def read_thing_posonly(query: str, /): ...
+    |
+help: Add `thing_id` to function signature
+286 | 
+287 | # Errors: positional-only parameter edge cases
+288 | @app.get("/things/{thing_id}")
+    - async def read_thing_posonly(query: str, /): ...
+289 + async def read_thing_posonly(query: str, /, thing_id): ...
+290 | 
+291 | @app.get("/things/{thing_id}")
+292 | 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:291:19
+    |
+289 | async def read_thing_posonly(query: str, /): ...
+290 |
+291 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+292 | async def read_thing_posonly_trailing(query: str, /,): ...
+    |
+help: Add `thing_id` to function signature
+289 | async def read_thing_posonly(query: str, /): ...
+290 | 
+291 | @app.get("/things/{thing_id}")
+    - async def read_thing_posonly_trailing(query: str, /,): ...
+292 + async def read_thing_posonly_trailing(query: str, /, thing_id,): ...
+293 | 
+294 | @app.get("/things/{thing_id}")
+295 | 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:294:19
+    |
+292 | async def read_thing_posonly_trailing(query: str, /,): ...
+293 |
+294 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+295 | 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:297:19
+    |
+295 | async def read_thing_posonly_default(query: str = "", /): ...
+296 |
+297 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+298 | 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:300:19
+    |
+298 | async def read_thing_posonly_default_trailing(query: str = "", /,): ...
+299 |
+300 | @app.get("/things/{thing_id}")
+    |                   ^^^^^^^^^^
+301 | async def read_thing_posonly_with_regular(query: str = "", /, x=None): ...
+    |
+help: Add `thing_id` to function signature