Stop modifying classes and always create new ones #260

hynek · 2017-10-06T15:51:15Z

This allows us to harmonize class creation for dict and slots classes.

Interestingly this seems to break our Hypothesis strategies. I can't dig any further right now but I wanted to put it out there. Helpful comments welcome.

Ref #223 #202

This allows us to harmonize class creation for dict and slots classes. Ref #223 #202

Tinche · 2017-10-06T17:13:52Z

I’m happy to take a look

…

On Friday, October 6, 2017, Hynek Schlawack ***@***.***> wrote: This allows us to harmonize class creation for dict and slots classes. Interestingly this seems to break our Hypothesis strategies. I can't dig any further right now but I wanted to put it out there. Ref #223 <#223> #202 <#202> ------------------------------ You can view, comment on, or merge this pull request online at: #260 Commit Summary - Add replace_class argument to attr.s - Use replaced classes in testing strategy File Changes - *A* changelog.d/260.deprecation.rst <https://github.com/python-attrs/attrs/pull/260/files#diff-0> (8) - *M* docs/api.rst <https://github.com/python-attrs/attrs/pull/260/files#diff-1> (2) - *M* src/attr/_make.py <https://github.com/python-attrs/attrs/pull/260/files#diff-2> (10) - *M* tests/test_dark_magic.py <https://github.com/python-attrs/attrs/pull/260/files#diff-3> (80) - *M* tests/utils.py <https://github.com/python-attrs/attrs/pull/260/files#diff-4> (14) Patch Links: - https://github.com/python-attrs/attrs/pull/260.patch - https://github.com/python-attrs/attrs/pull/260.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#260>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0h8Qc8dZeGrjl_PSN85kJg4b19oUY2ks5spkx0gaJpZM4PwrtR> .

hynek · 2017-10-06T19:05:41Z

A few more notes:

I'm not married to the naming.
I’m not raising a warning because it seems unfortunate to force people into setting the argument and makes it kind of impossible to remove it later. My hope is that most people don't ever realize that we changed something underneath.
I hate Python 2.

Tinche · 2017-10-07T11:44:30Z

Hm after a 5 minute investigation, there seems to be a problem with copy.copy on classes that are replace_class=True, frozen=True (and only those, slot classes are OK). Will look into it more later.

On a side note, is it possible to overwrite the class __repr__? As in, not the instance repr. Our Hypothesis classes are called HypClass, would be nicer if their repr had the flags too. I mean, I could always just change the name...

hynek · 2017-10-08T13:59:42Z

🤦‍♂️

codecov · 2017-10-08T14:02:45Z

Codecov Report

Merging #260 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #260   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         609    653   +44     
  Branches      134    135    +1     
=====================================
+ Hits          609    653   +44

Impacted Files	Coverage Δ
src/attr/_make.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c5677b...09c94b4. Read the comment docs.

changelog.d/260.deprecation.rst

+For the next year, the default value is ``replace_classes=False`` which is the current behavior.
+
+The default value will change to ``True`` no sooner than one year after this release.
+There are no concrete plans to remove ``replace_classes=False`` altogether but its use is discouraged.


hynek · 2017-10-09T09:35:51Z

So um @glyph and @njsmith, any ideas/opinions on how to make this thing happen without pissing people off unnecessarily?

hynek · 2017-10-10T10:45:17Z

Just to add another thought: my wish is that ideally, nobody notices a difference at all. So a three stage approach with a *kw-warning in two years seems best:

now: add option, default is False
Oct 2018: default is True
Oct 2019: option becomes a nop and starts raising a warning

Now, this kind of costs us a pointless year because people are not gonna try out that option unless they have a problem. It's like with pre-releases, nobody tests them.

Ergo, in Oct 2018, there will be people yelling at us in any case and we've maybe saved <0.01% of our users some headaches.

OTOH, I think I’ll do it anyway because I'm totally not ready to break hashing a second time this year. :| Also we'll be able to point to this: yes we did the right thing.

I really hope this is the last fundamental screwup in attrs. :|

I’d really like more input on this…happily also from @wsanchez @Lukasa @DRMacIver @wbolster @hawkowl

hynek · 2017-10-10T14:27:01Z

I have added the current plan to the news fragment.

DRMacIver · 2017-10-10T15:21:30Z

I don't think I actually understand the problem well enough to comment yet. What actually is the interaction with hashing and equality here that causes the problem?

I assume the "super" problem is that if I have something like:

@attr.s()
class Foo(Bar):
    def stuff(self):
        return super(Foo, self).stuff()

Then the "Foo" in the method is different from the Foo outside the method and this causes a supertype mismatch of some sort? I assume the class you're creating can't be a subclass of the existing Foo. Are you copying over the namespace and is that what causes you problems?

DRMacIver · 2017-10-10T15:38:23Z

OK. I've now done the reading of the news fragment and understand some of this better.

On the hashing thing... I think I'd personally be inclined to be less nice than you. I would argue that getting a hash when passing hash=False is just a bug and people relying on it are relying on a bug, and that just fixing that bug would be a non-breaking change. As you say, people are going to suffer that change at some point anyway, so I think you might as well just rip off the band-aid. ETA: At a minimum I think anyone relying on that hash should start getting big scary warnings when it gets called.

Personally the idea of adding a replace_class argument to attr.s doesn't sit well with me, especially when defaulting it to false. One of the rules we have in Hypothesis code review is that when adding a setting you're supposed to ask whether the user is ever likely to actually have a reason to care about the value of this setting. When adding a new scary change the user might care about being able to back it out, but they're certainly not going to care to opt into it because replace_class is about as internal to attrs as it gets.

Were it me I would adopt the following plan:

No optional behaviour. Just opt everyone into this straight up.
Think real hard about possible gotchas and how to fix them.
Before releasing, try running the test suite of a few of the larger open source users of attrs and see if anything hilarious happens.
Be on the ball for a certain amount of user screaming.
In the unlikely event that there are any actual unfixable bugs caused by this, add a replace_class option defaulting to True that emits a warning when you set it to False.

Note however this is coming from the guy who thought "Lets get real drunk and turn coverage on for every Hypothesis user" was a good idea, so I may have more of a YOLO attitude to feature evolution than you are comfortable with. 😉

wsanchez · 2017-10-10T16:53:33Z

I tend to agree with @DRMacIver, though it's possible that all the wine I had over the weekend is influencing me into associating with the "Lets get real drunk and…" camp.

I certainly agree regarding the hashing behavior; that's a bug and it should just get fixed. I don't see a strong argument for bending over to provide compatibility there; at a minimum, I'd have replace_class default to True, but as a user I'd rather fix the bug if this effects me than opt into buggy behavior. (But, to be fair, I also don't expect his to effect me.)

hynek · 2017-10-10T17:01:01Z

I’d like to stress one thing: hash=False is supposed to give you hash-by-id, iff, cmp=False on Python 3. It will always give you one on Python 2.

What we could do is using this as a unification/simplification for creating classes and attach object.__hash__ for non-slot classes to not breaking any compatibility.

Then continue with #202 and use an enum to control what actually happens and deprecate using bool for the hash argument. 🤔

DRMacIver · 2017-10-10T17:36:59Z

@hynek that sounds sensible to me.

hynek · 2017-10-13T06:08:37Z

OK, I’ve spent yesterday basically rewriting the class generation code because the old one was all about tacking on and then copying or removing.

Now it's much cleaner. There might be some doc inconsistencies I guess? I’m much happier with how it works now.

Complete rewrite, we need to re-review.

changelog.d/223.change.rst

+``attrs``-decoratorated classes are never modified now.
+Instead a new class is created with the appropriated methods and attributes attached from the get go.
+
+For backward-compatibility, the behavior of getting hashing by ID on Python 3 when passing ``@attr.s(hash=False)`` is manually retained by setting ``__hash__`` to ``object.__hash__``.


changelog.d/260.change.rst

+``attrs``-decoratorated classes are never modified now.
+Instead a new class is created with the appropriated methods and attributes attached from the get go.
+
+For backward-compatibility, the behavior of getting hashing by ID on Python 3 when passing ``@attr.s(hash=False)`` is manually retained by setting ``__hash__`` to ``object.__hash__``.


DRMacIver · 2017-10-13T19:58:50Z

What we could do is using this as a unification/simplification for creating classes and attach object.__hash__ for non-slot classes to not breaking any compatibility.

Sorry, I somehow misparsed what you originally said (which was totally unambiguous) and interpreted it as something slightly different that I think is a better idea.

What I would actually recommend here is that the default behaviour is to attach a __hash__ which calls object.__hash__ and returns its value, but also emits a deprecation warning saying that if this is the behaviour you want then you should be explicit about wanting it.

hynek · 2017-10-14T10:35:31Z

What I would actually recommend here is that the default behaviour is to attach a hash which calls object.hash and returns its value, but also emits a deprecation warning saying that if this is the behaviour you want then you should be explicit about wanting it.

The problem is that as of now, there is no explicit way of defining it. You totally can ask for unhashable but with cmp and the behavior also differs between Python versions. That’s why I want to start raising a warning as soon as there is actually a good way.

They may contain a unhashable default.

hynek · 2017-10-17T13:11:52Z

OK folks, I get it: this PR is fucking scary.

So I’m not gonna ask you to review and risk to share the blame. Mut what I am gonna ask you is to run it against your code bases and report back any breakage you encounter. Technically, nothing should break.

I just did that and the result is 6dd4b1d. I’ll take all the blame, but please help me not have an emergency 17.4 by running your test suite once. :D

DRMacIver

I've attempted a review of this and decided that I don't really feel like I know this code well enough to do approve/request changes, but I've added some comments inline.

src/attr/_make.py

+            super_attrs.extend(
+                a
+                for a in sub_attrs
+                if a not in super_attrs and a.name not in non_super_names


src/attr/_make.py

+                continue
+            for cell in closure_cells:
+                if cell.cell_contents is self._cls:
+                    set_closure_cell(cell, cls)


src/attr/_make.py

            raise TypeError(
                "Invalid value for hash.  Must be True, False, or None."
            )
        elif hash is False or (hash is None and cmp is False):
-            pass
+            builder.add_hash_by_id()


src/attr/_make.py

    """
    if these is None:
        ca_list = [(name, attr)
                   for name, attr
                   in cls.__dict__.items()
                   if isinstance(attr, _CountingAttr)]
-        for name, _ in ca_list:
-            delattr(cls, name)


DRMacIver · 2017-10-17T14:12:31Z

FWIW I've run the Hypothesis test suite with these changes and it seems fine, but I also don't think Hypothesis makes interesting enough use of attrs that that's a meaningful test of anything.

Tinche · 2017-10-17T14:33:08Z

I got one:

import attr
from typing import TypeVar, Generic

G = TypeVar('G', bound=int)


@attr.s
class A(Generic[G]):
    a = attr.ib()

Works with PyPI, not with this branch (on 3.6). GL :D

hynek · 2017-10-17T19:15:50Z

Thanks @DRMacIver, your look is very much appreciated!

@Tinche not sure if trolling? :)

Tinche · 2017-10-17T20:47:30Z

I'm not trolling, this was a failure from my work codebase. I've simplified it of course.

hynek · 2017-10-17T21:04:53Z

Related to the type work, not this PR right? Maybe open new issue and set milestone?

wsanchez · 2017-10-17T21:30:16Z

Well so I was wrong… this did break my code, specifically, this test.

This isn't my favorite code, so please don't judge me. :-)

I have a hack mix-in class I use for attrs-classes where I want some simple control over how comparisons are done… anyway it fails here.

wsanchez · 2017-10-17T21:35:43Z

(I'm 100% OK with changing this code, by the way, just reporting on test results, as requested.)

The test fails if we try to do a `hash is False and slots is True`.

hynek · 2017-10-19T11:35:46Z

Great news everyone, we’ve got ourselves another edge case:

import attr

@attr.s
class BaseNoSlots:
  def __init_subclass__(cls, param, **kwargs):
    super().__init_subclass__(**kwargs)
    cls.param = param

@attr.s    
class Foo(BaseNoSlots, param='foo'):
  pass

print(Foo.param)

This is taken from #269 and https://repl.it/Mmkb/0

Since we didn’t intervene into the class generation for non-slots before, it just worked.

Now we get:

>>> import attr
...
... @attr.s
... class BaseNoSlots:
...   def __init_subclass__(cls, param, **kwargs):
...     super().__init_subclass__(**kwargs)
...     cls.param = param
...
... @attr.s
... class Foo(BaseNoSlots, param='foo'):
...   pass
Traceback (most recent call last):
  File "<stdin>", line 10, in <module>
  File "/Users/hynek/Projects/attrs/src/attr/_make.py", line 573, in attrs
    return wrap(maybe_cls)
  File "/Users/hynek/Projects/attrs/src/attr/_make.py", line 566, in wrap
    return builder.build()
  File "/Users/hynek/Projects/attrs/src/attr/_make.py", line 379, in build
    cd,
TypeError: __init_subclass__() missing 1 required positional argument: 'param'
__init_subclass__() missing 1 required positional argument: 'param'

hynek · 2017-10-19T11:44:25Z

In any case: I’m going to make this PR only about the refactoring which is a net win. Then we’ll get attrs 17.3 out of the door and then we‘ll tackle it again. Maybe we’ll find a better way to attach __slots__.

hynek added 2 commits October 6, 2017 17:29

Add replace_class argument to attr.s

ca83971

This allows us to harmonize class creation for dict and slots classes. Ref #223 #202

Use replaced classes in testing strategy

9ea23f9

hynek mentioned this pull request Oct 6, 2017

release attrs 17.3.0? #259

Closed

Fix replace_class always generating slot classes.

33d61bc

Tinche requested changes Oct 8, 2017

View reviewed changes

Tinche previously approved these changes Oct 9, 2017

View reviewed changes

Polish changelog

84a89fb

Always replace, add a compat shim for hashing by object ID

f7a4c61

hynek changed the title ~~Add replace_class argument to attr.s~~ Stop modifying classes and always create new ones Oct 13, 2017

wsanchez reviewed Oct 13, 2017

View reviewed changes

hynek added 2 commits October 14, 2017 13:02

Better changelog wording

645dee7

Uncomment assert

56bc177

hynek force-pushed the replace-class branch from 0044d83 to 56bc177 Compare October 14, 2017 12:03

hynek added this to the 17.3 milestone Oct 16, 2017

wsanchez and others added 2 commits October 16, 2017 15:54

Merge branch 'master' into replace-class

ac432b1

Leave super_attrs a tuple

6dd4b1d

They may contain a unhashable default.

Fix how-does-it-work to new approach

628770c

DRMacIver reviewed Oct 17, 2017

View reviewed changes

hynek mentioned this pull request Oct 17, 2017

slots=True do not play nice with __init_subclass__ #269

Closed

hynek and others added 6 commits October 19, 2017 10:29

Add comment, add regression test

cff9557

The test fails if we try to do a `hash is False and slots is True`.

Clarify by looping

11e38bb

Fix super class attribute overwriting

507d9d5

Merge branch 'master' into replace-class

44c7ade

Fix cell rewriting some more. (#270)

69740a0

Add end-to-end test for effects of cells and __init_subclass__

09c94b4

hynek force-pushed the replace-class branch from 1b5436c to 09c94b4 Compare October 19, 2017 11:16

hynek mentioned this pull request Oct 19, 2017

[RFC] Quo vadis re: classes #223

Closed

hynek closed this Oct 19, 2017

hynek deleted the replace-class branch October 19, 2017 12:59

hynek mentioned this pull request Oct 19, 2017

Refactor class creation #272

Merged

Stop modifying classes and always create new ones #260

Stop modifying classes and always create new ones #260

Conversation

hynek commented Oct 6, 2017 • edited Loading

Tinche commented Oct 6, 2017 via email

hynek commented Oct 6, 2017

Tinche commented Oct 7, 2017

hynek commented Oct 8, 2017

codecov bot commented Oct 8, 2017 • edited Loading

Codecov Report

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

hynek commented Oct 9, 2017

hynek commented Oct 10, 2017

hynek commented Oct 10, 2017

DRMacIver commented Oct 10, 2017

DRMacIver commented Oct 10, 2017 • edited Loading

wsanchez commented Oct 10, 2017

hynek commented Oct 10, 2017

DRMacIver commented Oct 10, 2017

hynek commented Oct 13, 2017

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DRMacIver commented Oct 13, 2017 • edited Loading

hynek commented Oct 14, 2017

hynek commented Oct 17, 2017

DRMacIver left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

DRMacIver commented Oct 17, 2017

Tinche commented Oct 17, 2017

hynek commented Oct 17, 2017

Tinche commented Oct 17, 2017

hynek commented Oct 17, 2017 via email

wsanchez commented Oct 17, 2017

wsanchez commented Oct 17, 2017

hynek commented Oct 19, 2017

hynek commented Oct 19, 2017 • edited Loading

hynek commented Oct 6, 2017 •

edited

Loading

codecov bot commented Oct 8, 2017 •

edited

Loading

DRMacIver commented Oct 10, 2017 •

edited

Loading

DRMacIver commented Oct 13, 2017 •

edited

Loading

hynek commented Oct 19, 2017 •

edited

Loading