Improve handling of forward references #364

tomasr8 · 2023-04-11T21:16:19Z

As discussed here

I've checked manually and it finds a couple of issues in typeshed, but I didn't have the time to go through them and verify them.

Fixes #276
Fixes #317
Fixes #278

tomasr8 · 2023-04-11T21:44:18Z

Interesting 🤔

I'm getting these running on typeshed/stdlib:

stdlib/email/_header_value_parser.pyi:25:22: F821 undefined name 'TokenList'
stdlib/email/_header_value_parser.pyi:25:34: F821 undefined name 'Terminal'
stdlib/enum.pyi:212:12: F821 undefined name 'IntFlag'
stdlib/enum.pyi:240:49: F821 undefined name 'KEEP'
stdlib/http/cookiejar.pyi:24:26: F821 undefined name 'Cookie'
stdlib/mailbox.pyi:107:23: F821 undefined name 'MaildirMessage'
stdlib/mailbox.pyi:150:22: F821 undefined name 'mboxMessage'
stdlib/mailbox.pyi:153:22: F821 undefined name 'MMDFMessage'
stdlib/mailbox.pyi:156:18: F821 undefined name 'MHMessage'
stdlib/mailbox.pyi:179:32: F821 undefined name 'BabylMessage'
stdlib/multiprocessing/synchronize.pyi:17:24: F821 undefined name 'Semaphore'
stdlib/multiprocessing/synchronize.pyi:39:12: F821 undefined name 'SemLock'
stdlib/multiprocessing/synchronize.pyi:42:13: F821 undefined name 'SemLock'
stdlib/multiprocessing/synchronize.pyi:45:17: F821 undefined name 'SemLock'
stdlib/os/__init__.pyi:733:75: F821 undefined name '_ScandirIterator'
stdlib/sqlite3/dbapi2.pyi:393:17: F821 undefined name 'DatabaseError'
stdlib/sqlite3/dbapi2.pyi:394:21: F821 undefined name 'Error'

I'll check why the CI is failing tomorrow..

AlexWaygood · 2023-04-12T09:47:29Z

Oh, we recently switched off F821 for the whole typeshed repo: https://github.com/python/typeshed/pull/9976/files

It was causing too many false positives on .pyi files. (Pyflakes doesn't officially support stub files, and I don't think they have any interest in doing so, unfortunately.) We'd been working around the false positives with some ugly hacks, but it finally got too much, so we just disabled it altogether.

That means that the approach in this PR probably won't be viable, unfortunately 😔

tomasr8 · 2023-04-12T11:16:22Z

Hmm that's unfortunate..

Maybe something like this could work ? This should in theory ignore all F821 errors except for class definitions, though I haven't had the time to test it..

def handleNodeLoad(self, node, *args, **kwargs):
    super().handleNodeLoad(node, *args, **kwargs)
    parent = self.getParent(node)
    # Remove error message if it's F821 and we're in ClassDef
    if self.messages and isinstance(self.messages[-1], UndefinedName) and not isinstance(parent, ast.ClassDef):
        self.messages.pop()

AlexWaygood · 2023-04-12T11:54:21Z

Hmm that's unfortunate..

Maybe something like this could work ? This should in theory ignore all F821 errors except for class definitions, though I haven't had the time to test it..

def handleNodeLoad(self, node, *args, **kwargs):
    super().handleNodeLoad(node, *args, **kwargs)
    parent = self.getParent(node)
    # Remove error message if it's F821 and we're in ClassDef
    if self.messages and isinstance(self.messages[-1], UndefinedName) and not isinstance(parent, ast.ClassDef):
        self.messages.pop()

That looks like it might still emit false positives for assignment-via-annotations in the global scope, e.g.:

INSTANCE: str
upper = INSTANCE.upper

But, I'm not really too familiar with the pyflakes codebase. The easiest way to find out is to try it out ;)

In general, my approach to this area of the codebase has been "I hate that we monkey-patch pyflakes at all and I'm scared to change this code that I inherited and don't really understand fully" 😬 But, maybe it's time we bit the bullet...

Though technically allowed, this enforces that subclasses are defined after the classes they inherit from which improves readability.

By default pyflakes requires a future import for annotations in order to process forward references in annotations. This makes it so that the explicit import is not needed, which gets rid of a lot of F821 errors.

tomasr8 · 2023-04-13T15:49:06Z

OK so looking at the pyflakes code, it can already handle deferred annotations, but you need from __future__ import annotations, otherwise it does not get enabled. I added this fix and running it on typeshed with F821 enabled I get these errors (I compared the before and after and the only new errors are F821):

stdlib/email/_header_value_parser.pyi:25:22: F821 undefined name 'TokenList'
stdlib/email/_header_value_parser.pyi:25:34: F821 undefined name 'Terminal'
stdlib/enum.pyi:212:12: F821 undefined name 'IntFlag'
stdlib/enum.pyi:240:49: F821 undefined name 'KEEP'
stdlib/http/cookiejar.pyi:24:26: F821 undefined name 'Cookie'
stdlib/mailbox.pyi:107:23: F821 undefined name 'MaildirMessage'
stdlib/mailbox.pyi:150:22: F821 undefined name 'mboxMessage'
stdlib/mailbox.pyi:153:22: F821 undefined name 'MMDFMessage'
stdlib/mailbox.pyi:156:18: F821 undefined name 'MHMessage'
stdlib/mailbox.pyi:179:32: F821 undefined name 'BabylMessage'
stdlib/multiprocessing/synchronize.pyi:17:24: F821 undefined name 'Semaphore'
stdlib/multiprocessing/synchronize.pyi:39:12: F821 undefined name 'SemLock'
stdlib/multiprocessing/synchronize.pyi:42:13: F821 undefined name 'SemLock'
stdlib/multiprocessing/synchronize.pyi:45:17: F821 undefined name 'SemLock'
stdlib/os/__init__.pyi:733:75: F821 undefined name '_ScandirIterator'
stdlib/sqlite3/dbapi2.pyi:393:17: F821 undefined name 'DatabaseError'
stdlib/sqlite3/dbapi2.pyi:394:21: F821 undefined name 'Error'
stubs/boto/boto/s3/website.pyi:37:25: F821 undefined name 'RoutingRule'
stubs/openpyxl/openpyxl/utils/datetime.pyi:6:40: NQA102 "# noqa: F821" has no matching violations
stubs/openpyxl/openpyxl/utils/datetime.pyi:7:32: NQA102 "# noqa: F821" has no matching violations
stubs/passlib/passlib/apps.pyi:23:36: NQA102 "# noqa: F821" has no matching violations
stubs/passlib/passlib/apps.pyi:29:33: NQA102 "# noqa: F821" has no matching violations
stubs/passlib/passlib/apps.pyi:35:38: NQA102 "# noqa: F821" has no matching violations
stubs/polib/polib.pyi:46:24: F821 undefined name 'POEntry'
stubs/polib/polib.pyi:56:24: F821 undefined name 'MOEntry'
stubs/python-crontab/crontab.pyi:174:23: F821 undefined name 'CronSlice'
stubs/setuptools/pkg_resources/__init__.pyi:136:20: F821 undefined name 'NullProvider'
stubs/setuptools/pkg_resources/__init__.pyi:136:34: F821 undefined name 'IResourceProvider'
stubs/setuptools/pkg_resources/__init__.pyi:136:53: F821 undefined name 'IMetadataProvider'
stubs/setuptools/pkg_resources/__init__.pyi:76:28: F821 undefined name 'Distribution'

Most of these seem to be forward refs of classes which we want to forbid anyway.

(btw I still don't understand this B950 in tests, all other files exceed the line length..)

AlexWaygood · 2023-04-13T15:53:00Z

(btw I still don't understand this B950 in tests, all other files exceed the line length..)

Yeah that's really weird. Probably a bug in our CI setup somewhere. I'll look into it when I have the time.

It should be ignored for all .pyi files in this repo:

flake8-pyi/.flake8

Line 33 in 95d65fe

*.pyi: B, E301, E302, E305, E501, E701, E704, W503

tomasr8 · 2023-04-14T08:17:21Z

Yeah that's really weird. Probably a bug in our CI setup somewhere. I'll look into it when I have the time.

Must be something in the test suite triggering it. When I run python -m pytest, I get the same error locally as well, though when I add an explicit B950, the tests pass. Running flake directly via flake8 tests/ I get the correct behaviour without having to add B950..

tomasr8 · 2023-04-14T08:30:12Z

Ok, figured it out, the problem is that in some files we use --ignore=xxx. This should be --extend-ignore=xxx which adds to the list of ignores rather than replacing it.

Came up in [#364](#364 (comment)) Plain `--ignore` resets the list of ignores which means that the config file is not taken into account.

AlexWaygood · 2023-04-14T08:51:16Z

I'm open to dropping support for running the plugin with flake8 v4 if it's necessary for this PR.

tomasr8 · 2023-04-14T08:56:23Z

I'm open to dropping support for running the plugin with flake8 v4 if it's necessary for this PR.

I'll have a look later to see why exactly it's failing for v4. If it's an easy fix, I'll try to make the PR work for v4 as well.

AlexWaygood · 2023-04-14T09:01:20Z

I'm open to dropping support for running the plugin with flake8 v4 if it's necessary for this PR.

I'll have a look later to see why exactly it's failing for v4. If it's an easy fix, I'll try to make the PR work for v4 as well.

My guess is that it's because flake8 v4 pins an old version of pyflakes that doesn't yet know that forward references in annotations are okay sometimes. But that's just a guess, haven't looked into it at all.

AlexWaygood · 2023-04-14T09:02:57Z

Anyway flake8 v4 is very old at this point, and only really relevant for Python 3.7 (which is nearly end-of-life). So tbh if supporting flake8 v4 involves complicating our code in any way, I'd prefer for us just to drop support for v4 :)

AlexWaygood · 2023-04-14T10:21:08Z

stdlib/email/_header_value_parser.pyi:25:22: F821 undefined name 'TokenList'

This hit is problematic. Here's a simplified version of what we're doing in typeshed:

class TokenList(list[TokenList]): ...

It seems like the monkey-patching we do means that pyflakes-monkeypatched-by-flake8-pyi currently allows recursive class definitions like this. It would be a shame if we regressed on that, as recursive class definitions are needed surprisingly often when writing stubs, and they're a useful feature.

So, to be clear, ideally we'd disallow this:

class Foo(Bar): ...
class Bar: ...

But we'd continue to allow this:

class Foo(list[Bar]): ...
class Bar: ...

pyi.py

tomasr8 · 2023-04-14T12:55:26Z

I can see how having recursive definitions like

class TokenList(list[TokenList]): ...

would be be useful but this

class Foo(list[Bar]): ...
class Bar: ...

just seems like this with extra steps

class Foo(Bar): ...
class Bar: ...

If we agree that the last one breaks reading order, so does the previous one and I'd disallow both of them.. though maybe I'm missing some context here?

AlexWaygood · 2023-04-14T13:00:08Z

I can see how having recursive definitions like
class TokenList(list[TokenList]): ...
would be be useful but this
class Foo(list[Bar]): ...
class Bar: ...
just seems like this with extra steps
class Foo(Bar): ...
class Bar: ...
If we agree that the last one breaks reading order, so does the previous one and I'd disallow both of them.. though maybe I'm missing some context here?

The issue is that you could come up with cases like this, and I think it would be pretty hard to be confident that we'd secured ourselves against any chance of emitting false-positive errors:

class Foo(list[Bar]): ...
class Bar(Foo): ...

tomasr8 · 2023-04-14T15:21:23Z

class Foo(list[Bar]): ...
class Bar(Foo): ...

What would be the ideal solution in this case? Emitting undefined 'Bar'?

JelleZijlstra · 2023-04-14T15:25:15Z

It should be allowed without errors.

AlexWaygood · 2023-04-14T15:31:13Z

class Foo(list[Bar]): ...
class Bar(Foo): ...
What would be the ideal solution in this case? Emitting undefined 'Bar'?

Remember that stub files are never actually executed at runtime; they serve only as "data" for static type checkers and IDEs. As such, forward references are never an error in a .pyi file, just something that's sometimes undesirable as a matter of style, if it can be avoided.

In a case like this, there's no way of avoiding some kind of forward reference, and it's a perfectly valid construct in the context of a stub file, so we should ideally make sure that we don't emit any error for that code.

tomasr8 · 2023-04-27T12:20:45Z

Thanks for the review! I applied your suggestions and added a changelog entry. Let me know if it's ok :)

AlexWaygood

Thank you! Looks like this closes 3 flake8-pyi issues all at once, which is great! (I've updated the PR description.)

AlexWaygood · 2023-04-30T21:40:04Z

@JelleZijlstra or @Akuli, would one of you be able to take a quick look as well?

The typeshed_primer workflow isn't very useful here, because we currently silence F821 in typeshed's .flake8 file. If I unignore F821 in typeshed's .flake8 file, and then run flake8 stubs stdlib with this PR checked out, this is the result I get. I believe all new F821 hits are true positives (cases where we have unnecessary forward references that are bad stylistically), and all new NQA102 hits are cases where we can't avoid forward references, where flake8 no longer emits a false positive error:

stdlib\enum.pyi:212:12: F821 undefined name 'IntFlag'
stdlib\enum.pyi:240:49: F821 undefined name 'KEEP'
stdlib\lib2to3\fixes\fix_imports.pyi:16:24: NQA102 "# noqa: F821" has no matching violations
stdlib\lib2to3\fixes\fix_imports2.pyi:6:24: NQA102 "# noqa: F821" has no matching violations
stdlib\multiprocessing\synchronize.pyi:17:24: F821 undefined name 'Semaphore'
stdlib\multiprocessing\synchronize.pyi:39:12: F821 undefined name 'SemLock'
stdlib\multiprocessing\synchronize.pyi:42:13: F821 undefined name 'SemLock'
stdlib\multiprocessing\synchronize.pyi:45:17: F821 undefined name 'SemLock'
stdlib\sqlite3\dbapi2.pyi:393:17: F821 undefined name 'DatabaseError'
stdlib\sqlite3\dbapi2.pyi:394:21: F821 undefined name 'Error'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:121:22: F821 undefined name 'AnnotatedAliasedReturnsRows'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:122:35: F821 undefined name 'AnnotatedFromClause'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:123:29: F821 undefined name 'AnnotatedGenericFunction'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:124:26: F821 undefined name 'AnnotatedUnaryExpression'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:125:33: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:126:30: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:127:34: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:129:21: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:130:21: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:132:32: F821 undefined name 'AnnotatedColumnElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:133:36: F821 undefined name 'AnnotatedUnaryExpression'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:134:23: F821 undefined name 'AnnotatedColumnClause'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:135:29: F821 undefined name 'AnnotatedNamedColumn'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:137:23: F821 undefined name 'AnnotatedUnaryExpression'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:142:25: F821 undefined name 'AnnotatedFunctionElement'
stubs\SQLAlchemy\sqlalchemy\sql\annotation.pyi:161:22: F821 undefined name 'AnnotatedTableClause'
stubs\openpyxl\openpyxl\utils\datetime.pyi:6:40: NQA102 "# noqa: F821" has no matching violations
stubs\openpyxl\openpyxl\utils\datetime.pyi:7:32: NQA102 "# noqa: F821" has no matching violations
stubs\passlib\passlib\apps.pyi:23:36: NQA102 "# noqa: F821" has no matching violations
stubs\passlib\passlib\apps.pyi:29:33: NQA102 "# noqa: F821" has no matching violations
stubs\passlib\passlib\apps.pyi:35:38: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\compiler\plugin_pb2.pyi:128:155: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:255:148: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:336:150: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:657:155: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:919:142: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:936:144: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\descriptor_pb2.pyi:1170:165: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\type_pb2.pyi:125:133: NQA102 "# noqa: F821" has no matching violations
stubs\protobuf\google\protobuf\type_pb2.pyi:212:147: NQA102 "# noqa: F821" has no matching violations
stubs\setuptools\pkg_resources\__init__.pyi:76:28: F821 undefined name 'Distribution'
stubs\setuptools\pkg_resources\__init__.pyi:136:20: F821 undefined name 'NullProvider'
stubs\setuptools\pkg_resources\__init__.pyi:136:34: F821 undefined name 'IResourceProvider'
stubs\setuptools\pkg_resources\__init__.pyi:136:53: F821 undefined name 'IMetadataProvider'
stubs\tensorflow\tensorflow\compiler\xla\service\hlo_pb2.pyi:806:156: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\compiler\xla\service\hlo_pb2.pyi:1079:156: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\compiler\xla\xla_data_pb2.pyi:851:155: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\compiler\xla\xla_data_pb2.pyi:1310:160: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\compiler\xla\xla_data_pb2.pyi:1418:138: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\compiler\xla\xla_data_pb2.pyi:1576:153: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\framework\api_def_pb2.pyi:50:146: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\framework\dataset_options_pb2.pyi:135:162: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\framework\graph_transfer_info_pb2.pyi:196:159: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\config_pb2.pyi:348:146: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\config_pb2.pyi:375:164: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\config_pb2.pyi:686:182: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\config_pb2.pyi:1086:150: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\core_platform_payloads_pb2.pyi:32:158: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\data_service_pb2.pyi:59:165: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\data_service_pb2.pyi:163:161: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\rewriter_config_pb2.pyi:70:146: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\rewriter_config_pb2.pyi:118:152: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\rewriter_config_pb2.pyi:135:168: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\rewriter_config_pb2.pyi:154:154: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\saved_object_graph_pb2.pyi:475:152: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\saver_pb2.pyi:29:174: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\struct_pb2.pyi:334:159: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tensor_bundle_pb2.pyi:44:157: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\compilation_result_pb2.pyi:36:160: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\optimization_parameters_pb2.pyi:738:158: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\optimization_parameters_pb2.pyi:805:159: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\optimization_parameters_pb2.pyi:846:161: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\topology_pb2.pyi:31:170: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\tpu_embedding_configuration_pb2.pyi:30:153: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\tpu\tpu_embedding_configuration_pb2.pyi:51:177: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\protobuf\verifier_config_pb2.pyi:29:146: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\util\event_pb2.pyi:151:140: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\util\event_pb2.pyi:202:156: NQA102 "# noqa: F821" has no matching violations
stubs\tensorflow\tensorflow\core\util\test_log_pb2.pyi:486:157: NQA102 "# noqa: F821" has no matching violations

AlexWaygood · 2023-04-30T21:43:03Z

Also FYI @nipunn1313 -- looks like we might have to update some autogenerated stubs made using mypy-protobuf if this is merged (but then, you did ask for a change like this in #278 😉).

Will mypy-protobuf itself need to be updated in order to produce stubs compatible with flake8-pyi following this change, or does it add the # noqa comments to the autogenerated stubs based on the output of flake8?

github-actions · 2023-05-01T14:53:53Z

⚠ Flake8 diff showing the effect of this PR on typeshed:

> ./stubs/protobuf/google/protobuf/compiler/plugin_pb2.pyi:128:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:255:148: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:336:150: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:657:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:919:142: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:936:144: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:1170:165: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/type_pb2.pyi:125:133: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/type_pb2.pyi:212:147: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/service/hlo_pb2.pyi:806:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/service/hlo_pb2.pyi:1079:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:851:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1310:160: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1418:138: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1576:153: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/api_def_pb2.pyi:50:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/dataset_options_pb2.pyi:135:162: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/graph_transfer_info_pb2.pyi:196:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:348:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:375:164: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:686:182: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:1086:150: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/core_platform_payloads_pb2.pyi:32:158: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/data_service_pb2.pyi:59:165: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/data_service_pb2.pyi:163:161: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:70:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:118:152: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:135:168: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:154:154: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/saved_object_graph_pb2.pyi:475:152: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/saver_pb2.pyi:29:174: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/struct_pb2.pyi:334:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tensor_bundle_pb2.pyi:44:157: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/compilation_result_pb2.pyi:36:160: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:738:158: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:805:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:846:161: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/topology_pb2.pyi:31:170: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/tpu_embedding_configuration_pb2.pyi:30:153: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/tpu_embedding_configuration_pb2.pyi:51:177: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/verifier_config_pb2.pyi:29:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/event_pb2.pyi:151:140: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/event_pb2.pyi:202:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/test_log_pb2.pyi:486:157: NQA102 "# noqa: F821" has no matching violations

AlexWaygood · 2023-05-01T14:59:22Z

⚠ Flake8 diff showing the effect of this PR on typeshed:

Following python/typeshed#10123 and python/typeshed#10124, these are now the only hits I get if I run this PR's version of flake8-pyi on typeshed, after removing the F821 ignores from typeshed's .flake8 file.

Akuli

It looks good. Just one nit.

pyi.py

Co-authored-by: Akuli <[email protected]>

github-actions · 2023-05-01T15:19:38Z

⚠ Flake8 diff showing the effect of this PR on typeshed:

> ./stubs/protobuf/google/protobuf/compiler/plugin_pb2.pyi:128:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:255:148: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:336:150: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:657:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:919:142: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:936:144: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/descriptor_pb2.pyi:1170:165: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/type_pb2.pyi:125:133: NQA102 "# noqa: F821" has no matching violations
> ./stubs/protobuf/google/protobuf/type_pb2.pyi:212:147: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/service/hlo_pb2.pyi:806:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/service/hlo_pb2.pyi:1079:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:851:155: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1310:160: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1418:138: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/compiler/xla/xla_data_pb2.pyi:1576:153: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/api_def_pb2.pyi:50:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/dataset_options_pb2.pyi:135:162: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/framework/graph_transfer_info_pb2.pyi:196:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:348:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:375:164: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:686:182: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/config_pb2.pyi:1086:150: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/core_platform_payloads_pb2.pyi:32:158: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/data_service_pb2.pyi:59:165: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/data_service_pb2.pyi:163:161: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:70:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:118:152: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:135:168: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:154:154: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/saved_object_graph_pb2.pyi:475:152: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/saver_pb2.pyi:29:174: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/struct_pb2.pyi:334:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tensor_bundle_pb2.pyi:44:157: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/compilation_result_pb2.pyi:36:160: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:738:158: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:805:159: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/optimization_parameters_pb2.pyi:846:161: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/topology_pb2.pyi:31:170: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/tpu_embedding_configuration_pb2.pyi:30:153: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/tpu/tpu_embedding_configuration_pb2.pyi:51:177: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/protobuf/verifier_config_pb2.pyi:29:146: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/event_pb2.pyi:151:140: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/event_pb2.pyi:202:156: NQA102 "# noqa: F821" has no matching violations
> ./stubs/tensorflow/tensorflow/core/util/test_log_pb2.pyi:486:157: NQA102 "# noqa: F821" has no matching violations

nipunn1313 · 2023-05-01T18:01:01Z

Also FYI @nipunn1313 -- looks like we might have to update some autogenerated stubs made using mypy-protobuf if this is merged (but then, you did ask for a change like this in #278 😉).

Will mypy-protobuf itself need to be updated in order to produce stubs compatible with flake8-pyi following this change, or does it add the # noqa comments to the autogenerated stubs based on the output of flake8?

I don't totally understand the question nor what's going on this diff. Feel free to do whatever it takes to get it to land - and I can come play with it later.

mypy-protobuf is used to generate some files in typeshed - so it's possible there might be changes required - but I haven't dug in to verify yet. I don't know how it interacts with flake8 off the top of my head.

AlexWaygood · 2023-05-01T18:09:55Z

I don't totally understand the question nor what's going on this diff. Feel free to do whatever it takes to get it to land - and I can come play with it later.

Thanks @nipunn1313! Sorry my message above was unclear. I'll dig in in a bit, and file an issue over at mypy-protobuf if it's required 👍

AlexWaygood · 2023-05-01T18:10:14Z

Thanks again @tomasr8 for tackling this! It's a great improvement, I think.

tomasr8 · 2023-05-01T18:18:36Z

Thanks to you @AlexWaygood , @JelleZijlstra and @Akuli for all the reviews, suggestions and explanations! :)

Following #364, this method is now only called in one location in our code (and it doesn't exist on the superclass). We can just inline it. Refs #183

#364 means that pyflakes now doesn't emit nearly the same number of F822 false positives on stub files as it used too. However, the fix was made 'by accident', so we didn't document the change or add tests.

This comment has been minimized.

Sign in to view

tomasr8 added 2 commits April 13, 2023 17:28

Disallow forward refs for classes

fc6faea

Though technically allowed, this enforces that subclasses are defined after the classes they inherit from which improves readability.

Correctly handle forward references in annotations

9eeb64a

By default pyflakes requires a future import for annotations in order to process forward references in annotations. This makes it so that the explicit import is not needed, which gets rid of a lot of F821 errors.

tomasr8 force-pushed the class-forward-refs branch from e2175e3 to 9eeb64a Compare April 13, 2023 15:40

This comment has been minimized.

Sign in to view

Add more tests

d7b3761

This comment has been minimized.

Sign in to view

tomasr8 mentioned this pull request Apr 14, 2023

Use --extend-ignore instead of --ignore #366

Merged

AlexWaygood pushed a commit that referenced this pull request Apr 14, 2023

Use --extend-ignore instead of --ignore in tests (#366)

7e921e1

Came up in [#364](#364 (comment)) Plain `--ignore` resets the list of ignores which means that the config file is not taken into account.

Merge branch 'main' into class-forward-refs

f50b745

This comment has been minimized.

Sign in to view

AlexWaygood reviewed Apr 14, 2023

View reviewed changes

pyi.py Show resolved Hide resolved

AlexWaygood added 2 commits April 30, 2023 21:29

More expansive changelog entry

7718b09

More tests

f686ef7

AlexWaygood changed the title ~~Disallow forward refs for classes~~ Improve handling of forward references Apr 30, 2023

AlexWaygood approved these changes Apr 30, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

This was referenced May 1, 2023

Remove unnecessary F821 noqas python/typeshed#10123

Merged

Avoid unnecessary forward refs in class definitions python/typeshed#10124

Merged

AlexWaygood closed this May 1, 2023

AlexWaygood reopened this May 1, 2023

Akuli reviewed May 1, 2023

View reviewed changes

pyi.py Outdated Show resolved Hide resolved

Clarify comment

463f260

Co-authored-by: Akuli <[email protected]>

AlexWaygood merged commit 0f06cb2 into PyCQA:main May 1, 2023

tomasr8 deleted the class-forward-refs branch May 1, 2023 18:13

This was referenced May 1, 2023

Error for unnecessary "= ..." #98

Open

Get rid of PyiAwareFlakesChecker.deferHandleNode #373

Merged

AlexWaygood added a commit that referenced this pull request May 3, 2023

Get rid of PyiAwareFlakesChecker.deferHandleNode (#373)

cb05781

Following #364, this method is now only called in one location in our code (and it doesn't exist on the superclass). We can just inline it. Refs #183

This was referenced May 5, 2023

flake8: Enable F822 python/typeshed#10144

Merged

Document and test improvements to F822 #381

Merged

AlexWaygood mentioned this pull request Mar 12, 2024

F821 false positive for .pyi astral-sh/ruff#3011

Closed

Improve handling of forward references #364

Improve handling of forward references #364

Conversation

tomasr8 commented Apr 11, 2023 • edited by AlexWaygood Loading

This comment has been minimized.

tomasr8 commented Apr 11, 2023

AlexWaygood commented Apr 12, 2023 • edited Loading

tomasr8 commented Apr 12, 2023

AlexWaygood commented Apr 12, 2023 • edited Loading

This comment has been minimized.

tomasr8 commented Apr 13, 2023 • edited by AlexWaygood Loading

AlexWaygood commented Apr 13, 2023 • edited Loading

This comment has been minimized.

tomasr8 commented Apr 14, 2023

tomasr8 commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023 • edited Loading

This comment has been minimized.

tomasr8 commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023 • edited Loading

tomasr8 commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023 • edited Loading

tomasr8 commented Apr 14, 2023

JelleZijlstra commented Apr 14, 2023

AlexWaygood commented Apr 14, 2023 • edited Loading

tomasr8 commented Apr 27, 2023

AlexWaygood left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

AlexWaygood commented Apr 30, 2023 • edited Loading

AlexWaygood commented Apr 30, 2023

github-actions bot commented May 1, 2023

AlexWaygood commented May 1, 2023

Akuli left a comment

Choose a reason for hiding this comment

github-actions bot commented May 1, 2023

nipunn1313 commented May 1, 2023

AlexWaygood commented May 1, 2023

AlexWaygood commented May 1, 2023

tomasr8 commented May 1, 2023

tomasr8 commented Apr 11, 2023 •

edited by AlexWaygood

Loading

AlexWaygood commented Apr 12, 2023 •

edited

Loading

AlexWaygood commented Apr 12, 2023 •

edited

Loading

tomasr8 commented Apr 13, 2023 •

edited by AlexWaygood

Loading

AlexWaygood commented Apr 13, 2023 •

edited

Loading

AlexWaygood commented Apr 14, 2023 •

edited

Loading

AlexWaygood commented Apr 14, 2023 •

edited

Loading

AlexWaygood commented Apr 14, 2023 •

edited

Loading

AlexWaygood commented Apr 14, 2023 •

edited

Loading

AlexWaygood left a comment •

edited

Loading

AlexWaygood commented Apr 30, 2023 •

edited

Loading