Repository: python/mypy
Pull request: Fix --warn-return-any for NotImplemented
fixes #4534 

Problem statement:
--warn-return-any should not complain on NotImplemented when using abc
I'm using `abc.abstractmethod`:
```python
@abc.abstractmethod
def encode(self) -> bytes:
    return NotImplemented
```

And I get a warning: `warning: Returning Any from function declared to return "bytes"`

I don't think something is wrong with the code, it's totally normal way to declare and describe an interface and mypy shouldn't complain about it.

Hints from issue discussion:
Why do you think this is a standard pattern? I always thought that `NotImplemented` is reserved for reversed operators (like `__radd__` vs `__add__`) and few other special methods (like `__subclasshook__`). I think the standard way of doing what you want is:
```python
@abc.abstractmethod
def encode(self) -> bytes:
    raise NotImplementedError("Not a valid encoder -- implement `encode` before use")
```
and I hope mypy doesn't complain about this (if yes, then _this_ should be fixed).
You have a point there. I wonder if the correct type for `NotImplemented`
shouldn't be `object` instead of `Any`. Can you try this (it's a one-line
change in typeshed) and submit a PR?

@gvanrossum It is not possible because `NotImplemented` is actually used in many `__op__` methods that return non-`Any` types. All such cases will be flagged as error by mypy if we type `NotImplemented` as `object`.
A simplest example is `__eq__`, it is required to return `bool`, and therefore a standard pattern like:
```python 
def __eq__(self, other: object) -> bool:
    if isinstance(other, type(self)):
        ...
    return NotImplemented
```
will be flagged as an error.
Right. Maybe `--disallow-any-expr` can be hacked to ignore this case?
> Maybe `--disallow-any-expr` can be hacked to ignore this case?

Yes, this is easy, but would not be my priority TBH, if OP wants to make a PR then OK.
@ilevkivskyi I've reread the docs and you are right, I shouldn't use `NotImplemented` here. I'll make a PR about `return NotImplemented` case.

Strangely abc docs doesn't say anything about what should a body of an abstract method look like. PEP 3119 doesn't either (and has an example with `pass`).

I guess I should raise an exception here, thank you.

Commit message: Fix --warn-return-any for NotImplemented

Patch to review:
diff --git a/.gitignore b/.gitignore
index 98cbe9c3e6be..02e39c0c3bcd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -14,6 +14,7 @@ docs/build/
 .cache
 .runtest_log.json
 dmypy.json
+.pytest_cache/
 
 # Packages
 *.egg
diff --git a/mypy/checker.py b/mypy/checker.py
index 8e4af18c17c3..23046ae6638b 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -59,6 +59,7 @@
 from mypy.meet import is_overlapping_types
 from mypy.options import Options
 from mypy.plugin import Plugin, CheckerPluginInterface
+from mypy.sharedparse import BINARY_MAGIC_METHODS
 
 from mypy import experiments
 
@@ -2180,7 +2181,10 @@ def check_return_stmt(self, s: ReturnStmt) -> None:
                     # (Unless you asked to be warned in that case, and the
                     # function is not declared to return Any)
                     if (self.options.warn_return_any and not self.current_node_deferred and
-                            not is_proper_subtype(AnyType(TypeOfAny.special_form), return_type)):
+                            not is_proper_subtype(AnyType(TypeOfAny.special_form),
+                                                  return_type) and
+                            not (defn.name() in BINARY_MAGIC_METHODS and
+                                 is_literal_not_implemented(s.expr))):
                         self.msg.incorrectly_returning_any(return_type, s)
                     return
 
@@ -3232,6 +3236,10 @@ def remove_optional(typ: Type) -> Type:
         return typ
 
 
+def is_literal_not_implemented(n: Expression) -> bool:
+    return isinstance(n, NameExpr) and n.fullname == 'builtins.NotImplemented'
+
+
 def builtin_item_type(tp: Type) -> Optional[Type]:
     """Get the item type of a builtin container.
 
diff --git a/mypy/sharedparse.py b/mypy/sharedparse.py
index 1b3e5a3a9460..91015b6d693f 100644
--- a/mypy/sharedparse.py
+++ b/mypy/sharedparse.py
@@ -16,7 +16,6 @@
     "__delitem__",
     "__divmod__",
     "__div__",
-    "__divmod__",
     "__enter__",
     "__exit__",
     "__eq__",
@@ -92,6 +91,52 @@
 
 MAGIC_METHODS_POS_ARGS_ONLY = MAGIC_METHODS - MAGIC_METHODS_ALLOWING_KWARGS
 
+BINARY_MAGIC_METHODS = {
+    "__add__",
+    "__and__",
+    "__cmp__",
+    "__divmod__",
+    "__div__",
+    "__eq__",
+    "__floordiv__",
+    "__ge__",
+    "__gt__",
+    "__iadd__",
+    "__iand__",
+    "__idiv__",
+    "__ifloordiv__",
+    "__ilshift__",
+    "__imod__",
+    "__imul__",
+    "__ior__",
+    "__ipow__",
+    "__irshift__",
+    "__isub__",
+    "__ixor__",
+    "__le__",
+    "__lshift__",
+    "__lt__",
+    "__mod__",
+    "__mul__",
+    "__or__",
+    "__pow__",
+    "__radd__",
+    "__rand__",
+    "__rdiv__",
+    "__rfloordiv__",
+    "__rlshift__",
+    "__rmod__",
+    "__rmul__",
+    "__ror__",
+    "__rpow__",
+    "__rrshift__",
+    "__rshift__",
+    "__rsub__",
+    "__rxor__",
+    "__sub__",
+    "__xor__",
+}
+
 
 def special_function_elide_names(name: str) -> bool:
     return name in MAGIC_METHODS_POS_ARGS_ONLY
diff --git a/test-data/unit/check-warnings.test b/test-data/unit/check-warnings.test
index 4acbfe69df80..d812ed83efa8 100644
--- a/test-data/unit/check-warnings.test
+++ b/test-data/unit/check-warnings.test
@@ -143,6 +143,21 @@ def f() -> int: return g()
 [out]
 main:4: warning: Returning Any from function declared to return "int"
 
+[case testReturnAnyForNotImplementedInBinaryMagicMethods]
+# flags: --warn-return-any
+class A:
+    def __eq__(self, other: object) -> bool: return NotImplemented
+[builtins fixtures/notimplemented.pyi]
+[out]
+
+[case testReturnAnyForNotImplementedInNormalMethods]
+# flags: --warn-return-any
+class A:
+    def some(self) -> bool: return NotImplemented
+[builtins fixtures/notimplemented.pyi]
+[out]
+main:3: warning: Returning Any from function declared to return "bool"
+
 [case testReturnAnyFromTypedFunctionWithSpecificFormatting]
 # flags: --warn-return-any
 from typing import Any, Tuple
diff --git a/test-data/unit/fixtures/notimplemented.pyi b/test-data/unit/fixtures/notimplemented.pyi
new file mode 100644
index 000000000000..e619a6c5ad85
--- /dev/null
+++ b/test-data/unit/fixtures/notimplemented.pyi
@@ -0,0 +1,13 @@
+# builtins stub used in NotImplemented related cases.
+from typing import Any, cast
+
+
+class object:
+    def __init__(self) -> None: pass
+
+class type: pass
+class function: pass
+class bool: pass
+class int: pass
+class str: pass
+NotImplemented = cast(Any, None)


Code review comments:
diff_hunk: @@ -14,6 +14,7 @@ docs/build/
 .cache
 .runtest_log.json
 dmypy.json
+.pytest_cache/
path: .gitignore
text: @user1:
You can add this to you own global .gitignore -- I don't see a reason to include this in mypy's .gitignore.