Repository: python-attrs/attrs (python)

Issue:
init unique_filename is problematic for exception aggregation
We had a case where a custom converter was often failing due to an app issue.  The traceback will include a line with a unique ID based on the initial attr values in the class:

```
File "<attrs generated init 81db48c8801f50d1c320ad1b8d1e0028320432b2>", line 3, in __init__
```

Having a unique ID in the traceback in turn causes problems for exception fingerprinting (e.g. by Sentry).  I'm not thrilled about doing custom traceback filtering for such cases, and would like to explore other options.

I wonder what the full rationale is for ensuring the unique filename, and whether it could be made optional.

Including a hash of the attribute values is essentially like dumping the values of all attributes of a class instance as part of the traceback whenever there is a constructor error regarding a single attribute-- which is less than precise error reporting.

https://github.com/python-attrs/attrs/blob/dc1b5a01e98a9ba49d45ad07ba6ae0ee2d9c1b8e/src/attr/_make.py#L1268

Pull request:
Make unique_filename not depend on repr.
For debugging purposes, when a method is created it needs to be put
into a "filename".  This filename was generated using `repr(attrs)`
however this meant that the filename could change every time the
program was loaded depending on the whims of repr.  (In particular
free functions and lambdas would include ids)

This solves the problem by changing the name to be <attrs generated
{method} {module}.{qualname}> If it should happen that the name is
taken then -2 will be appended and incremented until a free filename
is found.

Fixes #558

I'll wait on a reply as to whether this is more or less what we were looking for before I go through the Pull Request checklist.

# Pull Request Check List

This is just a reminder about the most common mistakes.  Please make sure that you tick all *appropriate* boxes.  But please read our [contribution guide](https://www.attrs.org/en/latest/contributing.html) at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing to do.

- [x] Added **tests** for changed code.
- [ ] New features have been added to our [Hypothesis testing strategy](https://github.com/python-attrs/attrs/blob/master/tests/strategies.py).
- [ ] Changes or additions to public APIs are reflected in our type stubs (files ending in ``.pyi``).
    - [ ] ...and used in the stub test file `tests/typing_example.py`.
- [ ] Updated **documentation** for changed code.
    - [ ] New functions/classes have to be added to `docs/api.rst` by hand.
    - [ ] Changes to the signature of `@attr.s()` have to be added by hand too.
    - [ ] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/markup/para.html#directive-versionadded).
- [ ] Documentation in `.rst` files is written using [semantic newlines](https://rhodesmill.org/brandon/2012/one-sentence-per-line/).
- [x] Changes (and possible deprecations) have news fragments in [`changelog.d`](https://github.com/python-attrs/attrs/blob/master/changelog.d).

If you have *any* questions to *any* of the points above, just **submit and ask**!  This checklist is here to *help* you, not to deter you from contributing!


Patch:
diff --git a/changelog.d/560.change.rst b/changelog.d/560.change.rst
new file mode 100644
index 0000000..2183305
--- /dev/null
+++ b/changelog.d/560.change.rst
@@ -0,0 +1,2 @@
+The fake filename for generated methods is now more stable.
+It won't change when you restart the process.
diff --git a/src/attr/_make.py b/src/attr/_make.py
index 283aec6..cd3ce97 100644
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -1,10 +1,10 @@
 from __future__ import absolute_import, division, print_function
 
 import copy
-import hashlib
 import linecache
 import sys
 import threading
+import uuid
 import warnings
 
 from operator import itemgetter
@@ -665,7 +665,10 @@ class _ClassBuilder(object):
     def add_hash(self):
         self._cls_dict["__hash__"] = self._add_method_dunders(
             _make_hash(
-                self._attrs, frozen=self._frozen, cache_hash=self._cache_hash
+                self._cls,
+                self._attrs,
+                frozen=self._frozen,
+                cache_hash=self._cache_hash,
             )
         )
 
@@ -674,6 +677,7 @@ class _ClassBuilder(object):
     def add_init(self):
         self._cls_dict["__init__"] = self._add_method_dunders(
             _make_init(
+                self._cls,
                 self._attrs,
                 self._has_post_init,
                 self._frozen,
@@ -692,7 +696,8 @@ class _ClassBuilder(object):
         cd["__eq__"], cd["__ne__"], cd["__lt__"], cd["__le__"], cd[
             "__gt__"
         ], cd["__ge__"] = (
-            self._add_method_dunders(meth) for meth in _make_cmp(self._attrs)
+            self._add_method_dunders(meth)
+            for meth in _make_cmp(self._cls, self._attrs)
         )
 
         return self
@@ -986,7 +991,37 @@ def _attrs_to_tuple(obj, attrs):
     return tuple(getattr(obj, a.name) for a in attrs)
 
 
-def _make_hash(attrs, frozen, cache_hash):
+def _generate_unique_filename(cls, func_name):
+    """
+    Create a "filename" suitable for a function being generated.
+    """
+    unique_id = uuid.uuid4()
+    extra = ""
+    count = 1
+
+    while True:
+        unique_filename = "<attrs generated {0} {1}.{2}{3}>".format(
+            func_name,
+            cls.__module__,
+            getattr(cls, "__qualname__", cls.__name__),
+            extra,
+        )
+        # To handle concurrency we essentially "reserve" our spot in
+        # the linecache with a dummy line.  The caller can then
+        # set this value correctly.
+        cache_line = (1, None, (str(unique_id),), unique_filename)
+        if (
+            linecache.cache.setdefault(unique_filename, cache_line)
+            == cache_line
+        ):
+            return unique_filename
+
+        # Looks like this spot is taken. Try again.
+        count += 1
+        extra = "-{0}".format(count)
+
+
+def _make_hash(cls, attrs, frozen, cache_hash):
     attrs = tuple(
         a
         for a in attrs
@@ -995,10 +1030,7 @@ def _make_hash(attrs, frozen, cache_hash):
 
     tab = "        "
 
-    # We cache the generated hash methods for the same kinds of attributes.
-    sha1 = hashlib.sha1()
-    sha1.update(repr(attrs).encode("utf-8"))
-    unique_filename = "<attrs generated hash %s>" % (sha1.hexdigest(),)
+    unique_filename = _generate_unique_filename(cls, "hash")
     type_hash = hash(unique_filename)
 
     method_lines = ["def __hash__(self):"]
@@ -1055,7 +1087,7 @@ def _add_hash(cls, attrs):
     """
     Add a hash method to *cls*.
     """
-    cls.__hash__ = _make_hash(attrs, frozen=False, cache_hash=False)
+    cls.__hash__ = _make_hash(cls, attrs, frozen=False, cache_hash=False)
     return cls
 
 
@@ -1077,13 +1109,10 @@ WARNING_CMP_ISINSTANCE = (
 )
 
 
-def _make_cmp(attrs):
+def _make_cmp(cls, attrs):
     attrs = [a for a in attrs if a.cmp]
 
-    # We cache the generated eq methods for the same kinds of attributes.
-    sha1 = hashlib.sha1()
-    sha1.update(repr(attrs).encode("utf-8"))
-    unique_filename = "<attrs generated eq %s>" % (sha1.hexdigest(),)
+    unique_filename = _generate_unique_filename(cls, "eq")
     lines = [
         "def __eq__(self, other):",
         "    if other.__class__ is not self.__class__:",
@@ -1188,7 +1217,7 @@ def _add_cmp(cls, attrs=None):
         attrs = cls.__attrs_attrs__
 
     cls.__eq__, cls.__ne__, cls.__lt__, cls.__le__, cls.__gt__, cls.__ge__ = _make_cmp(  # noqa
-        attrs
+        cls, attrs
     )
 
     return cls
@@ -1258,14 +1287,11 @@ def _add_repr(cls, ns=None, attrs=None):
 
 
 def _make_init(
-    attrs, post_init, frozen, slots, cache_hash, base_attr_map, is_exc
+    cls, attrs, post_init, frozen, slots, cache_hash, base_attr_map, is_exc
 ):
     attrs = [a for a in attrs if a.init or a.default is not NOTHING]
 
-    # We cache the generated init methods for the same kinds of attributes.
-    sha1 = hashlib.sha1()
-    sha1.update(repr(attrs).encode("utf-8"))
-    unique_filename = "<attrs generated init {0}>".format(sha1.hexdigest())
+    unique_filename = _generate_unique_filename(cls, "init")
 
     script, globs, annotations = _attrs_to_init_script(
         attrs, frozen, slots, post_init, cache_hash, base_attr_map, is_exc


Test patch:
diff --git a/tests/test_dunders.py b/tests/test_dunders.py
index 94fdff8..69f91a8 100644
--- a/tests/test_dunders.py
+++ b/tests/test_dunders.py
@@ -58,6 +58,7 @@ def _add_init(cls, frozen):
     the tests for it are still useful to test the behavior of _make_init.
     """
     cls.__init__ = _make_init(
+        cls,
         cls.__attrs_attrs__,
         getattr(cls, "__attrs_post_init__", False),
         frozen,
@@ -759,3 +760,48 @@ class TestNothing(object):
         assert _Nothing() == _Nothing() == NOTHING
         assert not (_Nothing() != _Nothing())
         assert 1 != _Nothing()
+
+
+@attr.s(hash=True, cmp=True)
+class C(object):
+    pass
+
+
+# Store this class so that we recreate it.
+OriginalC = C
+
+
+@attr.s(hash=True, cmp=True)
+class C(object):
+    pass
+
+
+class TestFilenames(object):
+    def test_filenames(self):
+        """
+        The created dunder methods have a "consistent" filename.
+        """
+        assert (
+            OriginalC.__init__.__code__.co_filename
+            == "<attrs generated init tests.test_dunders.C>"
+        )
+        assert (
+            OriginalC.__eq__.__code__.co_filename
+            == "<attrs generated eq tests.test_dunders.C>"
+        )
+        assert (
+            OriginalC.__hash__.__code__.co_filename
+            == "<attrs generated hash tests.test_dunders.C>"
+        )
+        assert (
+            C.__init__.__code__.co_filename
+            == "<attrs generated init tests.test_dunders.C-2>"
+        )
+        assert (
+            C.__eq__.__code__.co_filename
+            == "<attrs generated eq tests.test_dunders.C-2>"
+        )
+        assert (
+            C.__hash__.__code__.co_filename
+            == "<attrs generated hash tests.test_dunders.C-2>"
+        )