Add lint for unqualified `noqa` by samestep · Pull Request #56272 · pytorch/pytorch

samestep · 2021-04-16T15:37:01Z

As this diff shows, currently there are a couple hundred instances of raw noqa in the codebase, which just ignore all errors on a given line. That isn't great, so this PR changes all existing instances of that antipattern to qualify the noqa with respect to a specific error code, and adds a lint to prevent more of this from happening in the future.

Interestingly, some of the examples the noqa lint catches are genuine attempts to qualify the noqa with a specific error code, such as these two:

test/jit/test_misc.py:27:            print(f"{hello + ' ' + test}, I'm a {test}") # noqa E999
test/jit/test_misc.py:28:            print(f"format blank") # noqa F541

However, those are still wrong because they are missing a colon, which actually causes the error code to be completely ignored:

If you change them to anything else, the warnings will still be suppressed.

If you add the necessary colons then it is revealed that E261 was also being suppressed, unintentionally:

test/jit/test_misc.py:27:57: E261 at least two spaces before inline comment
test/jit/test_misc.py:28:35: E261 at least two spaces before inline comment

I did try using flake8-noqa instead of a custom git grep lint, but it didn't seem to work. This PR is definitely missing some of the functionality that flake8-noqa is supposed to provide, though, so if someone can figure out how to use it, we should do that instead.

Test plan:

CI should pass on the tip of this PR, and we know that the lint works because the following CI run (before this PR was finished) failed:

https://github.com/pytorch/pytorch/runs/2365189927

facebook-github-bot · 2021-04-16T15:37:39Z

💊 CI failures summary and remediations

As of commit e624532 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

facebook-github-bot · 2021-04-16T19:57:05Z

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2021-04-19T15:37:18Z

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

janeyx99

What cool features are we missing without using flake8-noqa? This current lint seems functional enough.

samestep · 2021-04-19T16:18:54Z

What cool features are we missing without using flake8-noqa? This current lint seems functional enough.

@janeyx99 This git grep-based lint misses out on the following flake8-noqa error codes (I've copied just the relevant subset of the table here):

Code Message

NQA005 "# noqa: X000, X000" has duplicate codes, remove X000

NQA102 "# noqa: X000" has no matching violations

NQA103 "# noqa: X000, X001" has unmatched code(s), remove X001

facebook-github-bot · 2021-04-19T16:26:25Z

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

janeyx99 · 2021-04-19T19:30:32Z

What cool features are we missing without using flake8-noqa? This current lint seems functional enough.

@janeyx99 This git grep-based lint misses out on the following flake8-noqa error codes (I've copied just the relevant subset of the table here):

Code
Message

NQA005
"# noqa: X000, X000" has duplicate codes, remove X000

NQA102
"# noqa: X000" has no matching violations

NQA103
"# noqa: X000, X001" has unmatched code(s), remove X001

Yea I just tried the flake8-noqa locally and it did not output any errors for me either :/

facebook-github-bot · 2021-04-19T20:17:45Z

@samestep merged this pull request in e3900d2.

Summary: The other half of #56272. Pull Request resolved: #56290 Test Plan: CI should pass on the tip of this PR, and we know that the lint works because the following CI runs (before this PR was finished) failed: - https://github.com/pytorch/pytorch/runs/2384511062 - https://github.com/pytorch/pytorch/actions/runs/765036024 Reviewed By: seemethere Differential Revision: D27867219 Pulled By: samestep fbshipit-source-id: e648f07b6822867e70833e23ddafe7fb7eaca235

Summary: As this diff shows, currently there are a couple hundred instances of raw `noqa` in the codebase, which just ignore all errors on a given line. That isn't great, so this PR changes all existing instances of that antipattern to qualify the `noqa` with respect to a specific error code, and adds a lint to prevent more of this from happening in the future. Interestingly, some of the examples the `noqa` lint catches are genuine attempts to qualify the `noqa` with a specific error code, such as these two: ``` test/jit/test_misc.py:27: print(f"{hello + ' ' + test}, I'm a {test}") # noqa E999 test/jit/test_misc.py:28: print(f"format blank") # noqa F541 ``` However, those are still wrong because they are [missing a colon](https://flake8.pycqa.org/en/3.9.1/user/violations.html#in-line-ignoring-errors), which actually causes the error code to be completely ignored: - If you change them to anything else, the warnings will still be suppressed. - If you add the necessary colons then it is revealed that `E261` was also being suppressed, unintentionally: ``` test/jit/test_misc.py:27:57: E261 at least two spaces before inline comment test/jit/test_misc.py:28:35: E261 at least two spaces before inline comment ``` I did try using [flake8-noqa](https://pypi.org/project/flake8-noqa/) instead of a custom `git grep` lint, but it didn't seem to work. This PR is definitely missing some of the functionality that flake8-noqa is supposed to provide, though, so if someone can figure out how to use it, we should do that instead. Pull Request resolved: pytorch#56272 Test Plan: CI should pass on the tip of this PR, and we know that the lint works because the following CI run (before this PR was finished) failed: - https://github.com/pytorch/pytorch/runs/2365189927 Reviewed By: janeyx99 Differential Revision: D27830127 Pulled By: samestep fbshipit-source-id: d6dcf4f945ebd18cd76c46a07f3b408296864fcb

Summary: The other half of pytorch#56272. Pull Request resolved: pytorch#56290 Test Plan: CI should pass on the tip of this PR, and we know that the lint works because the following CI runs (before this PR was finished) failed: - https://github.com/pytorch/pytorch/runs/2384511062 - https://github.com/pytorch/pytorch/actions/runs/765036024 Reviewed By: seemethere Differential Revision: D27867219 Pulled By: samestep fbshipit-source-id: e648f07b6822867e70833e23ddafe7fb7eaca235

Add lints for unqualified noqa and type ignore

7a8adc7

facebook-github-bot added the cla signed label Apr 16, 2021

samestep added 4 commits April 16, 2021 11:00

Also unqualified ignores not at end of line

6fa71d3

Don't allow Flake8 error code partial prefixes

8d7b062

Only flag genuine noqa comments

3dfc27a

Fix all unqualified noqa

25962b9

samestep mentioned this pull request Apr 16, 2021

Add lint for unqualified type: ignore #56290

Closed

Split out type ignore lint into #56290

26928b5

samestep changed the title ~~Add lints for unqualified noqa and type: ignore~~ Add lint for unqualified noqa Apr 16, 2021

samestep marked this pull request as ready for review April 16, 2021 19:57

samestep requested review from BowenBao, H-Huang, albanD, jbschlosser, mingzhe09088, mrshenli, neginraoof, pritamdamania87, rohan-varma, spandantiwari, wayi1 and zhaojuanmao as code owners April 16, 2021 19:57

samestep requested a review from a team April 16, 2021 19:57

samestep added 2 commits April 19, 2021 08:27

Merge branch 'viable/strict' into lint-unqualified-lint-suppression

7606a38

Re-add intentional E265 violations in test_jit.py

b5843da

janeyx99 approved these changes Apr 19, 2021

View reviewed changes

Fix FB-internal Flake8 lints

e624532

facebook-github-bot closed this in e3900d2 Apr 19, 2021

facebook-github-bot added the Merged label Apr 19, 2021

samestep deleted the lint-unqualified-lint-suppression branch April 19, 2021 20:19

ezyang mentioned this pull request Apr 22, 2021

[external codegen] better yaml error messaging, added explicit error message tests #56597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint for unqualified `noqa`#56272

Add lint for unqualified `noqa`#56272
samestep wants to merge 9 commits intopytorch:masterfrom
samestep:lint-unqualified-lint-suppression

samestep commented Apr 16, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Apr 16, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Apr 16, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

janeyx99 left a comment

Uh oh!

samestep commented Apr 19, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

janeyx99 commented Apr 19, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

samestep commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

facebook-github-bot commented Apr 16, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

janeyx99 left a comment

Choose a reason for hiding this comment

Uh oh!

samestep commented Apr 19, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

janeyx99 commented Apr 19, 2021

Uh oh!

facebook-github-bot commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

samestep commented Apr 16, 2021 •

edited

Loading

facebook-github-bot commented Apr 16, 2021 •

edited

Loading