ref(dev): use sentry-flake8 as a flake8:local-plugins plugin (#33830)

since the plugin is very specific to getsentry/sentry itself and the getsentry/sentry-flake8 repo is currently broken

.github/workflows/development-environment.yml

@@ -11,6 +11,7 @@ on:
       - '.envrc'
       - 'Brewfile'
       - 'scripts/*'
+      - 'tools/*'
       - 'src/sentry/runner/commands/devserver.py'
       - 'src/sentry/runner/commands/devservices.py'
       - 'bin/load-mocks'
@@ -105,6 +106,24 @@ jobs:
           source .venv/bin/activate
           [[ $(python -V) == "Python $(cat .python-version)" ]]
 
+  tools-tests:
+    runs-on: ubuntu-20.04
+    timeout-minutes: 5
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-python@v3
+        with:
+          python-version: 3.8.12
+          cache: 'pip'
+          cache-dependency-path: |
+            requirements-dev.txt
+            requirements-pre-commit.txt
+      - name: install dependencies
+        run: pip install -r requirements-dev.txt -r requirements-pre-commit.txt
+      - name: run tests
+        run: make test-tools
+      - name: Handle artifacts
+        uses: ./.github/actions/artifacts
 
   # We don't yet test the bootstrap step
   # https://github.com/getsentry/bootstrap-sentry/blob/7af557be84920dd587e48613dbc308c937bc0e08/bootstrap.sh#L618-L619

Makefile

@@ -134,6 +134,11 @@ test-snuba:
 	pytest tests/snuba tests/sentry/eventstream/kafka tests/sentry/snuba/test_discover.py tests/sentry/search/events -vv --cov . --cov-report="xml:.artifacts/snuba.coverage.xml" --junit-xml=".artifacts/snuba.junit.xml"
 	@echo ""
 
+test-tools:
+	@echo "--> Running tools tests"
+	pytest -c /dev/null --confcutdir tests/tools tests/tools -vv --cov=tools --cov=tests/tools --cov-report="xml:.artifacts/tools.coverage.xml" --junit-xml=".artifacts/tools.junit.xml"
+	@echo ""
+
 backend-typing:
 	@echo "--> Running Python typing checks"
 	mypy --strict --warn-unreachable --config-file mypy.ini

requirements-pre-commit.txt

@@ -1,6 +1,7 @@
 pre-commit==2.15.0
 black==21.9b0
-sentry-flake8==2.0.0
+flake8==3.9.2
+flake8-bugbear==21.4.3
 pyupgrade==2.29.0
 isort==5.9.3
 requirements-parser>=0.2.0

setup.cfg

@@ -9,6 +9,11 @@ ignore = F999,E203,E501,E128,E124,E402,W503,W504,E731,C901,B007,B009,B010,B011
 # We already have a lot of E501's - these are lines black didn't wrap.
 # But rather than append # noqa: E501 to all of them, we just ignore E501 for now.
 
+[flake8:local-plugins]
+paths = .
+extension =
+    S=tools.flake8_plugin:SentryCheck
+
 [bdist_wheel]
 python-tag = py38
 

tests/tools/flake8_plugin_test.py

@@ -0,0 +1,82 @@
+import ast
+
+from tools.flake8_plugin import S001, S002, SentryCheck
+
+
+def _run(src):
+    lines = src.splitlines(True)
+    tree = ast.parse(src)
+    return list(SentryCheck(filename="example.py", tree=tree, lines=lines).run())
+
+
+def _errors(*errors):
+    return [SentryCheck.adapt_error(e) for e in errors]
+
+
+def test_S001():
+    S001_py = """\
+class A:
+    def assert_called_once():
+        pass
+
+
+A().assert_called_once()
+"""
+
+    errors = _run(S001_py)
+    assert errors == _errors(S001(6, 0, vars=("assert_called_once",)))
+
+
+def test_S002():
+    S002_py = """\
+print("print statements are not allowed")
+"""
+
+    errors = _run(S002_py)
+    assert errors == _errors(S002(1, 0))
+
+
+def test_S003():
+    S003_py = """\
+import json
+import simplejson
+from json import loads, load
+from simplejson import JSONDecoder, JSONDecodeError, _default_encoder
+import sentry.utils.json as good_json
+from sentry.utils.json import JSONDecoder, JSONDecodeError
+
+
+def bad_code():
+    a = json.loads("''")
+    b = simplejson.loads("''")
+    c = loads("''")
+    d = load()
+"""
+
+    errors = _run(S003_py)
+    assert errors == [
+        (
+            1,
+            0,
+            "S003: Use ``from sentry.utils import json`` instead.",
+            SentryCheck,
+        ),
+        (
+            2,
+            0,
+            "S003: Use ``from sentry.utils import json`` instead.",
+            SentryCheck,
+        ),
+        (
+            3,
+            0,
+            "S003: Use ``from sentry.utils import json`` instead.",
+            SentryCheck,
+        ),
+        (
+            4,
+            0,
+            "S003: Use ``from sentry.utils import json`` instead.",
+            SentryCheck,
+        ),
+    ]

tools/flake8_plugin.py

@@ -0,0 +1,177 @@
+import ast
+from collections import namedtuple
+from functools import partial
+
+import pycodestyle
+
+__version__ = "2.0.0"
+
+
+# We don't want Python 3 code to have Python 2 compatability futures.
+# Refer to Lib/__future__.py in CPython source.
+DISALLOWED_FUTURES = (
+    "absolute_import",
+    "division",
+    "print_function",
+    "unicode_literals",
+)
+
+
+class SentryVisitor(ast.NodeVisitor):
+    NODE_WINDOW_SIZE = 4
+
+    def __init__(self, filename, lines):
+        self.errors = []
+        self.filename = filename
+        self.lines = lines
+        self.node_stack = []
+        self.node_window = []
+
+    def visit(self, node):
+        self.node_stack.append(node)
+        self.node_window.append(node)
+        self.node_window = self.node_window[-self.NODE_WINDOW_SIZE :]
+        super().visit(node)
+        self.node_stack.pop()
+
+    def visit_ImportFrom(self, node):
+        if node.module == "__future__":
+            for nameproxy in node.names:
+                if nameproxy.name in DISALLOWED_FUTURES:
+                    self.errors.append(S005(node.lineno, node.col_offset))
+                    break
+
+        if node.module in S003.modules:
+            for nameproxy in node.names:
+                if nameproxy.name in S003.names:
+                    self.errors.append(S003(node.lineno, node.col_offset))
+                    break
+
+    def visit_Import(self, node):
+        for alias in node.names:
+            if alias.name.split(".", 1)[0] in S003.modules:
+                self.errors.append(S003(node.lineno, node.col_offset))
+
+    def visit_Call(self, node):
+        if isinstance(node.func, ast.Attribute):
+            for bug in (S004,):
+                if node.func.attr in bug.methods:
+                    call_path = ".".join(self.compose_call_path(node.func.value))
+                    if call_path in bug.invalid_paths:
+                        self.errors.append(bug(node.lineno, node.col_offset))
+                    break
+        self.generic_visit(node)
+
+    def visit_Attribute(self, node):
+        if node.attr in S001.methods:
+            self.errors.append(S001(node.lineno, node.col_offset, vars=(node.attr,)))
+
+    def visit_Name(self, node):
+        if node.id == "print":
+            self.check_print(node)
+
+    def visit_Print(self, node):
+        self.check_print(node)
+
+    def check_print(self, node):
+        if not self.filename.startswith("tests/"):
+            self.errors.append(S002(lineno=node.lineno, col=node.col_offset))
+
+    def compose_call_path(self, node):
+        if isinstance(node, ast.Attribute):
+            yield from self.compose_call_path(node.value)
+            yield node.attr
+        elif isinstance(node, ast.Name):
+            yield node.id
+
+
+class SentryCheck:
+    name = "sentry-flake8"
+    version = __version__
+
+    def __init__(self, tree=None, filename=None, lines=None, visitor=SentryVisitor):
+        self.tree = tree
+        self.filename = filename
+        self.lines = lines
+        self.visitor = visitor
+
+    def run(self):
+        if not self.tree or not self.lines:
+            self.load_file()
+
+        visitor = self.visitor(filename=self.filename, lines=self.lines)
+        visitor.visit(self.tree)
+
+        for e in visitor.errors:
+            try:
+                if pycodestyle.noqa(self.lines[e.lineno - 1]):
+                    continue
+            except IndexError:
+                pass
+
+            yield self.adapt_error(e)
+
+    def load_file(self):
+        """
+        Loads the file in a way that auto-detects source encoding and deals
+        with broken terminal encodings for stdin.
+        Stolen from flake8_import_order because it's good.
+        """
+
+        if self.filename in ("stdin", "-", None):
+            self.filename = "stdin"
+            self.lines = pycodestyle.stdin_get_value().splitlines(True)
+        else:
+            self.lines = pycodestyle.readlines(self.filename)
+
+        if not self.tree:
+            self.tree = ast.parse("".join(self.lines))
+
+    @classmethod
+    def adapt_error(cls, e):
+        """Adapts the extended error namedtuple to be compatible with Flake8."""
+        return e._replace(message=e.message.format(*e.vars))[:4]
+
+
+error = namedtuple("error", "lineno col message type vars")
+Error = partial(partial, error, message="", type=SentryCheck, vars=())
+
+S001 = Error(
+    message="S001: Avoid using the {} mock call as it is "
+    "confusing and prone to causing invalid test "
+    "behavior."
+)
+S001.methods = {
+    "assert_calls",
+    "assert_not_called",
+    "assert_called",
+    "assert_called_once",
+    "not_called",
+    "called_once",
+    "called_once_with",
+}
+
+S002 = Error(message="S002: print functions or statements are not allowed.")
+
+S003 = Error(message="S003: Use ``from sentry.utils import json`` instead.")
+S003.modules = {"json", "simplejson"}
+S003.names = {
+    "load",
+    "loads",
+    "dump",
+    "dumps",
+    "JSONEncoder",
+    "JSONDecodeError",
+    "_default_encoder",
+}
+
+S004 = Error(
+    message="S004: ``cgi.escape`` and ``html.escape`` should not be used. Use "
+    "sentry.utils.html.escape instead."
+)
+S004.methods = {"escape"}
+S004.invalid_paths = {"cgi", "html"}
+
+S005 = Error(
+    message=f"S005: The following __future__ are not allowed: {', '.join(DISALLOWED_FUTURES)}"
+)