fix(checker): Improve precision on span of missing return type error to function header#1882
fix(checker): Improve precision on span of missing return type error to function header#1882luffy-orf wants to merge 3 commits into
Conversation
maximilianruesch
left a comment
There was a problem hiding this comment.
Hey, thank you for the PR! The error message is definitely improved!
I am a bit careful about the slightly hacky approach (with the manual parsing), but I see that the AST module does not expose enough information to handle things in another way really. Nonetheless, I have some suggestions to improve the code as well as some suggestions!
| if source is None or file is None or line_offset is None: | ||
| if func_def.body: | ||
| return Span(start, to_span(func_def.body[0]).start) | ||
| return Span(start, to_span(func_def).end) |
There was a problem hiding this comment.
If this is a possibility for the user, you may want to add test cases covering this (or assert that they are present already), and if it is not, assert that these cases are not present and move on?
| if source is None or file is None or line_offset is None: | ||
| if func_def.body: | ||
| return Span(start, to_span(func_def.body[0]).start) | ||
| return Span(start, to_span(func_def).end) |
There was a problem hiding this comment.
| if source is None or file is None or line_offset is None: | |
| if func_def.body: | |
| return Span(start, to_span(func_def.body[0]).start) | |
| return Span(start, to_span(func_def).end) | |
| if source is None or file is None or line_offset is None: | |
| if func_def.body: | |
| return Span(start, to_span(func_def.body[0]).start) | |
| return to_span(func_def) |
The work is done anyway, and if I understand correctly these should be equivalent. Imo the intention of "giving up" is more understandable this way.
|
|
||
|
|
||
| def function_header_span(func_def: ast.FunctionDef) -> Span: | ||
| """Returns a span covering only the function header up to and including `:`.""" |
There was a problem hiding this comment.
Also describe cases where this is not possible, i.e. what the span will cover when one cannot identify the header span. So perhaps something like (with suitable line breaks for max line length):
| """Returns a span covering only the function header up to and including `:`.""" | |
| """Returns a span covering only the function header up to and including `:` on a best-effort basis, falling back to the full function definition.""" |
However, if you can prove that your below cases can all make the span exact (via a short argument), you do not need to add this.
| for col in range(col_begin, len(lines[i])): | ||
| char = lines[i][col] |
There was a problem hiding this comment.
| for col in range(col_begin, len(lines[i])): | |
| char = lines[i][col] | |
| for char in lines[i][col_begin:]: |
A bit of Python splicing magic. Also handles out of bounds equally as gracefullly. Probably improves performance a touch too, but I am not worried about that here (we are already exclusively in a failure case).
You can do a very similar thing to the outer loop.
| file = get_file(func_def) | ||
| line_offset = get_line_offset(func_def) | ||
| if source is None or file is None or line_offset is None: | ||
| if func_def.body: |
There was a problem hiding this comment.
Can you be more explicit here, i.e.
| if func_def.body: | |
| if len(func_def.body) > 0: |
|
@maximilianruesch Thanks for the review! Addressed all suggestions:
|
|
As an alternative approach #1846 tackles a similar problem using subcomponents of the AST node. In that PR I specifically wanted to include the decorators, which you might not want here, but exclude the function body (the main problem with falling back to the definition!). Does that work here? |
|
@acl-cqc I cannot immediately see how that changeset is relevant here. Specifically you improve error messages at call sites, whereas here we improve error messages at function definition sites. I also could not find measures taken in that PR that improve on function definition sites. Could you elaborate on this please? |
Summary
:) forMissingReturnAnnotationError, instead of the entire function definitionfunction_header_span()inspan.pyto compute the correct diagnostic spanTest plan
uv run pytest tests/error/test_misc_errors.py -k "return_not_annotated" -vuv run pytest tests/error/test_misc_errors.py -vuv run ruff check guppylang-internals/src/guppylang_internals/span.py guppylang-internals/src/guppylang_internals/checker/func_checker.py