Repository: scrapy/scrapy

Issue:
On better handling use cases of PEP-380 inside request callbacks
Recently I observed people writing something like this (when using Python 3.3+):

```
# coding: utf8

import scrapy


class TestSpider(scrapy.Spider):
    name = 'test'
    start_urls = ['http://httpbin.org/get']

    def parse(self, response):
        yield scrapy.Request('http://httpbin.org/get?foo=bar')
        return {'url': response.url}
```

That won't work as expected as the `return <some value>` statement here, as defined in [PEP-380](https://www.python.org/dev/peps/pep-0380/), is just equivalent to `raise StopIteration(<some value>)`.
Sample job logs: [link](https://gist.github.com/starrify/98ab313c64ce2e2926e94d8d178b9f70)

Currently (as of dc65e75) Scrapy simply ignores the "returned" value, which might be causing confusions to users. Sample job log of the code above: [link](https://gist.github.com/starrify/5dabc9433c3d68552d64b841c885e0b2)

A proposal is to generate an error (or at least warning) when such cases happen (`StopIteration` observed to have an value).

Discussion:
elacuesta: Right, before Python 3.3 `'return' with argument inside generator` was a `SyntaxError`, I need to skip the test in py2. Good news is the py3 tests are green \o/
codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=h1) Report
> Merging [#3869](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/c4bcfb1aee9250ebb1c9e1cd390b09b3303f99f8?src=pr&el=desc) will **decrease** coverage by `2.81%`.
> The diff coverage is `90%`.


```diff
@@            Coverage Diff             @@
##           master    #3869      +/-   ##
==========================================
- Coverage   85.56%   82.74%   -2.82%     
==========================================
  Files         164      164              
  Lines        9551     9575      +24     
  Branches     1431     1436       +5     
==========================================
- Hits         8172     7923     -249     
- Misses       1132     1393     +261     
- Partials      247      259      +12
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/core/scraper.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvc2NyYXBlci5weQ==) | `86.75% <87.5%> (-1.76%)` | :arrow_down: |
| [scrapy/utils/misc.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL21pc2MucHk=) | `95.09% <90.9%> (-1.2%)` | :arrow_down: |
| [scrapy/linkextractors/sgml.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL3NnbWwucHk=) | `0% <0%> (-96.81%)` | :arrow_down: |
| [scrapy/linkextractors/regex.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL3JlZ2V4LnB5) | `0% <0%> (-95.66%)` | :arrow_down: |
| [scrapy/linkextractors/htmlparser.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL2h0bWxwYXJzZXIucHk=) | `0% <0%> (-92.07%)` | :arrow_down: |
| [scrapy/core/downloader/handlers/s3.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9zMy5weQ==) | `62.9% <0%> (-32.26%)` | :arrow_down: |
| [scrapy/extensions/statsmailer.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2V4dGVuc2lvbnMvc3RhdHNtYWlsZXIucHk=) | `0% <0%> (-30.44%)` | :arrow_down: |
| [scrapy/utils/boto.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2JvdG8ucHk=) | `46.66% <0%> (-26.67%)` | :arrow_down: |
| [scrapy/\_monkeypatches.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L19tb25rZXlwYXRjaGVzLnB5) | `42.85% <0%> (-14.29%)` | :arrow_down: |
| [scrapy/link.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmsucHk=) | `86.36% <0%> (-13.64%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree-more) | |

codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=h1) Report
> Merging [#3869](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=desc) into [master](https://codecov.io/gh/scrapy/scrapy/commit/c4bcfb1aee9250ebb1c9e1cd390b09b3303f99f8?src=pr&el=desc) will **decrease** coverage by `2.81%`.
> The diff coverage is `90%`.


```diff
@@            Coverage Diff             @@
##           master    #3869      +/-   ##
==========================================
- Coverage   85.56%   82.74%   -2.82%     
==========================================
  Files         164      164              
  Lines        9551     9575      +24     
  Branches     1431     1436       +5     
==========================================
- Hits         8172     7923     -249     
- Misses       1132     1393     +261     
- Partials      247      259      +12
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3869?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/core/scraper.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvc2NyYXBlci5weQ==) | `86.75% <87.5%> (-1.76%)` | :arrow_down: |
| [scrapy/utils/misc.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL21pc2MucHk=) | `95.09% <90.9%> (-1.2%)` | :arrow_down: |
| [scrapy/linkextractors/sgml.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL3NnbWwucHk=) | `0% <0%> (-96.81%)` | :arrow_down: |
| [scrapy/linkextractors/regex.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL3JlZ2V4LnB5) | `0% <0%> (-95.66%)` | :arrow_down: |
| [scrapy/linkextractors/htmlparser.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmtleHRyYWN0b3JzL2h0bWxwYXJzZXIucHk=) | `0% <0%> (-92.07%)` | :arrow_down: |
| [scrapy/core/downloader/handlers/s3.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9zMy5weQ==) | `62.9% <0%> (-32.26%)` | :arrow_down: |
| [scrapy/extensions/statsmailer.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2V4dGVuc2lvbnMvc3RhdHNtYWlsZXIucHk=) | `0% <0%> (-30.44%)` | :arrow_down: |
| [scrapy/utils/boto.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2JvdG8ucHk=) | `46.66% <0%> (-26.67%)` | :arrow_down: |
| [scrapy/\_monkeypatches.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L19tb25rZXlwYXRjaGVzLnB5) | `42.85% <0%> (-14.29%)` | :arrow_down: |
| [scrapy/link.py](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmsucHk=) | `86.36% <0%> (-13.64%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/scrapy/scrapy/pull/3869/diff?src=pr&el=tree-more) | |

elacuesta: Travis error is unrelated
elacuesta: Closing and reopening to force the Travis build to run again.
elacuesta: I should also write a note about this on the docs.
elacuesta: @kmike:

`scrapy-bench` @ f03011d, on my admittedly old i3-4160

In some tests I modified [this line](https://github.com/scrapy/scrapy-bench/blob/f03011d63f4149eeaad6dd7908ffa6fcbbb3304e/books/books/spiders/followall.py#L55) from `return r` to `yield from r`, to trigger the code inspection.

Command: `scrapy-bench --n-runs=20 --book_url http://domain1:8880/index.html bookworm`

* Scrapy `master` branch @ ae4eab98
    `py36`, `return r`
    Mean : 59.91042690444443 Median : 61.46755669534238 Std Dev : 4.994336655654268

    `pypy`, `return r`
    Mean : 45.292893988704805 Median : 45.20211211580239 Std Dev : 1.0998095175238132

    `py36`, `yield from r`
    Mean : 61.68034161779178 Median : 61.85817134315842 Std Dev : 2.102064300135808
    
    `pypy`, `yield from r`
    Mean : 43.366948759982265 Median : 43.56445793776966 Std Dev : 2.036978866845991


* Scrapy `detect_return_in_generator_callbacks` branch @ 8ed9e806
    `py36`, `return r`
    Mean : 54.98886208722822 Median : 57.48276553924386 Std Dev : 7.392744405205249
    
    `pypy`, `return r`
    Mean : 43.1211460939031 Median : 43.05007627473238 Std Dev : 0.7440402187424218
    
    `py36`, `yield from r`
    Mean : 55.381843120702726 Median : 60.00684786392303 Std Dev : 8.969017328279934
    
    `pypy`, `yield from r`
    Mean : 42.526517136254654 Median : 43.12169432034433 Std Dev : 2.2184298213790634

There doesn't seem to be a noticeable impact on performance (in fact, there is a few seconds improvement, but that's probably just by chance :stuck_out_tongue:). If I'm not mistaken there is only one callback in this spider, and the function call is cached now, so the results might be partial. 
elacuesta: Travis failure is due to the fact that I removed the py2 compatibility, I can revert that commit at any time if there is in fact going to be a 1.8 release.
Gallaecio: Taking https://stackoverflow.com/questions/11815873/memoization-library-for-python-2-7 into account, and since adding a new Python 2-specific dependency for this at this point may not be wise, it may still make sense to make your changes only work on Python 3, using a `six.P2` check as you used to.

Patch:
diff --git a/scrapy/core/scraper.py b/scrapy/core/scraper.py
index b3d585cce..99114d3bb 100644
--- a/scrapy/core/scraper.py
+++ b/scrapy/core/scraper.py
@@ -9,7 +9,7 @@ from twisted.internet import defer
 
 from scrapy.utils.defer import defer_result, defer_succeed, parallel, iter_errback
 from scrapy.utils.spider import iterate_spider_output
-from scrapy.utils.misc import load_object
+from scrapy.utils.misc import load_object, warn_on_generator_with_return_value
 from scrapy.utils.log import logformatter_adapter, failure_to_exc_info
 from scrapy.exceptions import CloseSpider, DropItem, IgnoreRequest
 from scrapy import signals
@@ -18,6 +18,7 @@ from scrapy.item import BaseItem
 from scrapy.core.spidermw import SpiderMiddlewareManager
 from scrapy.utils.request import referer_str
 
+
 logger = logging.getLogger(__name__)
 
 
@@ -99,11 +100,13 @@ class Scraper(object):
     def enqueue_scrape(self, response, request, spider):
         slot = self.slot
         dfd = slot.add_response_request(response, request)
+
         def finish_scraping(_):
             slot.finish_response(response, request)
             self._check_if_closing(spider, slot)
             self._scrape_next(spider, slot)
             return _
+
         dfd.addBoth(finish_scraping)
         dfd.addErrback(
             lambda f: logger.error('Scraper bug processing %(request)s',
@@ -123,7 +126,7 @@ class Scraper(object):
         callback/errback"""
         assert isinstance(response, (Response, Failure))
 
-        dfd = self._scrape2(response, request, spider)  # returns spiders processed output
+        dfd = self._scrape2(response, request, spider)  # returns spider's processed output
         dfd.addErrback(self.handle_spider_error, request, response, spider)
         dfd.addCallback(self.handle_spider_output, request, response, spider)
         return dfd
@@ -142,7 +145,10 @@ class Scraper(object):
     def call_spider(self, result, request, spider):
         result.request = request
         dfd = defer_result(result)
-        dfd.addCallbacks(callback=request.callback or spider.parse,
+        callback = request.callback or spider.parse
+        warn_on_generator_with_return_value(spider, callback)
+        warn_on_generator_with_return_value(spider, request.errback)
+        dfd.addCallbacks(callback=callback,
                          errback=request.errback,
                          callbackKeywords=request.cb_kwargs)
         return dfd.addCallback(iterate_spider_output)
@@ -172,8 +178,8 @@ class Scraper(object):
         if not result:
             return defer_succeed(None)
         it = iter_errback(result, self.handle_spider_error, request, response, spider)
-        dfd = parallel(it, self.concurrent_items,
-            self._process_spidermw_output, request, response, spider)
+        dfd = parallel(it, self.concurrent_items, self._process_spidermw_output,
+                       request, response, spider)
         return dfd
 
     def _process_spidermw_output(self, output, request, response, spider):
@@ -200,8 +206,7 @@ class Scraper(object):
         """Log and silence errors that come from the engine (typically download
         errors that got propagated thru here)
         """
-        if (isinstance(download_failure, Failure) and
-                not download_failure.check(IgnoreRequest)):
+        if isinstance(download_failure, Failure) and not download_failure.check(IgnoreRequest):
             if download_failure.frames:
                 logger.error('Error downloading %(request)s',
                              {'request': request},
diff --git a/scrapy/utils/datatypes.py b/scrapy/utils/datatypes.py
index ffd1537c3..a52bbc70e 100644
--- a/scrapy/utils/datatypes.py
+++ b/scrapy/utils/datatypes.py
@@ -8,6 +8,7 @@ This module must not depend on any module outside the Standard Library.
 import collections
 import copy
 import warnings
+import weakref
 from collections.abc import Mapping
 
 from scrapy.exceptions import ScrapyDeprecationWarning
@@ -240,7 +241,6 @@ class LocalCache(collections.OrderedDict):
     """Dictionary with a finite number of keys.
 
     Older items expires first.
-
     """
 
     def __init__(self, limit=None):
@@ -254,6 +254,35 @@ class LocalCache(collections.OrderedDict):
         super(LocalCache, self).__setitem__(key, value)
 
 
+class LocalWeakReferencedCache(weakref.WeakKeyDictionary):
+    """
+    A weakref.WeakKeyDictionary implementation that uses LocalCache as its
+    underlying data structure, making it ordered and capable of being size-limited.
+
+    Useful for memoization, while avoiding keeping received
+    arguments in memory only because of the cached references.
+
+    Note: like LocalCache and unlike weakref.WeakKeyDictionary,
+    it cannot be instantiated with an initial dictionary.
+    """
+
+    def __init__(self, limit=None):
+        super(LocalWeakReferencedCache, self).__init__()
+        self.data = LocalCache(limit=limit)
+
+    def __setitem__(self, key, value):
+        try:
+            super(LocalWeakReferencedCache, self).__setitem__(key, value)
+        except TypeError:
+            pass  # key is not weak-referenceable, skip caching
+
+    def __getitem__(self, key):
+        try:
+            return super(LocalWeakReferencedCache, self).__getitem__(key)
+        except TypeError:
+            return None  # key is not weak-referenceable, it's not cached
+
+
 class SequenceExclude(object):
     """Object to test if an item is NOT within some sequence."""
 
diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py
index 9955fb1e7..cb0ee5af3 100644
--- a/scrapy/utils/misc.py
+++ b/scrapy/utils/misc.py
@@ -1,13 +1,18 @@
 """Helper functions which don't fit anywhere else"""
+import ast
+import inspect
 import os
 import re
 import hashlib
+import warnings
 from contextlib import contextmanager
 from importlib import import_module
 from pkgutil import iter_modules
+from textwrap import dedent
 
 from w3lib.html import replace_entities
 
+from scrapy.utils.datatypes import LocalWeakReferencedCache
 from scrapy.utils.python import flatten, to_unicode
 from scrapy.item import BaseItem
 
@@ -161,3 +166,44 @@ def set_environ(**kwargs):
                 del os.environ[k]
             else:
                 os.environ[k] = v
+
+
+_generator_callbacks_cache = LocalWeakReferencedCache(limit=128)
+
+
+def is_generator_with_return_value(callable):
+    """
+    Returns True if a callable is a generator function which includes a
+    'return' statement with a value different than None, False otherwise
+    """
+    if callable in _generator_callbacks_cache:
+        return _generator_callbacks_cache[callable]
+
+    def returns_none(return_node):
+        value = return_node.value
+        return value is None or isinstance(value, ast.NameConstant) and value.value is None
+
+    if inspect.isgeneratorfunction(callable):
+        tree = ast.parse(dedent(inspect.getsource(callable)))
+        for node in ast.walk(tree):
+            if isinstance(node, ast.Return) and not returns_none(node):
+                _generator_callbacks_cache[callable] = True
+                return _generator_callbacks_cache[callable]
+
+    _generator_callbacks_cache[callable] = False
+    return _generator_callbacks_cache[callable]
+
+
+def warn_on_generator_with_return_value(spider, callable):
+    """
+    Logs a warning if a callable is a generator function and includes
+    a 'return' statement with a value different than None
+    """
+    if is_generator_with_return_value(callable):
+        warnings.warn(
+            'The "{}.{}" method is a generator and includes a "return" statement with a '
+            'value different than None. This could lead to unexpected behaviour. Please see '
+            'https://docs.python.org/3/reference/simple_stmts.html#the-return-statement '
+            'for details about the semantics of the "return" statement within generators'
+            .format(spider.__class__.__name__, callable.__name__), stacklevel=2,
+        )


Test patch:
diff --git a/tests/test_utils_datatypes.py b/tests/test_utils_datatypes.py
index 38a25778e..e5aa56eb9 100644
--- a/tests/test_utils_datatypes.py
+++ b/tests/test_utils_datatypes.py
@@ -2,7 +2,9 @@ import copy
 import unittest
 from collections.abc import Mapping, MutableMapping
 
-from scrapy.utils.datatypes import CaselessDict, LocalCache, SequenceExclude
+from scrapy.http import Request
+from scrapy.utils.datatypes import CaselessDict, LocalCache, LocalWeakReferencedCache, SequenceExclude
+from scrapy.utils.python import garbage_collect
 
 
 __doctests__ = ['scrapy.utils.datatypes']
@@ -255,5 +257,67 @@ class LocalCacheTest(unittest.TestCase):
             self.assertEqual(cache[str(x)], x)
 
 
+class LocalWeakReferencedCacheTest(unittest.TestCase):
+
+    def test_cache_with_limit(self):
+        cache = LocalWeakReferencedCache(limit=2)
+        r1 = Request('https://example.org')
+        r2 = Request('https://example.com')
+        r3 = Request('https://example.net')
+        cache[r1] = 1
+        cache[r2] = 2
+        cache[r3] = 3
+        self.assertEqual(len(cache), 2)
+        self.assertNotIn(r1, cache)
+        self.assertIn(r2, cache)
+        self.assertIn(r3, cache)
+        self.assertEqual(cache[r2], 2)
+        self.assertEqual(cache[r3], 3)
+        del r2
+
+        # PyPy takes longer to collect dead references
+        garbage_collect()
+
+        self.assertEqual(len(cache), 1)
+
+    def test_cache_non_weak_referenceable_objects(self):
+        cache = LocalWeakReferencedCache()
+        k1 = None
+        k2 = 1
+        k3 = [1, 2, 3]
+        cache[k1] = 1
+        cache[k2] = 2
+        cache[k3] = 3
+        self.assertNotIn(k1, cache)
+        self.assertNotIn(k2, cache)
+        self.assertNotIn(k3, cache)
+        self.assertEqual(len(cache), 0)
+
+    def test_cache_without_limit(self):
+        max = 10**4
+        cache = LocalWeakReferencedCache()
+        refs = []
+        for x in range(max):
+            refs.append(Request('https://example.org/{}'.format(x)))
+            cache[refs[-1]] = x
+        self.assertEqual(len(cache), max)
+        for i, r in enumerate(refs):
+            self.assertIn(r, cache)
+            self.assertEqual(cache[r], i)
+        del r  # delete reference to the last object in the list
+
+        # delete half of the objects, make sure that is reflected in the cache
+        for _ in range(max // 2):
+            refs.pop()
+
+        # PyPy takes longer to collect dead references
+        garbage_collect()
+
+        self.assertEqual(len(cache), max // 2)
+        for i, r in enumerate(refs):
+            self.assertIn(r, cache)
+            self.assertEqual(cache[r], i)
+
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/test_utils_misc/test_return_with_argument_inside_generator.py b/tests/test_utils_misc/test_return_with_argument_inside_generator.py
new file mode 100644
index 000000000..bdbec1beb
--- /dev/null
+++ b/tests/test_utils_misc/test_return_with_argument_inside_generator.py
@@ -0,0 +1,37 @@
+import unittest
+
+from scrapy.utils.misc import is_generator_with_return_value
+
+
+class UtilsMiscPy3TestCase(unittest.TestCase):
+
+    def test_generators_with_return_statements(self):
+        def f():
+            yield 1
+            return 2
+
+        def g():
+            yield 1
+            return 'asdf'
+
+        def h():
+            yield 1
+            return None
+
+        def i():
+            yield 1
+            return
+
+        def j():
+            yield 1
+
+        def k():
+            yield 1
+            yield from g()
+
+        assert is_generator_with_return_value(f)
+        assert is_generator_with_return_value(g)
+        assert not is_generator_with_return_value(h)
+        assert not is_generator_with_return_value(i)
+        assert not is_generator_with_return_value(j)
+        assert not is_generator_with_return_value(k)  # not recursive