Skip to content

[fastapi] Avoid false positive for class dependencies (FAST003) #18271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

twentyone212121
Copy link
Contributor

Summary

Closes #17226.

This PR updates the FAST003 rule to correctly handle FastAPI class dependencies. Specifically, if a path parameter is declared in either:

  • a pydantic.BaseModel used as a dependency, or
  • the __init__ method of a class used as a dependency,

then FAST003 will no longer incorrectly report it as unused.

FastAPI allows a shortcut when using annotated class dependencies - Depends can be called without arguments, e.g.:

class MyParams(BaseModel):
    my_id: int

@router.get("/{my_id}")
def get_id(params: Annotated[MyParams, Depends()]): ...

This PR ensures that such usage is properly supported by the linter.

Note: Support for dataclasses is not included in this PR. Let me know if you’d like it to be added.

Test Plan

Added relevant test cases to the FAST003.py fixture.

Copy link
Contributor

github-actions bot commented May 23, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre added the rule Implementing or modifying a lint rule label May 28, 2025
@ntBre ntBre self-requested a review May 28, 2025 21:02
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I had a couple of very minor suggestions, but this looks good to me.

How much work do you think it would be to add support for dataclasses? I don't feel too strongly either way. I think we could handle typing.NamedTuples with just a | ["typing", "NamedTuple"] in the is_pydantic_base_model match, but that's probably even less common.

The issue was about BaseModel anyway, so I think it's totally fine to leave it here.

// that FastAPI will call to create an instance of the class itself.
// https://fastapi.tiangolo.com/tutorial/dependencies/classes-as-dependencies/#shortcut
if arguments.is_empty() {
Self::from_dependency_name(tuple.elts[0].as_name_expr()?, semantic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure it's impossible to get here if tuple.elts is empty, but we could use tuple.elts.first()? just in case.

Comment on lines 410 to 414
let Some(qualified_name) = semantic.resolve_qualified_name(expr) else {
return false;
};

matches!(qualified_name.segments(), ["pydantic", "BaseModel"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let Some(qualified_name) = semantic.resolve_qualified_name(expr) else {
return false;
};
matches!(qualified_name.segments(), ["pydantic", "BaseModel"])
semantic.resolve_qualified_name(expr)
.is_some_and(|qualified_name| matches!(qualified_name.segments(), ["pydantic", "BaseModel"]))

Comment on lines 366 to 374
// Get non-posonly, non-variadic parameters without `self`
init_def
.parameters
.args
.iter()
.skip(1)
.chain(&init_def.parameters.kwonlyargs)
.map(|param| param.name().as_str())
.collect()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use non_posonly_non_variadic_parameters like in the function branch and just tack skip(1) on the end? I think that would be the same as this, just with the chain and skip calls swapped, which seems like it should be okay.

@twentyone212121
Copy link
Contributor Author

Thanks for the review! I’ve made the changes you suggested in the latest commit.

Regarding dataclasses, the simple option would be to check if "dataclass" is in class_def.decorator_list. But since pydantic has its own version (pydantic.dataclasses.dataclass) and there’s also support from the attrs library, it gets a bit trickier.

I saw that Ruff already has some logic for handling this in ruff/rules/helpers.rs, which seems pretty thorough. But it seems like I can't access it from fastapi module.

/// Enumeration of various kinds of dataclasses recognised by Ruff
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub(super) enum DataclassKind {
/// dataclasses created by the stdlib `dataclasses` module
Stdlib,
/// dataclasses created by the third-party `attrs` library
Attrs(AttrsAutoAttribs),
}
/// Return the kind of dataclass this class definition is (stdlib or `attrs`),
/// or `None` if the class is not a dataclass.
pub(super) fn dataclass_kind<'a>(
class_def: &'a ast::StmtClassDef,
semantic: &SemanticModel,
) -> Option<(DataclassKind, &'a ast::Decorator)> {

That said, I’m not sure we need that level of complexity for this rule. Open to your thoughts if you think it’s worth adding support for them here.

@twentyone212121
Copy link
Contributor Author

CI / ecosystem check is failing due to a Connect Timeout Error. Not sure what to do about this

@ntBre
Copy link
Contributor

ntBre commented May 29, 2025

Ah okay, I didn't really consider the full variety of "dataclasses." I think it's fine to leave the PR as is! Nice find on the helpers, though, that will come in handy if we update this later. I think we'd just need to change pub(super) to pub(crate) to access them here, but it's possible that we'd have to change some module visibilities or even move the code too.

CI / ecosystem check is failing due to a Connect Timeout Error. Not sure what to do about this

Looks like this resolved eventually, probably just a temporary network issue.

I'll give this another quick review soon, hopefully today!

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks again!

@ntBre ntBre merged commit e677863 into astral-sh:main Jun 2, 2025
34 checks passed
dcreager added a commit that referenced this pull request Jun 3, 2025
…aration

* origin/main:
  [ty] Infer `list[T]` when unpacking non-tuple type (#18438)
  [ty] Meta-type of type variables should be type[..] (#18439)
  [`pyupgrade`] Make fix unsafe if it deletes comments (`UP050`) (#18390)
  [`pyupgrade`] Make fix unsafe if it deletes comments (`UP004`) (#18393)
  [ty] Support using legacy typing aliases for generic classes in type annotations (#18404)
  Use ty's completions in playground (#18425)
  Update editor setup docs about Neovim and Vim (#18324)
  Update NPM Development dependencies (#18423)
  Infer `list[T]` for starred target in unpacking (#18401)
  [`refurb`] Mark `FURB180` fix unsafe when class has bases (#18149)
  [`fastapi`] Avoid false positive for class dependencies (`FAST003`) (#18271)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FAST003 false positive when path parameter is included inside a BaseModel dependency
2 participants