FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix class/def line reporting and ignoring for 3.8+. by brandtbucher · Pull Request #6753 · python/mypy · GitHub

  1. python
  2. mypy

Repository navigation

Fix class/def line reporting and ignoring for 3.8+. - #6753

Merged
gvanrossum merged 4 commits into
python:masterfrom
brandtbucher:def-lines
May 7, 2019
ConversationCommits (4)ChecksFiles changed
Merged

Fix class/def line reporting and ignoring for 3.8+.#6753
gvanrossum merged 4 commits into
python:masterfrom
brandtbucher:def-lines

Conversation

brandtbucher commented May 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 Expression-scoped ignores in Python 3.8. #6648. See the recent discussion at Function line number information is not always correct #3871 for reasoning.

For other users:

  • 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 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.

brandtbucher commented May 6, 2019

Copy link
Copy Markdown
Member Author

Bump. Hopefully this doesn't get too stale from all of the new sprint PRs!

gvanrossum commented May 6, 2019

Copy link
Copy Markdown
Member

Alas, it's already stale. Could you merge the conflicts?

brandtbucher commented May 6, 2019

Copy link
Copy Markdown
Member Author

@gvanrossum Merged, tests are passing.

gvanrossum reviewed May 6, 2019
View reviewed changes

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is getting close, but I have a couple of questions, since this looks like it will be fairly invasive...

Comment thread mypy/fastparse.py Outdated
# the opposite end of the range to search for "# type: ignore"
# comments in. This reports the correct line in the event of an
# error, but keeps old pre-3.8 "# type: ignore" comments working
# correctly.

gvanrossum May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This comment confused me. The key thing to note here is that we want to point lineno to the line that has the def keyword, while we want to point end_lineno to the line that would have been used for Python 3.7 and before. And we must compute end_lineno (i.e. the "old start line") using the crufty way given here since the old way of computing it (i.e. n.lineno + len(n.decorator_list) won't have the same result in 3.8.

Sorry that this is still so confusing. The code is right!

gvanrossum May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are you going to update these comments? I hope so!

brandtbucher May 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'll try to reword this to be more concise. Maybe something like:

# Set end_lineno to the old pre-3.8 lineno, in order to keep
# existing "# type: ignore" comments working:

gvanrossum May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sounds good -- also for the other location!

Comment thread mypy/fastparse.py Outdated
# comments in. This reports the correct line in the event of an
# error, but keeps old pre-3.8 "# type: ignore" comments working
# correctly.
lineno = n.lineno

gvanrossum May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you even need this? It seems this was already set on line 432 above.

brandtbucher May 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice catch. I believe that's left over from an earlier iteration of this change.

Comment thread mypy/fastparse.py Outdated
# lineno is where the error is reported, and end_lineno defines the
# opposite end of the range to search for "# type: ignore" comments in.
# This reports the correct line in the event of an error, but keeps old
# pre-3.8 "# type: ignore" comments working correctly.

gvanrossum May 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Similar comment (except I wasn't really confused here, since I had just figured out the previous one :-).

brandtbucher May 6, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As above:

# Set end_lineno to the old mypy 0.700 lineno, in order to keep
# existing "# type: ignore" comments working:

Comment thread test-data/unit/check-attr.test

brandtbucher commented May 6, 2019

Copy link
Copy Markdown
Member Author

New changes are passing, ready for another review.

gvanrossum merged commit 2f99e09 into python:master May 7, 2019

gvanrossum commented May 7, 2019

Copy link
Copy Markdown
Member

Thanks! Congratulations on getting this thorny PR in. Excellent work.

brandtbucher commented May 7, 2019

Copy link
Copy Markdown
Member Author

Thanks, that really means a lot coming from you. Always happy to help!

brandtbucher deleted the def-lines branch May 7, 2019 01:16
brandtbucher mentioned this pull request Aug 22, 2019
Closed
hauntsaninja mentioned this pull request Aug 3, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

gvanrossum gvanrossum left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Function line number information is not always correct

2 participants

Back | FazBrowse Home | New Git URL