Why report entire functions as an error on "missing return statement"

[shaperilio]

If mypy --show-column-numbers --show-error-end thinks a function is missing a return statement, the entire function is the error. This is also the case for note: <something> defined here which appears, for example, when you call a function with the wrong arguments.

I think one could argue that reporting the entire function as the location of the note/error is not more helpful than, for example, reporting just the function signature (or even just the function name) as the error range.

It's not as much an issue when looking at command line output, but more visual tools (e.g. VSCode) use --show-column-numbers --show-error-end to highlight errors. This results in entire functions being highlighted, which is very invasive and makes it hard to work, for example, if mypy runs while I'm in the middle of writing a function, and the entire region I'm typing in is suddenly alarmingly red.

Comments are not included as part of the error if they are at the end of the function-in-progress, which makes sense, but then, if the point is to show that the problem is "anywhere in this block of code", you could argue an indented comment should probably be included.

I tried to argue that this is a visualization problem on the editor's side, but they argue back that they don't want to interpret your output in specific cases and thus have to chase changes you make, which is also valid.

Is there a real need to report entire functions as a problem or a note?

(An example of how VSCode shows this)

[shaperilio]

A couple of related examples:

Forgotten self

class Boo:
    def forgot_self(an_arg: int) -> None:
          ...

The mypy error "Self argument missing for non-static method" spans the entire method.

Forgotten self, but no arguments at all

class Boo:
    def forgot_self() -> None:
          ...

The error "Method must have at least one argument, did you forget self?" spans only the character d in def.

I would argue all of these should just span the function / method definition, i.e. from d to :.

[hauntsaninja]

Seems like a reasonable request. PR welcome!

[mschoettle]

Here is the referenced issue for the vscode mypy extension: microsoft/vscode-mypy#262

Another example is "Function is missing a return type annotation" which also reports the entire range of the function instead of just the function declaration.

@hauntsaninja Do you have a pointer on where one could start poking around to fix this?

[JelleZijlstra]

I would start by grepping for the error message to find where it's raised. Then (possibly going a few levels up the call stack) you should be able to find the AST node that is used to construct the location for the error, and adjust the logic to use a smaller span.

[shaperilio]

I would start by grepping for the error message to find where it's raised. Then (possibly going a few levels up the call stack) you should be able to find the AST node that is used to construct the location for the error, and adjust the logic to use a smaller span.

I actually ran out of steam pretty quickly when I saw these things were coming from AST. It did not seem easy, at least the way I was trying to do it. My branch is here if anyone is interested in what I tried.

(Edit: link to my branch was wrong.)

[mschoettle]

Thanks @JelleZijlstra!

I started by adding a test case which is in this draft PR: #17006. @shaperilio we can add the other ones you found as well.

I found the error messages and codes in message_registry.py. The particular one I am interested in is raised in checker.py:

mypy/mypy/checker.py

Line 1509 in 4259e37

self.fail(message_registry.FUNCTION_TYPE_EXPECTED, fdef)

The end line and column for the AST node seem to be set here when visiting a function definition:

mypy/mypy/fastparse.py

Lines 990 to 991 in 4259e37

	end_line = getattr(n, "end_lineno", None)
	end_column = getattr(n, "end_col_offset", None)

I am not sure if this is the right place to add extra logic or change the end since the function definition spans the whole block. Would it be more appropriate to adjust the end for specific error codes?

[JelleZijlstra]

I don't think we should change this in fastparse, because the end lineno is correct for the node. Instead, we should change the logic only where the error is raised.

[Hnasar]

no-untyped-def is another error code that unnecessarily highlights the entire function

[shaperilio]

The end line and column for the AST node seem to be set here when visiting a function definition:

mypy/mypy/fastparse.py

Lines 990 to 991 in 4259e37

	end_line = getattr(n, "end_lineno", None)
	end_column = getattr(n, "end_col_offset", None)

I am not sure if this is the right place to add extra logic or change the end since the function definition spans the whole block. Would it be more appropriate to adjust the end for specific error codes?

This is exactly where I had ended up, and I had modified fastparse.py but it seemed a bit hacky (see here; I couldn't figure out how to get anything better directly from AST.

[mschoettle]

I have a draft PR #17006 that adjusts the line/column in messages.py for FuncDef. What is missing is the column end. It seems that @shaperilio figured out how do determine that which we could add if this is something that is needed.

@JelleZijlstra Would you be able to take a quick look at the PR and let me know if the overall approach goes in the right direction?

[shaperilio]

I have a draft PR #17006 that adjusts the line/column in messages.py for FuncDef. What is missing is the column end. It seems that @shaperilio figured out how do determine that which we could add if this is something that is needed.

I made a draft PR (#17203) as well just to see what I had done more clearly. Turns out my changes to fastparse.py were no good (not surprising).

@mschoettle I'll add some test cases as you suggested and we'll go from there.

[mschoettle]

@shaperilio Thanks to your initial test and Jelle's suggestion on the PR I got on the right track and figured something out. I introduced new properties and determine the def's end line and column using the return and arguments (in that order). The fallback right now is to use the start column. There is also the possibility of calculating to the end of the function name (column + 4 + length of name) or the already existing end (which would highlight the whole function).

I'll still have to check if the other cases you described are fixed with the current changes or not.

[mschoettle]

@shaperilio Added some before and after screenshots from vscode with some examples in the PR referenced above.