F821 (Undefined name) and F722 (bad forward annotation) can trigger on @no_type_check-ed annotations

[Wuestengecko]

Hi! I found that F821 ("Undefined name") and F722 ("Syntax error in forward annotation") can erroneously trigger on functions that marked @no_type_check, if they happen to either use names that aren't in scope, or aren't even valid Python.

Consider the following example file:

from typing import no_type_check

@no_type_check
def f821(arg: "A") -> "R":
    pass

@no_type_check
def f722(arg: "this isn't python") -> "this isn't python either":
    pass

Save it as "demo.py" and run ruff with:

ruff check --isolated --select=F722,F821 demo.py --output-format concise

This will emit the following errors, which are all false-positives:

demo.py:4:16: F821 Undefined name `A`
demo.py:4:24: F821 Undefined name `R`
demo.py:8:15: F722 Syntax error in forward annotation: `this isn't python`
demo.py:8:39: F722 Syntax error in forward annotation: `this isn't python either`
Found 4 errors.

I'm presuming (but haven't tested) that this could apply to other rules that inspect annotations as well. Ruff should treat these annotations as opaque strings without any meaning. As per PEP-484:

Functions with the @no_type_check decorator should be treated as having no annotations.

---

ruff --version: 0.7.0
settings: no config file exists

[MichaReiser]

Thanks for reporting. Suppressing some rules annotated with @no_type_check makes sense to me.

[AlexWaygood]

This seems reasonable to me too

[charliermarsh]

Should we just skip visiting them altogether when in a @no_type_check context?

[AlexWaygood]

Should we just skip visiting them altogether when in a @no_type_check context?

That might make sense, yeah

[MichaReiser]

Should we just skip visiting them altogether when in a @no_type_check context?

Are you suggesting skipping such functions only for F821 and F722 or all rules?

[charliermarsh]

I'm suggesting skipping looking at those nodes entirely, for all rules.

[charliermarsh]

Alternatively, we could visit them as strings (so enforce rules like implicit concat -- weird but possible) but skip parsing them and visiting the content.

[MichaReiser]

To clarify, with all nodes you mean skipping the entire function or only the type annotations? I'm somewhat fine skipping the annotations but would find it a stretch if we skip the entire function

[charliermarsh]

Just the annotations. The rest of the function would be unchanged.

[InSyncWithFoo]

This seems to have been resolved by #14615.

[MichaReiser]

We'll probably revert this change. It's a bit more involved, see #14698