Repository: guilatrova/tryceratops (language: python)

Issue:
New violation: Prefer TypeError for unexpected types 
Hi there,

A couple of times on existing code bases I've ran into a problem with built-in exceptions. I'm hoping to interest you into accepting it into this project, as it's the closest I've seen so far to the use case.

A simple GitHub search results in many codebases struggling with this pattern.

## The issue

Often built-in exceptions are used regardless of their semantics. This results in confusing diagnostic information for the user. 

```python
if isinstance(my_var, int):
    pass
else:
    raise ValueError("...") #should be typeerror
```

The above examples has variations with multiple elif statements and other exception types as RuntimeError. 

Similarly, if the if/elif conditions check values, not type, then a TypeError is misleading.

## Proposed solution

Detect these patterns and report them, ideally fix them automatically (as pyupgrade would do). 

Are you open to including this functionality in the package? If you're to busy, but open, then the community could take a turn ([hacktoberfest](https://hacktoberfest.digitalocean.com/)).


Discussion / hints:
Hi @sbrugman ! 👋
Thanks for trying Tryceratops 🦖  and sharing your thoughts.

### Violation definition

That's a great suggestion! The [Python docs](https://docs.python.org/3/library/exceptions.html#TypeError) makes the definition clear:

> Raised when an operation or function is applied to an object of inappropriate type. [...]
> This exception may be raised by user code to indicate that an attempted operation on an object is not supported [...]
> Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.

So it fits the description you just mentioned!  I'd suggest we create a new Violation: **TC004 - Prefer `TypeError` exception for invalid type**.

### Autofix support

I agree, we already have some implemented, so it should be fine.

### Variant support

I believe we should stick just to some classic built-in exception classes... But I'm not completely sure yet.
I'd recommend considering only the following classes:  `Exception`, `ValueError`, and `RuntimeError` as you mentioned. Do you feel strongly about including any other types?

I just want to ensure the developer can create a `class OperationXUnsupportedType(TypeError)` if he wishes.

### Implementation + Release

> Are you open to including this functionality in the package? 

Yes! 😄 

> If you're too busy, but open, then the community could take a turn (hacktoberfest).

I'm kind of busy due to my work and other side projects regarding microservices that I'm planning to do 😢 
so I can't commit to delivering it. I'll check how to include Tryceratops into the hacktoberfest! Thanks for suggesting!

Also, be my guest to implement it if you have the time! ❤️ 😄 
Hi there @guilatrova - mind if I take a crack at an implementation? 

@swellander Be my guest! Thanks for suggesting ❤️

Please, check out the [contributing guidelines](https://github.com/guilatrova/tryceratops/blob/259bf9c1eb357f2ebf737f5b3f9c52220645490f/docs/CONTRIBUTING.md), and take some time to understand how the commit messages play with the auto release setup.
@guilatrova sounds good, will do!
@swellander Any progress on this one? Feel free to start a _draft PR_ in case you'd like any help with this :)
Hi, sorry for the radio silence and PR delay! I will collect what I have done so far into a draft PR after I finish with work today. 

Fix patch:
diff --git a/docs/violations/README.md b/docs/violations/README.md
index 5d2ebd4..54304c1 100644
--- a/docs/violations/README.md
+++ b/docs/violations/README.md
@@ -7,6 +7,7 @@
 | ----------------- | ---------------------------------------------------------- |
 | [TC002](TC002.md) | Create your own exception                                  |
 | [TC003](TC003.md) | Avoid specifying long messages outside the exception class |
+| [TC004](TC004.md) | Prefer `TypeError` exception for invalid type              |
 
 ## `TC1xx` - General
 
diff --git a/docs/violations/TC004.md b/docs/violations/TC004.md
new file mode 100644
index 0000000..fe9d26c
--- /dev/null
+++ b/docs/violations/TC004.md
@@ -0,0 +1,31 @@
+# `TC004` - Prefer `TypeError` exception for invalid type
+
+## Why is it bad
+
+Using semantically incorrect exceptions results in confusing diagnostic information for the user.
+
+The Python docs make the definition clear:
+
+> Raised when an operation or function is applied to an object of inappropriate type. [...]
+This exception may be raised by user code to indicate that an attempted operation on an object is not supported [...]
+Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.
+
+[Source](https://docs.python.org/3/library/exceptions.html#TypeError)
+
+## How it looks like
+
+```py
+if isinstance(my_var, int):
+    pass
+else:
+    raise ValueError(f'{my_var} must be an int')
+```
+
+## How it should be
+
+```py
+if isinstance(my_var, int):
+    pass
+else:
+    raise TypeError(f'{my_var} must be an int')
+```
diff --git a/src/tryceratops/analyzers/__init__.py b/src/tryceratops/analyzers/__init__.py
index 0cc5929..db8ac17 100644
--- a/src/tryceratops/analyzers/__init__.py
+++ b/src/tryceratops/analyzers/__init__.py
@@ -2,7 +2,7 @@
 
 from typing import TYPE_CHECKING, Set
 
-from . import call, exception_block, try_block
+from . import call, conditional, exception_block, try_block
 from .base import BaseAnalyzer
 
 if TYPE_CHECKING:
@@ -14,6 +14,7 @@
     call.CallRaiseVanillaAnalyzer,  # type: ignore
     call.CallRaiseLongArgsAnalyzer,  # type: ignore
     call.CallAvoidCheckingToContinueAnalyzer,  # type: ignore
+    conditional.PreferTypeErrorAnalyzer,
     exception_block.ExceptReraiseWithoutCauseAnalyzer,
     exception_block.ExceptVerboseReraiseAnalyzer,
     exception_block.ExceptBroadPassAnalyzer,
diff --git a/src/tryceratops/analyzers/conditional.py b/src/tryceratops/analyzers/conditional.py
new file mode 100644
index 0000000..710ac7f
--- /dev/null
+++ b/src/tryceratops/analyzers/conditional.py
@@ -0,0 +1,64 @@
+import ast
+from typing import Union
+
+from tryceratops.violations import codes
+
+from .base import BaseAnalyzer
+
+STANDARD_NON_TYPE_ERROR_IDS = (
+    "ArithmeticError",
+    "AssertionError",
+    "AttributeError",
+    "BufferError",
+    "EOFError",
+    "Exception",
+    "ImportError",
+    "LookupError",
+    "MemoryError",
+    "NameError",
+    "ReferenceError",
+    "RuntimeError",
+    "SyntaxError",
+    "SystemError",
+    "ValueError",
+)
+
+
+class PreferTypeErrorAnalyzer(BaseAnalyzer):
+    violation_code = codes.PREFER_TYPE_ERROR
+
+    def _is_checking_type(self, node: Union[ast.stmt, ast.expr]) -> bool:
+        if isinstance(node, ast.If):
+            return self._is_checking_type(node.test) and all(
+                [self._is_checking_type(stm) for stm in node.orelse if isinstance(stm, ast.If)]
+            )
+        if isinstance(node, ast.UnaryOp):
+            return self._is_checking_type(node.operand)
+        if isinstance(node, ast.BoolOp):
+            return all([self._is_checking_type(value) for value in node.values])
+        if isinstance(node, ast.Call):
+            if isinstance(node.func, ast.Name):
+                if node.func.id in ("isinstance", "issubclass", "callable"):
+                    return True
+        return False
+
+    def _check_is_raise_other_than_typeerror(self, node: ast.Raise) -> None:
+        if isinstance(node.exc, ast.Call):
+            if isinstance(node.exc.func, ast.Name):
+                if node.exc.func.id in STANDARD_NON_TYPE_ERROR_IDS:
+                    self._mark_violation(node.exc.func)
+        elif isinstance(node.exc, ast.Name):
+            if node.exc.id in STANDARD_NON_TYPE_ERROR_IDS:
+                self._mark_violation(node.exc)
+
+    def _check_for_raises(self, node):
+        for stm in ast.iter_child_nodes(node):
+            if isinstance(stm, ast.Raise):
+                self._check_is_raise_other_than_typeerror(stm)
+        if isinstance(node, ast.If):
+            for stm in node.orelse:
+                self._check_for_raises(stm)
+
+    def visit_If(self, node: ast.If) -> None:
+        if self._is_checking_type(node):
+            self._check_for_raises(node)
diff --git a/src/tryceratops/violations/codes.py b/src/tryceratops/violations/codes.py
index 4b02d30..4a625fe 100644
--- a/src/tryceratops/violations/codes.py
+++ b/src/tryceratops/violations/codes.py
@@ -4,6 +4,7 @@
     "TC003",
     "Avoid specifying long messages outside the exception class",
 )
+PREFER_TYPE_ERROR = ("TC004", "Prefer TypeError exception for invalid type")
 
 # TC1xx - General
 CHECK_TO_CONTINUE = (


Test patch:
diff --git a/src/tests/analyzers_conditional_test.py b/src/tests/analyzers_conditional_test.py
new file mode 100644
index 0000000..af225bc
--- /dev/null
+++ b/src/tests/analyzers_conditional_test.py
@@ -0,0 +1,51 @@
+from functools import partial
+
+from tryceratops import analyzers
+from tryceratops.violations import codes
+
+from .analyzer_helpers import assert_violation, read_sample
+
+
+def test_prefer_type_error():
+    tree = read_sample("conditional_prefer_type_error")
+    analyzer = analyzers.conditional.PreferTypeErrorAnalyzer()
+
+    assert_type_error = partial(
+        assert_violation, codes.PREFER_TYPE_ERROR[0], codes.PREFER_TYPE_ERROR[1]
+    )
+
+    violations = analyzer.check(tree, "filename")
+
+    assert len(violations) == 31
+
+    assert_type_error(12, 14, violations[0])
+    assert_type_error(19, 14, violations[1])
+    assert_type_error(30, 14, violations[2])
+    assert_type_error(37, 14, violations[3])
+    assert_type_error(44, 14, violations[4])
+    assert_type_error(51, 14, violations[5])
+    assert_type_error(58, 14, violations[6])
+    assert_type_error(65, 14, violations[7])
+    assert_type_error(72, 14, violations[8])
+    assert_type_error(79, 14, violations[9])
+    assert_type_error(86, 14, violations[10])
+    assert_type_error(93, 14, violations[11])
+    assert_type_error(100, 14, violations[12])
+    assert_type_error(107, 14, violations[13])
+    assert_type_error(114, 14, violations[14])
+    assert_type_error(121, 14, violations[15])
+    assert_type_error(128, 14, violations[16])
+    assert_type_error(135, 14, violations[17])
+    assert_type_error(142, 14, violations[18])
+    assert_type_error(149, 14, violations[19])
+    assert_type_error(156, 14, violations[20])
+    assert_type_error(163, 14, violations[21])
+    assert_type_error(170, 14, violations[22])
+    assert_type_error(177, 14, violations[23])
+    assert_type_error(184, 14, violations[24])
+    assert_type_error(191, 14, violations[25])
+    assert_type_error(198, 14, violations[26])
+    assert_type_error(205, 14, violations[27])
+    assert_type_error(212, 14, violations[28])
+    assert_type_error(219, 14, violations[29])
+    assert_type_error(226, 14, violations[30])
diff --git a/src/tests/samples/violations/conditional_prefer_type_error.py b/src/tests/samples/violations/conditional_prefer_type_error.py
new file mode 100644
index 0000000..8eca31a
--- /dev/null
+++ b/src/tests/samples/violations/conditional_prefer_type_error.py
@@ -0,0 +1,258 @@
+"""
+Violation:
+
+Prefer TypeError when relevant.
+"""
+
+
+def incorrect_basic(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_multiple_type_check(some_arg):
+    if isinstance(some_arg, (int, str)):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+class MyClass:
+    pass
+
+
+def incorrect_with_issubclass(some_arg):
+    if issubclass(some_arg, MyClass):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_with_callable(some_arg):
+    if callable(some_arg):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_ArithmeticError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise ArithmeticError("...")  # should be typeerror
+
+
+def incorrect_AssertionError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise AssertionError("...")  # should be typeerror
+
+
+def incorrect_AttributeError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise AttributeError("...")  # should be typeerror
+
+
+def incorrect_BufferError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise BufferError("...")  # should be typeerror
+
+
+def incorrect_EOFError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise EOFError("...")  # should be typeerror
+
+
+def incorrect_ImportError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise ImportError("...")  # should be typeerror
+
+
+def incorrect_LookupError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise LookupError("...")  # should be typeerror
+
+
+def incorrect_MemoryError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise MemoryError("...")  # should be typeerror
+
+
+def incorrect_NameError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise NameError("...")  # should be typeerror
+
+
+def incorrect_ReferenceError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise ReferenceError("...")  # should be typeerror
+
+
+def incorrect_RuntimeError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise RuntimeError("...")  # should be typeerror
+
+
+def incorrect_SyntaxError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise SyntaxError("...")  # should be typeerror
+
+
+def incorrect_SystemError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise SystemError("...")  # should be typeerror
+
+
+def incorrect_ValueError(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise ValueError("...")  # should be typeerror
+
+
+def incorrect_not_operator_isinstance(some_arg):
+    if not isinstance(some_arg):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_and_operator_isinstance(arg1, arg2):
+    if isinstance(some_arg) and isinstance(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_or_operator_isinstance(arg1, arg2):
+    if isinstance(some_arg) or isinstance(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_multiple_operators_isinstance(arg1, arg2, arg3):
+    if not isinstance(arg1) and isinstance(arg2) or isinstance(arg3):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_not_operator_callable(some_arg):
+    if not callable(some_arg):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_and_operator_callable(arg1, arg2):
+    if callable(some_arg) and callable(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_or_operator_callable(arg1, arg2):
+    if callable(some_arg) or callable(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_multiple_operators_callable(arg1, arg2, arg3):
+    if not callable(arg1) and callable(arg2) or callable(arg3):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_not_operator_issubclass(some_arg):
+    if not issubclass(some_arg):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_and_operator_issubclass(arg1, arg2):
+    if issubclass(some_arg) and issubclass(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_or_operator_issubclass(arg1, arg2):
+    if issubclass(some_arg) or issubclass(arg2):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_multiple_operators_issubclass(arg1, arg2, arg3):
+    if not issubclass(arg1) and issubclass(arg2) or issubclass(arg3):
+        pass
+    else:
+        raise Exception("...")  # should be typeerror
+
+
+def incorrect_multi_conditional(arg1, arg2):
+    if isinstance(arg1, int):
+        pass
+    elif isinstance(arg2, int):
+        raise Exception("...")  # should be typeerror
+
+
+class MyCustomTypeValidation(Exception):
+    pass
+
+
+def correct_custom_exception(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise MyCustomTypeValidation("...")  # that's correct, because it's not vanilla
+
+
+def correct_complex_conditional(val):
+    if val is not None and (not isinstance(val, int) or val < 0):
+        raise ValueError(...)  # fine if this is not a TypeError
+
+
+def correct_multi_conditional(some_arg):
+    if some_arg == 3:
+        pass
+    elif isinstance(some_arg, int):
+        pass
+    else:
+        raise Exception("...")  # fine if this is not a TypeError
+
+
+def correct_should_ignore(some_arg):
+    if isinstance(some_arg, int):
+        pass
+    else:
+        raise TypeError("...")