Skip to content
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

RUF029 gives false positives with FastApi routes #12925

Closed
TomerBin opened this issue Aug 16, 2024 · 4 comments · Fixed by #12938
Closed

RUF029 gives false positives with FastApi routes #12925

TomerBin opened this issue Aug 16, 2024 · 4 comments · Fixed by #12938
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule

Comments

@TomerBin
Copy link
Contributor

I would like to suggest that RUF029 will skip functions that are FastApi routes. In their docs, it is explicitly mentioned that sometimes routes should be declared as async function, even when not doing any asynchronous operation.
As a first step we can introduce a new rule setting that will allow opting into this new behavior.

One caveat is with FastApi dependencies that we should handle the same way but we can't since dependencies are just native functions and can't be detected in their deceleration (only in usage), but I believe that ignoring routes and keeping the current behavior for dependencies is a good step.

If you find this reasonable, I would like to implement it 😊

@MichaReiser
Copy link
Member

That sounds reasonable. RUF027 should probably also be disabled for path decorators.

The only concern is that we start to have a lot of framework specific code. Something we generally try to avoid. @AlexWaygood any opinion on that?

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview mode features labels Aug 16, 2024
@AlexWaygood
Copy link
Member

Yeah, I'm a little wary of adding special-casing for third-party libraries to generalised rules like RUF027 and RUF029, as it feels like a slippery slope. Perhaps there's a case to be made that fastAPI is popular enough to deserve special treatment, but we can't feasibly compile an exhaustive, accurate list of all third-party packages that require template strings or async functions.

I'm also not sure how we'd actually address the false positive here. It would require quite sophisticated semantic analysis (including some attempt at type inference), which wouldn't necessarily be accurate without multifile analysis. IIUC, the false positive is emitted on code like this:

from fastapi import FastAPI

app = FastAPI()

@app.get('/')
async def read_results():  # RUF029
    return foo()

To get rid of this false positive, we'd have to:

  • Check all decorators on the async function before emitting a RUF029 diagnostic
  • For each decorator, check to see if it's a call expression where func is an attribute access
    • If so, check to see if the value of the Attribute node is a name that refers to a binding where the value of the binding is a FastAPI object
    • And check to see if the attr of the Attribute node is the name of a method that we know is used to decorate fastAPI routes.
    • If both of those are true for all decorators on the async function, don't emit RUF029

It would be possible, but it would be complicated, and would require encoding a lot of knowledge about fastAPI internals into the rule. I'm not sure if it would be worth it, personally. Possibly the best course of action would be to add a "Known limitations" section to the RUF029 docs about this, and recommend that fastAPI users disable this rule.

@charliermarsh
Copy link
Member

I think we already have an is_fastapi_route_decorator that is being used across various rules.

@AlexWaygood
Copy link
Member

I think we already have an is_fastapi_route_decorator that is being used across various rules.

Ah, nice. That resolves my concern about the added complexity. Given that we already have this logic implemented, and given that I can't really see any other way of addressing the false positive (you couldn't really add a configuration option here), I suppose we may as well use it to get rid of the false positive in RUF029, then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants