F821 false positive for .pyi

[JonathanPlasse]

class Distribution(IMetadataProvider): ...
class IMetadataProvider: ...

ruff detects the F821 while flake8 with flake8-pyi does not (flake8-pyi must be installed otherwise it will also detect an error).

Link to the playground

[ngnpope]

Surely this is correct though? At the time of declaration of Distribution it is necessary for IMetadataProvider to have been already declared.

[JonathanPlasse]

No, it is allowed in stub files, this is taken from typeshed. Stub files are only read by linters never executed.

[ngnpope]

Ok. Fair enough, I know declarations in stub files can be out of order. I guess I'm wondering why the declarations cannot just be reordered?

[JonathanPlasse]

You are right even if it is allowed does not mean it should be done.
Should it be fixed?

[JonathanPlasse]

I vote fixing it.

[charliermarsh]

Can you look into how flake8-pyi suppresses these? What mechanism is it using? I'm surprised.

[JonathanPlasse]

This is done here.
When you run flake8 with flake8-pyi installed with --verbose you can see this

❯ flake8 stubs/setuptools/pkg_resources/__init__.pyi --verbose
flake8.checker            MainProcess     91 INFO     Making checkers
flake8.bugbear            MainProcess    107 INFO     Optional warning B950 not present in selected warnings: None. Not firing it at all.
flake8.pyi                MainProcess    119 INFO     Replacing FlakesChecker with PyiAwareFlakesChecker while checking 'stubs/setuptools/pkg_resources/__init__.pyi'
flake8.main.application   MainProcess    194 INFO     Finished running
flake8.main.application   MainProcess    194 INFO     Reporting errors
flake8.main.application   MainProcess    195 INFO     Found a total of 209 violations and reported 0

[charliermarsh]

Woah that's wild!

[charliermarsh]

Yeah, we should probably just change the behavior of ast.rs on pyi files to match whatever they do there then.

[GabDug]

Hey!

Are there any plans for this issue, now that flake8-pyi has been fully integrated?

Thanks and have a great day!

[AlexWaygood]

In #10341, I fixed the following F821 false positive on .pyi files:

x: int
y = x  # F821 previously incorrectly emitted here in a `.pyi` file

But Ruff still emits an error on the original snippet in this issue if it's linting a .pyi file:

class Distribution(IMetadataProvider): ...  # F821 still emitted here currently due to the forward reference
class IMetadataProvider: ...

However, with the latest version of flake8-monkeypatched-by-flake8-pyi, flake8 also emits an F821 error on that snippet. This is due to changes we made in flake8-pyi in PyCQA/flake8-pyi#364. The motivation for the flake8-pyi changes were:

• We wanted to reduce the amount of monkeypatching we were doing to a core minimum.
• We concluded that, although this kind of forward reference is legal in a stub file, it is never necessary when it comes to base classes (and other similar situations), and should probably be considered bad style -- so forbidding it was probably a good thing anyway.

I'm not sure that ruff should necessarily make the same decisions that flake8-pyi made, however:

• We don't have the same concerns about keeping monkeypatching to a minimum, since we're not doing any monkeypatching at all
• F821 is a rule that's concerned with correctness, and a forward reference like this isn't a correctness issue in a stub -- unlike in a .py file, it's perfectly legal; it's just bad style. So maybe we should avoid emitting F821 here, and emit a diagnostic with a different error code instead, to make it clear that we're banning it out of stylistic concerns rather than correctness concerns?

Curious for @charliermarsh's opinion here!

[charliermarsh]

Thanks for the clear write-up. I think my preference would be...

1. Not emitting anything.
2. Emitting a new, dedicated rule.
3. Emitting F821 (which feels incorrect, as you point out above).

[AlexWaygood]

I'd definitely prefer it if we emitted something on these constructs in a stub file -- type checkers don't flag them (since they're legal in .pyi files), and it's honestly pretty confusing if you come across a class in a stub file that inherits from a class that hasn't been defined yet 😄

So that implies (2). I'll start off by making a PR that stops ruff from emitting F821 on this kind of construct, and then look at implementing a new rule for this.

Should the new rule go in the RUF ruleset?

[charliermarsh]

Sounds good. Yeah, unless we want to add it to PYI and coordinate with flake8-pyi to reserve that code.