Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expression-scoped ignores in Python 3.8. #6648

Merged
merged 12 commits into from Apr 11, 2019

Conversation

@brandtbucher
Copy link
Contributor

commented Apr 9, 2019

Fixes #1032. On Python 3.8, we use end_lineno information to scope # type: ignore comments to enclosing expressions. Auto-formatter users, rejoice!

The --warn-unused-ignores semantics should be clear from the test cases. Basically, ignores in inner scopes take precedence over ignores in enclosing scopes, and the first of multiple ignores in a scope "wins".

A few notes:

  • This changes the type of Errors.ignored_lines from a set to a dict, which maps ignored lines to the line of the comment they originated from. This structure is necessary in order to support --warn-unused-ignores.
  • This adds a new slot, Context.end_line. It defaults to None in the absence of an end_lineno or if Context is not an expression.
  • ErrorInfo.origin now has a third member, representing the end line of the error context. It is used to determine the range of lines to search for # type: ignore comments. If unavailable, it defaults to the same value as the origin line.
  • Because this uses 3.8-only AST features, a new check-38.test file has been created, and is hidden behind a version guard in testcheck.py.
@ilevkivskyi
Copy link
Collaborator

left a comment

Thank you for PR!

Generally looks good, but I have few comments.

test-data/unit/check-38.test Show resolved Hide resolved
mypy/treetransform.py Outdated Show resolved Hide resolved
mypy/test/testcheck.py Outdated Show resolved Hide resolved
mypy/errors.py Outdated Show resolved Hide resolved
mypy/errors.py Outdated Show resolved Hide resolved
mypy/nodes.py Outdated Show resolved Hide resolved
mypy/fastparse.py Outdated Show resolved Hide resolved
mypy/fastparse.py Outdated Show resolved Hide resolved
mypy/fastparse.py Outdated Show resolved Hide resolved

brandtbucher added some commits Apr 9, 2019

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

Please let me know when this is ready for another review.

@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Thanks for the feedback @ilevkivskyi! I've gone ahead and addressed your concerns, and am ready for another review.

I've chosen to maintain a set of source comment lines as you suggested, in addition to the mapping of ignore scopes. One nice benefit is, with a change in the order of traversal, we can now avoid expensive dictionary copies at each ignored expression node.

@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks for updates! I have few more comments.

mypy/fastparse.py Outdated Show resolved Hide resolved
test-data/unit/check-38.test Outdated Show resolved Hide resolved
mypy/fastparse.py Outdated Show resolved Hide resolved
@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I'm ready for a new review @ilevkivskyi.

I've updated my implementation to use a new Context.end_line slot. It's much cleaner than my first attempt, and works as expected. I've also updated the tests, as you requested.

@brandtbucher brandtbucher changed the title Give ignores expression-wide scope in Python 3.8. Scoped ignores in Python 3.8. Apr 11, 2019

@brandtbucher brandtbucher changed the title Scoped ignores in Python 3.8. Expression-scoped ignores in Python 3.8. Apr 11, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

LGTM now!

@ilevkivskyi ilevkivskyi merged commit 153536b into python:master Apr 11, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@brandtbucher

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Thanks for the guidance @ilevkivskyi! Happy to contribute.

gvanrossum added a commit that referenced this pull request May 7, 2019

Fix class/def line reporting and ignoring for 3.8+ (#6753)
Fixes #3871. This change fixes a few things concerning class/function definition line numbers.

For users running mypy with Python 3.8+:
- Function definitions are now reported on the correct line, rather than the line of the first decorator plus the number of decorators.
- Class definitions are now reported on the correct line, rather than the line of the first decorator.
- **These changes are fully backward compatible with respect to `# type: ignore` lines**. In other words, existing comments will _not_ need to be moved. This is accomplished by defining an ignore "scope" just like we added to expressions with #6648. See the recent discussion at #3871 for reasoning.

For other users (running mypy with Python 3.7 or earlier):
- Class definition lines are reported by using the same decorator-offset estimate as functions. This is both more accurate, and necessary to get 15 or so tests to pass for both pre- and post-3.8 versions. This seems fine, since there [doesn't appear](#6539 (comment)) to be a compelling reason why it was different in the first place.
- **This change is also backward-compatible with existing ignores, as above.**

About 15 tests had to have their error messages / nodes moved down one line, and 4 new regression tests have been added to `check-38.test`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.