Don't return with operand when conceptually void

[EliahKagan]

This changes return None to return where it appears in functions that are "conceptually void," i.e. cases where a function always returns None, its purpose is never to provide a return value to the caller, and it would be a bug (or at least confusing style) to use the return value or have the call as a subexpression.

This is already the prevailing style, so this is a consistency improvement, as well as improving clarity by avoiding giving the impression that "None" is significant in those cases.

Of course, functions that sometimes return values other than None do not have return None replaced with a bare return statement. Those are left alone (when they return None, it means something).

For the most part this is straightforward, but:

• The handle_stderr local function in IndexFile.checkout is also slightly further refactored to better express what the early return is doing.

  Because it is a local function, the diff in the "Files Changed" tab of this pull request makes it look like it is in the checkout method, where such a change would not be correct. Instead, that is the enclosing scope, and it is in a nested function where the change does make sense.

• The None operand is removed as usual in GitConfigParser.read, even though a superclass has a same-named method with a return type that is not None. Usually when this happens, the superclass method returns something like Optional[Something], and it is a case of return-type covariance, in which case the None operands should still be written. Here, however, the overriding method does not intend to be an override: it has an incompatible signature and cannot be called as if it were the superclass method, nor is its return type compatible.

• The None operand is retained in the code at the very end of git.cmd.handle_process_output, even though the return type is annotated as None, because the function also returns the expression finalizer(process). The argument finalizer is itself annotated to return None, but it looks like the intent may be to play along with the strange case that finalizer returns a non-None value, and forward it:

  GitPython/git/cmd.py

  Lines 207 to 210 in b8ee9be

  	if finalizer:
  	return finalizer(process)
  	else:
  	return None

  So changing return None to return doesn't make sense there. But depending on intent, maybe it should be just:

  if finalizer:
      finalizer(process)

I noticed after #1754, which made changes to Git.AutoInterrupt._terminate, was approved, that I had should have fixed how it used return in some places and return None in others. So I looked for return None that should be return throughout the project to see what else to fix along with it, resulting in this PR.

[Byron]

What a wonderful improvement, with a rationale I can absolutely get behind.

I wonder if -> None annotations should also be removed as it's probably clearer what the intention is if there is no return value at all, especially given the now 'bare' return statements.

[EliahKagan]

I wonder if -> None annotations should also be removed as it's probably clearer what the intention is if there is no return value at all, especially given the now 'bare' return statements.

I recommend against that, because the absence of a return-type annotation means -> Any.

Well, really it is more complicated than that:

• When a function has no parameter type annotations and no return type annotations, all parameter and return types are implicitly Any, and also the function is considered by static type checkers to be dynamically typed, causing code in it to be skipped over by type checkers.
• When a function has at least one parameter type annotation or a return type annotation, then any parameter or return type that is not annotated is implicitly Any, but the function is considered by static type checkers as statically typed and they check the code inside it. (Of course, Python is still dynamically typed; the static type system is still a separate type system with its own separate rules, such as int being a subtype of float.) Except...
• If the function is an __init__ method with at least one parameter type annotation and no return type annotation, then the return type is implicitly None. The idea here, as I understand it, is that although one should always write -> None even on __init__ when defining it with type annotations, this is often unintuitive, so if someone defines __init__ in a way that cause static type checkers to consider it statically typed but omits -> None, then -> None was intended.

(Note that a function can have no non-Any annotation yet still be considered statically typed by static type checkers, so long as at least one : Any or -> Any annotation is present. Code in the function itself can then be detected as having type errors, including some code that uses the dynamically typed parameters. For example, when x has type Any, introducing a by a = [x] causes it to have type list[Any], also called list, but operations lists don't support, e.g., a.add(10) or a.append(10, 20) or a + 3, will still be detected as type errors.)

[Byron]

Oh, I see! Thanks for elaborating. So -> None is actually the way to do it: None as type is like void, and not to be confused with None, the value.

[EliahKagan]

It's true that None in a type annotation means something different from None elsewhere, since the None object isn't a type. Its type, NoneType, can't be written easily, not being a builtin. Dynamically, one can write type(None). It is also possible to import NoneType from types, but annotating with it goes strongly enough against convention that static type checkers regard it as an error. (This is probably best, because if it were used, it might be with the intention of signifying something subtly different from None, yet its meaning as a static type annotation would not be able to be different.)

However, None as a return type really does express the type of the None object, and it is meaningful when the None object is a value that makes sense to think of as the result, as well as in the "conceptually void" case where it may be better to not think of there being a result.

The common case is in unions. One should not usually write Union[Something, None], because Optional[Something] is equivalent and nicer. But it is reasonable to write Something | None.

But there are also narrow situations where None by itself as a return type does not really mean what I am calling conceptually void. A simple but somewhat unsatisfying example:

def get_sentinel() -> None:
    """This used to vary. Now it doesn't but we have to have it for compatibility."""
    return None

The following is, I think, a more satisfying example, in which a -> None method in a derived class is not really conceptually void because it is a case of return-type covariance, and it will sometimes be called on an expression whose type is that of the base class, whose specific type the caller may not know:

from typing import override

class Message: ...

class Session: ...

class Connection:
    def create_send(self, message: Message) -> Session | None:
        ...

class EphemerealConnection(Connection):
    @override
    def create_send(self, message: Message) -> None:
        ...

(This is also valid without the @override annotation, which was introduced recently.)

[EliahKagan]

By the way, I have learned that there is a flake8-return plugin that includes rules to detect this sort of thing, in both directions, most of the time. However, I don't propose that plugin itself be added, since those rules are also built into ruff.