[IMP] core: deprecate returning domains from onchanges

* add a runtime warning when an onchange method contains the string
  'domain'
* add a lint test to forbid the string 'domain' in onchange methods,
  this is easy to defeat (e.g. `dict(domain=...)`) and can have false
  positives (literal `'domain'` strings for reasons other than an
  onchange result) but overall it seems to work nicely
* implemented as a non-pylint test as pylint takes ages to run. While
  at it, remove the long-disabled and long-useless
  PEP3110TokenChecker (checks for `except Exception, a` which is a
  syntax error in P3)

Task 2115472

closes odoo/odoo#41918

Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>

odoo/addons/test_lint/tests/__init__.py

@@ -1,3 +1,4 @@
 from . import test_pylint
 from . import test_ecmascript
 from . import test_markers
+from . import test_onchange_domains

odoo/addons/test_lint/tests/_odoo_checkers.py

@@ -1,44 +0,0 @@
-#!/usr/bin/env python
-# -*- coding: utf-8 -*-
-
-import tokenize
-
-from pylint import checkers, interfaces
-
-class PEP3110TokenChecker(checkers.BaseTokenChecker):
-    __implements__ = interfaces.ITokenChecker
-    name = 'python3'
-    enabled = False
-
-    msgs = {
-        'E3110': ('Python 3 uses `as` instead of comma token to catch exceptions',
-                  'no-comma-exception',
-                  'See http://www.python.org/dev/peps/pep-3110/',
-                  {'maxversion': (3, 0)}),
-    }
-
-    def process_tokens(self, tokens):
-        comma_found = in_except = False
-        pcount = 0
-        for tok_type, token, start, _, _ in tokens:
-            if tok_type == tokenize.NAME and token == 'except':
-                in_except = True
-                comma_found = False
-                pcount = 0
-
-            elif tok_type == tokenize.OP and in_except:
-                if token == '(':
-                    pcount += 1
-                elif token == ')':
-                    pcount -= 1
-                elif token == ',' and pcount == 0:
-                    comma_found = True
-                if token == ':':
-                    if in_except and comma_found:
-                        self.add_message('no-comma-exception', line=start[0])
-                    comma_found = in_except = False
-                    pcount = 0
-
-
-def register(linter):
-    linter.register_checker(PEP3110TokenChecker(linter))

odoo/addons/test_lint/tests/lint_case.py

@@ -0,0 +1,22 @@
+import fnmatch
+import os
+j = os.path.join
+
+from odoo.modules import get_modules, get_module_path
+from odoo.tests import BaseCase
+
+
+class LintCase(BaseCase):
+    """ Utility method for lint-type cases
+    """
+
+    def iter_module_files(self, *globs):
+        """ Yields the paths of all the module files matching the provided globs
+        (AND-ed)
+        """
+        for modroot in map(get_module_path, get_modules()):
+            for root, _, fnames in os.walk(modroot):
+                fnames = [j(root, n) for n in fnames]
+                for glob in globs:
+                    fnames = fnmatch.filter(fnames, glob)
+                yield from fnames

odoo/addons/test_lint/tests/test_ecmascript.py

@@ -6,10 +6,11 @@ import os
 import re
 import subprocess
 from unittest import skipIf
-from odoo.tests.common import TransactionCase
 from odoo import tools
 from odoo.modules import get_modules, get_module_path
 
+from . import lint_case
+
 MAX_ES_VERSION = 'es8'
 
 _logger = logging.getLogger(__name__)
@@ -21,7 +22,7 @@ except IOError:
 
 
 @skipIf(es_check is None, "es-check tool not found on this system")
-class TestECMAScriptVersion(TransactionCase):
+class TestECMAScriptVersion(lint_case.LintCase):
 
     longMessage = True
 
@@ -30,16 +31,12 @@ class TestECMAScriptVersion(TransactionCase):
 
         black_re = re.compile(r'summernote.+(intro\.js|outro.js)$')
 
-        mod_paths = [get_module_path(m) for m in get_modules()]
-        files_to_check = []
-        for p in mod_paths:
-            for dp, _, file_names in os.walk(p):
-                if 'static/test' in dp or "static/src/tests" in dp:
-                    continue
-                for fn in file_names:
-                    fullpath_name = os.path.join(dp, fn)
-                    if fullpath_name.endswith('.js') and not black_re.search(fullpath_name):
-                        files_to_check.append(fullpath_name)
+        files_to_check = [
+            p for p in self.iter_module_files('*.js')
+            if not 'static/test' in p
+            if not 'static/src/tests' in p
+            if not black_re.search(p)
+        ]
 
         _logger.info('Testing %s js files', len(files_to_check))
         cmd = [es_check, MAX_ES_VERSION] + files_to_check

odoo/addons/test_lint/tests/test_markers.py

@@ -4,14 +4,15 @@
 import logging
 import os
 import odoo
-from odoo.tests.common import TransactionCase
+
+from . import lint_case
 
 _logger = logging.getLogger(__name__)
 MARKERS = [b'<' * 7, b'>' * 7]
 EXTENSIONS = ('.py', '.js', '.xml', '.less', '.sass')
 
 
-class TestConflictMarkers(TransactionCase):
+class TestConflictMarkers(lint_case.LintCase):
 
     def check_file(self, fullpath_name):
 

odoo/addons/test_lint/tests/test_onchange_domains.py

@@ -0,0 +1,60 @@
+import ast
+import itertools
+import os
+
+from . import lint_case
+
+class OnchangeChecker(ast.NodeVisitor):
+    def visit(self, node):
+        method = 'visit_' + node.__class__.__name__
+        visitor = getattr(self, method, self.generic_visit)
+        return visitor(node)
+
+    def generic_visit(self, node):
+        for field, value in ast.iter_fields(node):
+            if isinstance(value, list):
+                for item in value:
+                    if isinstance(item, ast.AST):
+                        yield from self.visit(item)
+            elif isinstance(value, ast.AST):
+                yield from self.visit(value)
+
+    def matches_onchange(self, node):
+        if isinstance(node, ast.Call):
+            if isinstance(node.func, ast.Attribute):
+                return node.func.attr == 'onchange'
+            if isinstance(node.func, ast.Name):
+                return node.func.id == 'onchange'
+        return False
+
+    def visit_FunctionDef(self, node):
+        walker = ast.walk(node) if any(map(self.matches_onchange, node.decorator_list)) else []
+        # can stop at the first match: an @onchange function either mentions
+        # domains or does not
+        return itertools.islice((
+            n for n in walker
+            if isinstance(n, getattr(ast, 'Str', type(None))) and n.s == 'domain'
+            or isinstance(n, getattr(ast, 'Constant', type(None))) and n.value == 'domain'
+        ), 1)
+
+class TestOnchangeDomains(lint_case.LintCase):
+    """ Would ideally have been a pylint module but that's slow as molasses
+    (takes minutes to run, and can blow up entirely depending on the pylint
+    version)
+    """
+    def test_forbid_domains_in_onchanges(self):
+        """ Dynamic domains (returning a domain from an onchange) are deprecated
+        and should not be used in "standard" Odoo anymore
+        """
+        checker = OnchangeChecker()
+        rs = []
+        for path in self.iter_module_files('*.py'):
+            with open(path, 'rb') as f:
+                t = ast.parse(f.read(), path)
+            rs.extend(zip(itertools.repeat(os.path.relpath(path)), checker.visit(t)))
+
+        rs.sort(key=lambda t: t[0])
+        assert not rs, "probable domains in onchanges at\n" + '\n'.join(
+            "- %s:%d" % (path, node.lineno)
+            for path, node in rs
+        )

odoo/addons/test_lint/tests/test_pylint.py

@@ -31,6 +31,7 @@ class TestPyLint(TransactionCase):
 
         'mixed-indentation',
         'deprecated-method',
+        # custom checkers
         'sql-injection',
     ]
 
@@ -69,7 +70,7 @@ class TestPyLint(TransactionCase):
             '--enable=%s' % ','.join(self.ENABLED_CODES),
             '--reports=n',
             "--msg-template='{msg} ({msg_id}) at {path}:{line}'",
-            '--load-plugins=pylint.extensions.bad_builtin,_odoo_checkers,_odoo_checker_sql_injection',
+            '--load-plugins=pylint.extensions.bad_builtin,_odoo_checker_sql_injection',
             '--bad-functions=%s' % ','.join(self.BAD_FUNCTIONS),
             '--deprecated-modules=%s' % ','.join(self.BAD_MODULES)
         ]

odoo/models.py

@@ -5894,6 +5894,10 @@ Record ids: %(records)s
                 res['value'].pop('id', None)
                 self.update({key: val for key, val in res['value'].items() if key in self._fields})
             if res.get('domain'):
+                _logger.warning(
+                    "onchange method %s returned a domain, this is deprecated",
+                    method.__qualname__
+                )
                 result.setdefault('domain', {}).update(res['domain'])
             if res.get('warning'):
                 result['warnings'].add((