BUG: fix evolve behaviour when a validator fails.

[cournape]

This is a naive solution to #175. TypeError may be coming from many sources, and it is not correct to assume it always comes from a non-existing attr member as the evolve codes now. Instead, we now check specifically for this case when raising a custom error, and just re-raise in other cases.

Fixes #175

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #176   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         551    555    +4     
  Branches      122    125    +3     
=====================================
+ Hits          551    555    +4

Impacted Files	Coverage Δ
src/attr/_funcs.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 a48124b...82d610f. Read the comment docs.

[Tinche]

So this is error handling case so performance doesn't really matter here, but there might be a more efficient way.

Instead of constructing a dictionary, could you try something like (pseudocode):

attrs = fields(cls)
for name in changes:
    if getattr(attrs, name, None) is None:
        raise AttrsAttributeNotFoundError

Have to run now, will look again later.

[Tinche]

pyperf shows the getattr approach is a little faster so let's stick with that.

[cournape]

So there is actually a problem with this solution, because changes is mutated to handle private args. What is the desired behaviour here ? E.g. which ones should work/fail in the following:

from attr import attributes, attr, evolve
from attr.validators import instance_of


@attributes
class C(object):
    a = attr(validator=instance_of(int))
    _b = attr(validator=instance_of(int))


c1 = C(1, 2)
# Should work ? Right now it does not (c2.b == 2)
c2 = evolve(c1, b=3)
# Should work ? That behaves as expected
c3 = evolve(c1, _b=3)
# Should that really work ??
c4 = evolve(c1, _b=3, b=4)

[Tinche]

Huh, good catch. There was a small discussion around whether to support the setter syntax or the init syntax over at #116. We settled on supporting both, since I figured it was easy to do and there's no ambiguity in the attribute names (i.e. you can't have _b and b attributes). Let's revisit this.

c2 = evolve(c1, b=3) not working at all is definitely not good. The fix depends on the following issue.

I don't actually know what the right way is to resolve evolve(c1, _b=3, b=4). The fact this is not obvious to me is kind of an argument in favor of not supporting both syntaxes. Since this is a design issue and not an implementation thing let's ask @hynek for his opinion.

Given the additional discussion of private attributes from #171, I'm in favor of only supporting the setter syntax in evolve. (Summoning @glyph here as well.) So only evolve(c1, _b=3) would be valid, all the others would throw.

[Tinche]

This code would be a simple fix I think.

        attr_name = a.name  # To deal with private attributes.
        if attr_name[0] == "_":
            init_name = attr_name[1:]
            if attr_name in changes:
                # attr_name (not init_name) in changes, translate
                changes[init_name] = changes.pop(attr_name)
            elif init_name not in changes:
                changes[init_name] = getattr(inst, attr_name)
            # else: init_name is already in changes, leave it there
        else:
            if attr_name not in changes:
                changes[attr_name] = getattr(inst, attr_name)

But it still doesn't fix evolve(c1, b=3, _b=4).

[cournape]

Fixing should be fairly easy once we agree on what the behaviour should be. Detecting evolve(b=.., _b=...) should be straightforward (detect dict length change), and then you can do more work to figure out which attributes are given "twice" in the except block.

[hynek]

Is this blocking on something?

[cournape]

Can you comment on #176 (comment) ?

[hynek]

Paging @glyph because he expressed opinions on this exact topic recently.

[glyph]

I think the question of which syntax to use actually cuts to the heart of a serious design problem with evolve, which is: evolve really can't reasonably be a public API.

Consider this case:

import attr
@attr.s
class _Tree(object):            # private type
    _parent = attr.ib(default=None)
    _children = attr.ib(default=attr.Factory(list))
    def add(self, child):
        if child._parent is not None:
            child._parent = self
        self._children.append(child)
def tree(children=()):          # public constructor
    new = _Tree()
    for child in children:
        new.add(child)
# ... application ...
leaf = tree()
branch = tree(leaf)
root = tree(branch)

If you call evolve() on one of these objects, and pass parent (or _parent) or child but not both, you're creating an inconsistent state, and nothing really told you that you were doing that.

The reason I'm raising this here is that the question as to whether to give evolve parameters underscores or not is the question as to whether the parameters are considered "same 'public' status as __init__ arguments" or "same 'public' status as attributes". Except it turns out that it doesn't matter because evolve is apparently public even when the attributes and constructor are both private.

If evolve itself remains public, I think that dropping the underscores makes sense, as it's effectively a "void the warranty" type API, where you should either call it on self or objects where you explicitly know it's safe (immutable value types, for example).

However, if we did something like this:

@attr.s
class Point3D:
    x = attr.ib()
    y = attr.ib()
    z = attr.ib()
    evolve = attr.evolver()

Point3D(1, 2, 3).evolve(y=7)

so that classes could explicitly "opt in" to evolvulation then it would make sense to use the private attribute names in that case.

I suspect that this view may be controversial though. Thoughts?

[hynek]

My thoughts at this point is that nobody building APIs should even expose the fact their classes are attrs-based. It has always been my focus to make attrs transparent to endusers.

Honestly, I’ve personally even arrived at the point where I treat __init__s as private APIs but I think that’s a bridge too far for most. :)

In my mind we have two conflicting arguments:

1. __init__ strips underscores because it’s potentially a public API – making evolve() behave the same way would be nice
2. attr.evolve() should never be part of a public API of anyone so it really should not strip. It’s like a setter for FP-connoisseurs.

Did I forget something? My mind is wandering towards nr. 2 or

[glyph]

My inclination is to say that for now, we should go ahead and make it consistent with __init__, since the semantics are "make a new thing like this, except...", and "make a new thing like this" is ___init__, and making it behave differently from __init__ is just confusing. (Perhaps __init__ should accept underscored and non-underscored kw variants as well, I don't have as strong an opinion about that; but however it works, it should be the same between them.)

But, the documentation should be adjusted to make it clear that the first argument to evolve must be a namespace that the caller controls, and that just like typing ._, typing evolve voids your warranty.

The only real problem with this is that sometimes the constructor has been made private through some other shenanigans, but evolve made it public again indirectly through the instance. Upon further reflection, this problem goes deeper than evolve itself: consider type(x)(args, for, new, x). No underscores typed there either.

Rather than try to hack up evolve to make this continue to appear to work, maybe let's discuss first-class support for private constructors on a different issue, and come up with a way that fields and evolve should interact with that separately.

[hynek]

OK so back to my original hunch: evolve() shall behave like __init__ as well as possible.

[hynek]

JFTR @cournape , we have an agreement above. :) This needs to be fixed before 17.1, so I’d be super grateful if you could finish it up! No hurry though, there’s still some stuff open. Just wanted to stress that this should be unblocked (and if not, please yell at us).

[Tinche]

Thank you for your efforts @cournape, I've added a few commits and made PR #182 to get this done.