ZODB Components: Enable checking of imports with pylint.

Until now it was disabled which means that if Component A imports Component B
and the latter is not in {validated,modified} state, Component A could be
validated without raising any error and failed at execution time.

As this relies on Pylint transform/hook and avoiding monkey patch as much
as possible, make Products.ERP5Type.patches.pylint available for Python3 (not
actually tested with ERP5 but no AttributeError nor ImportError with Python3
with this code).

Also, allow developer to call validate action from 'modified' state to handle
import use case:
  1. Edit A which raises an error stating that B.
  2. Fix B and validate it.
  3. Validate again A without requiring a modification of {reference,version,text_content}.

product/ERP5/bootstrap/erp5_core/WorkflowTemplateItem/portal_workflow/component_validation_workflow/scripts/checkConsistency.py

-state_change['object'].Base_checkConsistency()
+obj = state_change['object']
+
+# User explicitely wants to validate again (example use case: import-error
+# on ZODB Components which have now been fixed)
+if obj.getValidationState() == 'modified':
+  obj.checkSourceCode()
+
+obj.Base_checkConsistency()

product/ERP5/bootstrap/erp5_core/WorkflowTemplateItem/portal_workflow/component_validation_workflow/states/modified.xml

@@ -26,6 +26,7 @@
                 <string>invalidate_action</string>
                 <string>modify</string>
                 <string>validate</string>
+                <string>validate_action</string>
               </tuple>
             </value>
         </item>

product/ERP5/bootstrap/erp5_core/WorkflowTemplateItem/portal_workflow/dynamic_class_generation_interaction_workflow/scripts/Component_checkSourceCodeAndValidateAfterModified.py

 obj = state_change['object']
-
-error_list = []
-warning_list = []
-for message_dict in obj.checkSourceCode():
-  message = '%s:%3d,%3d: %s' % (message_dict['type'],
-                                  message_dict['row'],
-                                  message_dict['column'],
-                                  message_dict['text'])
-
-  if message_dict['type'] in ('F', 'E'):
-    error_list.append(message)
-  else:
-    warning_list.append(message)
-
-obj.setTextContentWarningMessageList(warning_list)
-obj.setTextContentErrorMessageList(error_list)
-
-if not error_list and obj.getValidationState() == 'modified':
+obj.checkSourceCode()
+if not obj.getTextContentErrorMessageList() and obj.getValidationState() == 'modified':
   obj.checkConsistencyAndValidate()

product/ERP5Type/Utils.py

@@ -498,8 +498,6 @@ def checkPythonSourceCode(source_code_str, portal_type=None):
            # TODO-arnau: Enable it properly would require inspection API
            # '%s %r has no %r member'
            '--disable=E1101,E1103',
-           # 'No name %r in module %r'
-           '--disable=E0611',
            # map and filter should not be considered bad as in some cases
            # map is faster than its recommended replacement (list
            # comprehension)

product/ERP5Type/dynamic/component_package.py

@@ -414,6 +414,14 @@ class ComponentDynamicPackage(ModuleType):
 
       delattr(package, name)
 
+      # Clear pylint cache
+      try:
+        from astroid.builder import MANAGER
+      except ImportError:
+        pass
+      else:
+        MANAGER.astroid_cache.pop(module_name, None)
+
 class ToolComponentDynamicPackage(ComponentDynamicPackage):
   def reset(self, *args, **kw):
     """

product/ERP5Type/mixin/component.py

@@ -309,7 +309,22 @@ class ComponentMixin(PropertyRecordableMixin, Base):
     Check Component source code through Pylint or compile() builtin if not
     available
     """
-    return checkPythonSourceCode(self.getTextContent(), self.getPortalType())
+    error_list = []
+    warning_list = []
+    for message_dict in checkPythonSourceCode(self.getTextContent(),
+                                              self.getPortalType()):
+      message = '%s:%3d,%3d: %s' % (message_dict['type'],
+                                    message_dict['row'],
+                                    message_dict['column'],
+                                    message_dict['text'])
+
+      if message_dict['type'] in ('F', 'E'):
+        error_list.append(message)
+      else:
+        warning_list.append(message)
+
+    self.setTextContentWarningMessageList(warning_list)
+    self.setTextContentErrorMessageList(error_list)
 
   security.declareProtected(Permissions.ModifyPortalContent, 'PUT')
   def PUT(self, REQUEST, RESPONSE):

product/ERP5Type/patches/pylint.py

@@ -19,74 +19,195 @@
 # 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
 
 from __future__ import absolute_import
-from inspect import getargspec
 import sys
 
+# TODO: make sure that trying to use it does not import isort, because the
+#       latter hacks Python in order to execute:
+#           sys.setdefaultencoding('utf-8')
+#       This changes the behaviour of some ERP5 code.
+sys.modules.setdefault('isort', None)
+
+## All arguments are passed as arguments and this needlessly outputs a 'No
+## config file found, using default configuration' message on stderr.
+try:
+    from logilab.common.configuration import OptionsManagerMixIn
+except ImportError:
+    # pylint 2.x (python3)
+    from pylint.config import OptionsManagerMixIn
+OptionsManagerMixIn.read_config_file = lambda *args, **kw: None
+
+## Pylint transforms and plugin to generate AST for ZODB Components
+from astroid.builder import AstroidBuilder
+from astroid.exceptions import AstroidBuildingException
+from astroid import MANAGER
+
 try:
-    # TODO: Add support for newer versions pylint. Meanwhile, make sure that
-    #       trying to use it does not import isort, because the latter hacks
-    #       Python in order to execute:
-    #           sys.setdefaultencoding('utf-8')
-    #       This changes the behaviour of some ERP5 code.
-    sys.modules.setdefault('isort', None)
-
-    from pylint.checkers.imports import ImportsChecker
-    import astroid
-    ImportsChecker.get_imported_module
-except (AttributeError, ImportError):
-    pass
-else:
-    def _get_imported_module(self, importnode, modname):
+  from astroid.builder import _guess_encoding
+except ImportError:
+  # XXX: With python3, tokenize.detect_encoding() is used instead. This
+  # should do the same instead of copying/pasting legacy code...
+  import re
+  _ENCODING_RGX = re.compile(r"\s*#+.*coding[:=]\s*([-\w.]+)")
+  def _guess_encoding(string):
+      """get encoding from a python file as string or return None if not found"""
+      # check for UTF-8 byte-order mark
+      if string.startswith('\xef\xbb\xbf'):
+          return 'UTF-8'
+      for line in string.split('\n', 2)[:2]:
+          # check for encoding declaration
+          match = _ENCODING_RGX.match(line)
+          if match is not None:
+              return match.group(1)
+def string_build(self, data, modname='', path=None):
+    """
+    build astroid from source code string and return rebuilded astroid
+
+    Monkey patched to check encoding properly, as `file_build()` does:
+      # `data` can only be an str (not unicode)
+      module = self._data_build(data, modname, path)
+      # But here it tries to `encode()` which will fails if there is any
+      # non-ASCII character...
+      module.file_bytes = data.encode('utf-8')
+      return self._post_build(module, 'utf-8')
+    """
+    encoding = _guess_encoding(data)
+    if encoding is None:
+        # Encoding not defined in the source file, assuming utf-8...
+        encoding = 'utf-8'
+    try:
+        # BytesIO() does not handle unicode:
+        #   TypeError: 'unicode' does not have the buffer interface
+        if isinstance(data, unicode):
+            data = data.encode(encoding)
+        else:
+            # Just to avoid error later on...
+            data.decode(encoding)
+    except Exception as exc:
+        from zLOG import LOG, WARNING
+        LOG("Products.ERP5Type.patches.pylint", WARNING,
+            "%s: Considered as not importable: Wrong encoding? (%r)" %
+            (modname, exc))
+        raise AstroidBuildingException(exc)
+    module = self._data_build(data, modname, path)
+    module.file_bytes = data
+    return self._post_build(module, encoding)
+AstroidBuilder.string_build = string_build
+
+from astroid import nodes
+def erp5_package_transform(node):
+    """'
+    erp5/' directory on the filesystem is different from 'erp5' module when
+    running ERP5, so replace entirely this node completely to avoid pylint
+    checking erp5/ directory structure for module and returns errors...
+    """
+    # Cannot call string_build() as this would be called again and again
+    erp5_package_node = nodes.Module('erp5', None)
+    erp5_package_node.package = True
+    erp5_package_node._absolute_import_activated = True
+    return erp5_package_node
+MANAGER.register_transform(nodes.Module,
+                           erp5_package_transform,
+                           lambda n: n.name == 'erp5')
+
+def _buildAstroidModuleFromComponentModuleName(modname):
+    from Products.ERP5.ERP5Site import getSite
+    from Acquisition import aq_base
+    portal = getSite()
+    component_tool = aq_base(portal.portal_components)
+    component_obj = None
+    component_id = modname[len('erp5.component.'):]
+    if '_version' in modname:
         try:
-            return importnode.do_import_module(modname)
-        except astroid.InferenceError, ex:
-            # BEGIN
-
-            # XXX-arnau: Ignore ERP5 dynamic modules, hackish but required
-            # until proper introspection is implemented because otherwise it
-            # is impossible to validate Components importing other Components
-            # and as it is static analysis, the module should not be loaded
-            # anyway
-            if modname.startswith('erp5'):
-                return
+            obj = getattr(component_tool,
+                          component_id.replace('_version', '', 1))
+        except AttributeError:
+            raise AstroidBuildingException
+        if obj.getValidationState() in ('modified', 'validated'):
+            component_obj = obj
+        else:
+            raise AstroidBuildingException
 
-            # Handle ImportError try/except checking for missing module before
-            # falling back to code handling such case (#9386)
-            pnode = importnode.parent
-            if pnode and isinstance(pnode, astroid.TryExcept):
-                for handler in pnode.handlers:
-                    # Handling except:
-                    if not handler.type:
-                        return
-
-                    # Handling ImportError and its Exception base classes
-                    for klass in ImportError.mro():
-                        if klass is object:
-                            break
-                        elif klass.__name__ == handler.type.name:
-                            return
-            # END
-
-            if str(ex) != modname:
-                args = '%r (%s)' % (modname, ex)
-            else:
-                args = repr(modname)
-            self.add_message("F0401", args=args, node=importnode)
+    else:
+        try:
+            package, reference = component_id.split('.', 1)
+        except ValueError:
+            raise AstroidBuildingException
+        for version in portal.getVersionPriorityNameList():
+            try:
+                obj = getattr(component_tool,
+                              '%s.%s.%s' % (package, version, reference))
+            except AttributeError:
+                continue
+
+            if obj.getValidationState() in ('modified', 'validated'):
+                component_obj = obj
+                break
+
+    if component_obj is None:
+        raise AstroidBuildingException
+
+    # module_build() could also be used but this requires importing
+    # the ZODB Component and also monkey-patch it to support PEP-302
+    # for __file__ starting with '<'
+    module = AstroidBuilder(MANAGER).string_build(
+        component_obj.getTextContent(validated_only=True),
+        modname)
+    return module
 
-    if 'modnode' in getargspec(ImportsChecker.get_imported_module).args:
-        # BBB for pylint < 1.4.0
-        def get_imported_module(self, modnode, importnode, modname):
-            return _get_imported_module(self, importnode, modname)
+def fail_hook_erp5_component(modname):
+    if not modname.startswith('erp5.'):
+        raise AstroidBuildingException
+
+    if (modname in ('erp5.portal_type',
+                    'erp5.component',
+                    'erp5.component.module',
+                    'erp5.component.extension',
+                    'erp5.component.document',
+                    'erp5.component.tool',
+                    'erp5.component.interface',
+                    'erp5.component.mixin',
+                    'erp5.component.test') or
+        (modname.startswith('erp5.component.') and modname.endswith('_version'))):
+        module = AstroidBuilder(MANAGER).string_build('', modname)
+        if modname.startswith('erp5.component'):
+            module.package = True
     else:
-        get_imported_module = _get_imported_module
+        module = _buildAstroidModuleFromComponentModuleName(modname)
 
-    ImportsChecker.get_imported_module = get_imported_module
+    module._absolute_import_activated = True
+    return module
+MANAGER.register_failed_import_hook(fail_hook_erp5_component)
 
-    # All arguments are passed as arguments and this needlessly outputs a 'No
-    # config file found, using default configuration' message on stderr.
-    from logilab.common.configuration import OptionsManagerMixIn
-    OptionsManagerMixIn.read_config_file = lambda *args, **kw: None
+## Patch to handle 'no-name-in-module' for attributes added by monkey
+## patches in Products/XXX/patches.
+##
+## Instead of monkey patching, an alternative would be to use Pylint
+## transforms but this would require either checking dynamically which
+## attributes has been added (much more complex than the current approach)
+## or listing them statically (inconvenient).
+from pylint.checkers import BaseChecker
+from pylint.interfaces import UNDEFINED
+BaseChecker_add_message = BaseChecker.add_message
+def add_message(self, msg_descr, line=None, node=None, args=None,
+                confidence=UNDEFINED):
+    """
+    Monkey patched to dynamically ignore some error/warning messages
+    """
+    if msg_descr == 'no-name-in-module':
+        name, module_name = args
+        if not module_name.startswith('erp5.'):
+            # Do not call __import__ as this may load ZODB Component which
+            # should use 'version' and not use monkey patches...
+            try:
+                getattr(sys.modules[module_name], name)
+            except (KeyError, AttributeError):
+                pass
+            else:
+                # Do nothing as this does exist
+                return
+    BaseChecker_add_message(self, msg_descr, line=line, node=node,
+                            args=args, confidence=confidence)
+BaseChecker.add_message = add_message
 
-finally:
-    if sys.modules['isort'] is None:
-        del sys.modules['isort']
+if sys.modules['isort'] is None:
+    del sys.modules['isort']

product/ERP5Type/tests/testDynamicClassGeneration.py

+# -*- coding: utf-8 -*-
 ##############################################################################
 #
 # Copyright (c) 2010 Nexedi SARL and Contributors. All Rights Reserved.
@@ -1454,8 +1455,13 @@ class _TestZodbComponent(SecurityTestCase):
       text_content=text_content,
       portal_type=self._portal_type)
 
-  def _getComponentFullModuleName(self, module_name):
-    return self._document_class._getDynamicModuleNamespace() + '.' + module_name
+  def _getComponentFullModuleName(self, module_name, version=None):
+    if version is None:
+      return (self._document_class._getDynamicModuleNamespace() + '.' +
+              module_name)
+    else:
+      return (self._document_class._getDynamicModuleNamespace() + '.' +
+              version + '_version' + '.' + module_name)
 
   def failIfModuleImportable(self, module_name):
     """
@@ -2137,6 +2143,170 @@ def function_foo(*args, **kwargs):
     self.assertUserCanModifyDocument(user_id, component)
     self.assertUserCanDeleteDocument(user_id, component)
 
+  def _assertAstroidCacheContent(self,
+                                 must_be_in_cache_set,
+                                 must_not_be_in_cache_set):
+    from astroid.builder import MANAGER
+    should_not_be_in_cache_list = []
+    for modname in MANAGER.astroid_cache:
+      if modname in must_not_be_in_cache_set:
+        should_not_be_in_cache_list.append(modname)
+
+      if modname in must_be_in_cache_set:
+        must_be_in_cache_set.remove(modname)
+
+    self.assertEqual(should_not_be_in_cache_list, [])
+    self.assertEqual(must_be_in_cache_set, set())
+
+  def testPylint(self):
+    # One 'imported' Component for each use case:
+    #   from erp5.component.module import M1
+    #   from erp5.component.module.M1 import hoge
+    imported_reference1 = self._generateReference('TestPylintImported1')
+    imported_component1 = self._newComponent(imported_reference1)
+    imported_component1.setTextContent(imported_component1.getTextContent() + """
+def hoge():
+  return 'OK'
+""")
+
+    imported_reference2 = self._generateReference('TestPylintImported2')
+    imported_component2 = self._newComponent(imported_reference2)
+    imported_component2.setTextContent(imported_component2.getTextContent() + """
+def hoge():
+  return 'OK'
+""")
+
+    reference = self._generateReference('TestPylint')
+    component = self._newComponent(reference)
+    self.portal.portal_workflow.doActionFor(component, 'validate_action')
+    self.tic()
+    self.assertEqual(component.getValidationState(), 'validated')
+    self.assertEqual(component.getTextContentErrorMessageList(), [])
+    self.assertEqual(component.getTextContentWarningMessageList(), [])
+    self.assertModuleImportable(reference)
+
+    namespace = self._document_class._getDynamicModuleNamespace()
+
+    imported_module1 = self._getComponentFullModuleName(
+      imported_reference1)
+    imported_module1_with_version = self._getComponentFullModuleName(
+      imported_reference1, version='erp5')
+
+    imported_module2 = self._getComponentFullModuleName(
+      imported_reference2)
+    imported_module2_with_version = self._getComponentFullModuleName(
+      imported_reference2, version='erp5')
+
+    component.setTextContent(
+      """# -*- coding: utf-8 -*-
+# Source code with non-ASCII character should not fail: éàホゲ
+from %(namespace)s import %(reference1)s
+from %(namespace)s.erp5_version import %(reference1)s
+
+from %(module2)s import hoge
+from %(module2_with_version)s import hoge
+
+import %(module2)s
+import %(module2_with_version)s
+
+# To avoid 'unused-import' warnings...
+%(reference1)s.hoge()
+hoge()
+%(module2)s.hoge()
+%(module2_with_version)s.hoge()
+
+# Attributes added through Products.XXX.patches: Must not raise error
+from Products.DCWorkflow.DCWorkflow import ValidationFailed
+# To avoid 'unused-import' warnings...
+ValidationFailed('anything')
+
+""" % (dict(namespace=namespace,
+            reference1=imported_reference1,
+            module2=imported_module2,
+            module2_with_version=imported_module2_with_version)) +
+      component.getTextContent())
+    self.tic()
+    self._assertAstroidCacheContent(
+      must_be_in_cache_set={'%s' % namespace,
+                            '%s.erp5_version' % namespace},
+      must_not_be_in_cache_set={imported_module1,
+                                imported_module1_with_version,
+                                imported_module2,
+                                imported_module2_with_version})
+    self.assertEqual(component.getValidationState(), 'modified')
+    self.assertEqual(
+      component.getTextContentErrorMessageList(),
+      ["E:  3,  0: No name '%s' in module '%s' (no-name-in-module)" %
+       (imported_reference1, namespace),
+       "E:  4,  0: No name '%s' in module '%s.erp5_version' (no-name-in-module)" %
+       (imported_reference1, namespace),
+       # Spurious message but same as filesystem modules: 2 errors raised
+       # (no-name-in-module and import-error)
+       "E:  6,  0: No name '%s' in module '%s' (no-name-in-module)" %
+       (imported_reference2, namespace),
+       "F:  6,  0: Unable to import '%s' (import-error)" %
+       imported_module2,
+       # Spurious message (see above comment)
+       "E:  7,  0: No name '%s' in module '%s.erp5_version' (no-name-in-module)" %
+       (imported_reference2, namespace),
+       "F:  7,  0: Unable to import '%s' (import-error)" %
+       imported_module2_with_version,
+       # Spurious message (see above comment)
+       "E:  9,  0: No name '%s' in module '%s' (no-name-in-module)" %
+       (imported_reference2, namespace),
+       "F:  9,  0: Unable to import '%s' (import-error)" %
+       imported_module2,
+       # Spurious message (see above comment)
+       "E: 10,  0: No name '%s' in module '%s.erp5_version' (no-name-in-module)" %
+       (imported_reference2, namespace),
+       "F: 10,  0: Unable to import '%s' (import-error)" %
+       imported_module2_with_version])
+    self.assertEqual(component.getTextContentWarningMessageList(), [])
+
+    ## Simulate user:
+    # 1) First check and validate 'imported' Components
+    self.portal.portal_workflow.doActionFor(imported_component1, 'validate_action')
+    self.portal.portal_workflow.doActionFor(imported_component2, 'validate_action')
+    self.tic()
+    self.assertEqual(imported_component1.getValidationState(), 'validated')
+    self.assertEqual(imported_component2.getValidationState(), 'validated')
+
+    # 2) Then validate again the main one
+    self.portal.portal_workflow.doActionFor(component, 'validate_action')
+    self.tic()
+    self._assertAstroidCacheContent(
+      must_be_in_cache_set={'%s' % namespace,
+                            '%s.erp5_version' % namespace,
+                            imported_module1,
+                            imported_module1_with_version,
+                            imported_module2,
+                            imported_module2_with_version},
+      must_not_be_in_cache_set=set())
+    self.assertEqual(component.getValidationState(), 'validated')
+    self.assertEqual(component.getTextContentErrorMessageList(), [])
+    self.assertEqual(component.getTextContentWarningMessageList(), [])
+
+    component.setTextContent(
+      """# -*- coding: utf-8 -*-
+from %(module)s import undefined
+from %(module_with_version)s import undefined
+
+# To avoid 'unused-import' warning...
+undefined()
+
+""" % (dict(module=imported_module2,
+            module_with_version=imported_module2_with_version)) +
+      component.getTextContent())
+    self.tic()
+    self.assertEqual(component.getValidationState(), 'modified')
+    self.assertEqual(
+      component.getTextContentErrorMessageList(),
+      ["E:  2,  0: No name 'undefined' in module '%s' (no-name-in-module)" %
+       imported_module2,
+       "E:  3,  0: No name 'undefined' in module '%s' (no-name-in-module)" %
+       imported_module2_with_version])
+    self.assertEqual(component.getTextContentWarningMessageList(), [])
+
 from Products.ERP5Type.Core.ExtensionComponent import ExtensionComponent
 
 class TestZodbExtensionComponent(_TestZodbComponent):