[flake8-type-checking] Add exemption for runtime evaluated decorator classes

[viccie30]

Summary

This PR makes ruff recognize functions decorated with methods,
like FastAPI uses to specify endpoints.

I've implemented the check by generalizing
ruff_linter::src::rules::fastapi::rules::is_fastapi_route_call to
recognize methods of specific classes used as decorators.

Test Plan

I have added tests by duplicating and adjusting
crates/ruff_linter/resources/test/fixtures/flake8_type_checking/runtime_evaluated_decorators_{1..3}.py.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Would this address #13713 ?

[viccie30]

Would this address #13713 ?

I think so.

[MichaReiser]

Thank you. I think this makes sense.

@Daverball, I'd be interested in your thoughts on this change because you're the most familiar with our type-checking rules.

[Daverball]

This looks good to me. The flake8 plugin currently does something less clever for FastAPI support, with a toggle that you have to enable if you use FastAPI, which increases false negatives in the rest of your code. This approach seems more balanced and flexible.

My only concern is that we probably want to add additional settings to support marker generics like sqlalchemy.orm.Mapped, which require some or all of the symbols to be available at runtime. I think FastAPI also has some things like that. So at that point we'd have at least five very similar options, which begs the question of whether there's maybe a better way to organize them.

[viccie30]

My only concern is that we probably want to add additional settings to support marker generics like sqlalchemy.orm.Mapped, which require some or all of the symbols to be available at runtime. I think FastAPI also has some things like that. So at that point we'd have at least five very similar options, which begs the question of whether there's maybe a better way to organize them.

I had throught about rolling this option into runtime-evaluated-decorators by adding a fallback from matching any identifier in the list literally to matching any value that is initialized by any identifier in the list, but I thought separating the two was cleaner.

[Daverball]

The one thing this currently fails to cover is sharing your instances between modules:

from datetime import datetime
from mymodule import app

@app.put("/datetime")
def set_datetime(value: datetime) -> None:
    pass

The question is if that matters, since red knot presumably will be able to handle that case in the future.

---

But we could also support that use-case by extending runtime-evaluated-decorators to match if the dotted name starts the same way, rather than only when it matches exactly:

[tool.ruff.lint.flake8-type-checking]
runtime-evaluated-decorators = ["mymodule.app"]

would match @mymodule.app but also @mymodule.app.route

[viccie30]

The one thing this currently fails to cover is sharing your instances between modules:

from datetime import datetime
from mymodule import app

@app.put("/datetime")
def set_datetime(value: datetime) -> None:
    pass

The question is if that matters, since red knot presumably will be able to handle that case in the future.

But we could also support that use-case by extending runtime-evaluated-decorators to match if the dotted name starts the same way, rather than only when it matches exactly:

[tool.ruff.lint.flake8-type-checking]
runtime-required-decorators = ["mymodule.app"]

would match @mymodule.app but also @mymodule.app.route

I had thought about how to add that possibility, but I could not find a clean way. If this is something you would like to see as you sketched it above, I'd be happy to add it.

[Daverball]

If this is something you would like to see as you sketched it above, I'd be happy to add it.

I think that would be enough to cover most FastAPI use-cases, so it seems like a desirable improvement to the semantics of that setting. Although it might require some documentation improvements, so people understand, that this is something they can do.

It might also be interesting to investigate whether we can get away with just the original setting with this change, as long as we can match the binding in the module it was defined to the fully qualified name. I.e.

mymodule/__init__.py

from fastapi import FastAPI as Api

app = Api()

@app.put("/datetime")  # matches "mymodule.app" because we're in `mymodule`.
def set_datetime(value: datetime) -> None:
    pass

[Daverball]

Actually, the other use-case can already be supported by adding mymodule.app.put etc. although that would be quite tedious and error-prone, so maybe it makes more sense to support glob patterns, i.e. mymodule.app.*, rather than arbitrary sub-matches.

[viccie30]

Actually, the other use-case can already be supported by adding mymodule.app.put etc. although that would be quite tedious and error-prone, so maybe it makes more sense to support glob patterns, i.e. mymodule.app.*, rather than arbitrary sub-matches.

Would you like me to add that to this PR or open another PR for that? And do you still want to merge this PR?

[Daverball]

I have no merging power. All I can give is my opinion.

I think the feature is fine the way you implemented it, but if we can support this use-case with only the existing setting by slightly changing the semantics, that would be even better, since we wouldn't need to rely on a potentially expensive resolve_assignment that way and can keep the configuration more simple.

If you feel like experimenting with the suggested approach in a separate PR, feel free. But please don't feel compelled to invest the time. I'm happy to try it myself, although I definitely won't get around to it today.

@MichaReiser Care to chime in, with how you would like this to move forward?