From cf9be5344a89dd8e14f8241ec69de9c984ec1e05 Mon Sep 17 00:00:00 2001
From: willbeaufoy <will@willbeaufoy.net>
Date: Mon, 11 May 2020 19:35:25 +0100
Subject: [PATCH] Prevent create_instance() returning None (#4532)

Currently create_instance() can return None if an extension is
incorrectly implemented, but the extension will still show up as
enabled in the logs. This can cause confusion, as in the linked bug.

This change prevents this occurring by throwing an error if
create_instance() will return None.
---
 scrapy/utils/misc.py              | 15 ++++++++++++---
 tests/test_utils_misc/__init__.py | 14 +++++++++++---
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py
index 52cfba208..ab7cf9deb 100644
--- a/scrapy/utils/misc.py
+++ b/scrapy/utils/misc.py
@@ -137,17 +137,26 @@ def create_instance(objcls, settings, crawler, *args, **kwargs):
     ``*args`` and ``**kwargs`` are forwarded to the constructors.
 
     Raises ``ValueError`` if both ``settings`` and ``crawler`` are ``None``.
+
+    Raises ``TypeError`` if the resulting instance is ``None`` (e.g. if an
+    extension has not been implemented correctly).
     """
     if settings is None:
         if crawler is None:
             raise ValueError("Specify at least one of settings and crawler.")
         settings = crawler.settings
     if crawler and hasattr(objcls, 'from_crawler'):
-        return objcls.from_crawler(crawler, *args, **kwargs)
+        instance = objcls.from_crawler(crawler, *args, **kwargs)
+        method_name = 'from_crawler'
     elif hasattr(objcls, 'from_settings'):
-        return objcls.from_settings(settings, *args, **kwargs)
+        instance = objcls.from_settings(settings, *args, **kwargs)
+        method_name = 'from_settings'
     else:
-        return objcls(*args, **kwargs)
+        instance = objcls(*args, **kwargs)
+        method_name = '__new__'
+    if instance is None:
+        raise TypeError("%s.%s returned None" % (objcls.__qualname__, method_name))
+    return instance
 
 
 @contextmanager
diff --git a/tests/test_utils_misc/__init__.py b/tests/test_utils_misc/__init__.py
index 6f945cd01..015a0e5a2 100644
--- a/tests/test_utils_misc/__init__.py
+++ b/tests/test_utils_misc/__init__.py
@@ -114,8 +114,12 @@ class UtilsMiscTestCase(unittest.TestCase):
         #   2. with from_settings() constructor
         #   3. with from_crawler() constructor
         #   4. with from_settings() and from_crawler() constructor
-        spec_sets = ([], ['from_settings'], ['from_crawler'],
-                     ['from_settings', 'from_crawler'])
+        spec_sets = (
+            ['__qualname__'],
+            ['__qualname__', 'from_settings'],
+            ['__qualname__', 'from_crawler'],
+            ['__qualname__', 'from_settings', 'from_crawler'],
+        )
         for specs in spec_sets:
             m = mock.MagicMock(spec_set=specs)
             _test_with_settings(m, settings)
@@ -123,7 +127,7 @@ class UtilsMiscTestCase(unittest.TestCase):
             _test_with_crawler(m, settings, crawler)
 
         # Check adoption of crawler settings
-        m = mock.MagicMock(spec_set=['from_settings'])
+        m = mock.MagicMock(spec_set=['__qualname__', 'from_settings'])
         create_instance(m, None, crawler, *args, **kwargs)
         m.from_settings.assert_called_once_with(crawler.settings, *args,
                                                 **kwargs)
@@ -131,6 +135,10 @@ class UtilsMiscTestCase(unittest.TestCase):
         with self.assertRaises(ValueError):
             create_instance(m, None, None)
 
+        m.from_settings.return_value = None
+        with self.assertRaises(TypeError):
+            create_instance(m, settings, None)
+
     def test_set_environ(self):
         assert os.environ.get('some_test_environ') is None
         with set_environ(some_test_environ='test_value'):