Fix error report highlight for unmatched type annotation #27195
Conversation
| def build_def(ctx, py_def, type_line, self_name=None): | ||
| body = py_def.body | ||
| r = ctx.make_range(py_def.lineno, py_def.col_offset, | ||
| r = ctx.make_range(py_def.lineno + len(py_def.decorator_list), |
There was a problem hiding this comment.
Without this change, the error highlight gets one line off, like:
RuntimeError:
Number of type annotations (2) did not match the number of function parameters (1):
at __scratch__/example.py:4:0
@torch.jit.script
~~~~~~~~~~~~~~~~~ <--- HERE
def f1(x):
# type: (int, int) -> None
pass
Essentially this changes SourceRange of Decl (and also Def) so that it excludes decorator lines.
As far as I can tell, SourceRange is only used for error reporting, so I thought this change is appropriate.
But, maybe I'm missing something (since I only grepped e.g. def.range or decl.range), so it would be great if someone could assess the effect of this change.
|
It seems python test code doesn't pass linter. I'll handle that. |
|
Flake8 lint error is fixed now. |
driazati
left a comment
driazati
left a comment
There was a problem hiding this comment.
Sorry for the long delay in reviewing, but this looks good! The test failures all seem flaky so this is good to land once the nit comment is addressed.
Thanks for the fix!
torch/csrc/jit/script/parser.cpp
Outdated
| } | ||
|
|
||
| TreeRef parseClass() { | ||
| TreeRef parseClass() { |
There was a problem hiding this comment.
nit: spurious whitespace, please revert
There was a problem hiding this comment.
Sorry about that. I reverted this change.
|
@driazati Thanks so much for reviewing! One question though. Can I squash my commits into single commit and also rebase it to latest master? |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
You shouldn't have to, our landing process will squash and rebase the whole PR automatically |
|
@driazati Wow, that's cool, I didn't know that. So, maybe nothing more I need to do here. I'll leave it to you then. Thanks! |
Summary: This PR fixes pytorch#25801 (see there for my verbose analysis). As an example, for the following code: ``` import torch torch.jit.script def f1(x): # type: (int, int) -> None pass ``` this PR will change error message from this: ``` RuntimeError: Number of type annotations (2) did not match the number of function parameters (1): # type: (int, int) -> None ``` to this: ``` RuntimeError: Number of type annotations (2) did not match the number of function parameters (1): at __scratch__/example.py:4:0 torch.jit.script def f1(x): ~~~~~~~~ <--- HERE # type: (int, int) -> None pass ``` Pull Request resolved: pytorch#27195 Differential Revision: D17910902 Pulled By: driazati fbshipit-source-id: af5c6353069d005752d6c7f0bd6a0c6db8437e55
6 participants
This PR fixes #25801 (see there for my verbose analysis).
As an example, for the following code:
this PR will change error message from this:
to this: