Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init=False attributes which depend on keyword-only attributes are impossible #450

Closed
rgabbard opened this Issue Oct 8, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@rgabbard
Copy link
Contributor

rgabbard commented Oct 8, 2018

The initialization of an init=False attribute cannot (straightforwardly) depend on a kw_only=True attribute because there is no legal ordering of these two attributes.

from attr import attrs, attrib


@attrs
class BrokenInitFirst:
    _to_init: str  = attrib(init=False)
    kwarg: str = attrib(kw_only=True)

    @_to_init.default
    def _init_to_init(self) -> str:
        return self.kwarg + "foo"


BrokenInitFirst(kwarg="meep")

produces:

AttributeError: 'BrokenInitFirst' object has no attribute 'kwarg'

while

from attr import attrs, attrib


@attrs
class BrokenKwArgFirst:
    kwarg: str = attrib(kw_only=True)
    _to_init: str = attrib(init=False)

    @_to_init.default
    def _init_to_init(self) -> str:
        return self.kwarg + "foo"


BrokenKwArgFirst(kwarg="meep")

produces

  File "/Users/gabbard/anaconda3/envs/cwc-event/lib/python3.6/site-packages/attr/_make.py", line 904, in attrs
    return wrap(maybe_cls)
  File "/Users/gabbard/anaconda3/envs/cwc-event/lib/python3.6/site-packages/attr/_make.py", line 855, in wrap
    cache_hash,
  File "/Users/gabbard/anaconda3/envs/cwc-event/lib/python3.6/site-packages/attr/_make.py", line 471, in __init__
    cls, these, auto_attribs, kw_only
  File "/Users/gabbard/anaconda3/envs/cwc-event/lib/python3.6/site-packages/attr/_make.py", line 416, in _transform_attrs
    a=a
ValueError: Non keyword-only attributes are not allowed after a keyword-only attribute.  Attribute in question: Attribute(name='_to_init', default=Factory(factory=<function BrokenKwArgFirst._init_to_init at 0x10830df28>, takes_self=True), validator=None, repr=True, cmp=True, hash=None, init=False, metadata=mappingproxy({}), type=<class 'str'>, converter=None, kw_only=False)

These examples can also be found in https://github.com/rgabbard/attrs-kwonly-init-bug

This is related to #448 . Assuming there is some sort of internal ordering of attributes which also controls their initialization order, I think the solution here is to sort kw_only=True attributes after kw_only=False, and init=False last of all.

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Oct 8, 2018

At a quick glance it looks like performing the sort mentioned above (first kw_only=False, init=True, then kw_only=True, init=True, last init=False) around https://github.com/python-attrs/attrs/blob/master/src/attr/_make.py#L378 should do the trick for both this and #448 . @hynek , if this seems like the right strategy to you, I can make a PR with it.

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Oct 26, 2018

@hynek : bumping the question above, since I just ran into this problem again today. If you can confirm you are happy with the proposed approach to a fix, I am happy to submit a PR with it.

@hynek

This comment has been minimized.

Copy link
Member

hynek commented Oct 27, 2018

I’m sorry you hit me in the middle of my vacation and I'm having a hard time to catch up with the more complex issues.

And yes I agree that:

  1. init=False attributes should never cause a Non keyword-only attributes… error
  2. it’s kinda weird that we don’t allow kw_only attributes anywhere. someone should research why we chose that route. they are a lot more useful if you can sprinkle them anywhere.

So feel free to submit a PR if you manage to achieve that without breaking backward compatibility. Even in subclassing scenarios. I tend to think that it might be better to make the condition slightly more complicated instead of re-ordering them because that might have other unforeseen side effects.

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Oct 29, 2018

@hynek : After more thought I think you are correct that using a more complex condition is better. I will poke at this as I have free time. :-)

@wsanchez

This comment has been minimized.

Copy link

wsanchez commented Oct 29, 2018

@hynek: I don't think you commented on the BrokenInitFirst case. As with #448, it would introduce a (simpler) difference between the order of attributes at the order of arguments to __init__. But the arguments for allowing them anywhere would be similar, no?

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Oct 30, 2018

I went through the initial and final PRs as well as the three related issues (#38 , #106 , #335 ). It looks not allowing kw_only attributes anywhere was not a decision that was made very explicitly. The closest thing I see to a discussion of this is @hynek 's comment here that "I think it’s fair to expect that a non-kw_only attribute must not come after a kw_only attribute" and @Tinche 's comment:

Currently the definition order of attributes is the same as the order of the attributes in the generated __init__. This is strictly by @hynek's executive order and I personally agree with the rationale. If it wasn't intentional we would just sort the attributes and you wouldn't need to put arguments with defaults last. The order of definition is used in other places too: the __repr__, comparison methods, attr.fields.

and subsequent discussion.

I think the way forward is:

  • to maintain the current order of arguments (that is, declaration order) for purposes of repr, field initialization, etc.
  • to alter the condition to not thrown an exception if an init=False attribute follows a kw_only attribute
  • to float kw_only attributes to the end of the argument list when generating __init__
@wsanchez

This comment has been minimized.

Copy link

wsanchez commented Oct 31, 2018

I think that makes sense. It doesn't address BrokenInitFirst, though, right?

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Oct 31, 2018

@wsanchez : I am a little uncertain whether BrokenInitFirst needs to be fixed, since the general rule seems to be that an init=False attribute with a default needs to come later in the attribute ordering than any fields it depends on (me calling it Broken was misleading - I was just trying to illustrate that no legal attribute ordering for this case existed; as long as we fix one of the two options then the particular problem I had in mind is solved).

@wsanchez

This comment has been minimized.

Copy link

wsanchez commented Oct 31, 2018

Whoops sorry, I glossed over the dependency there, but mostly wanted to be clear that this case wasn’t addressed. I agree it shouldn’t be.

rgabbard added a commit to rgabbard/attrs that referenced this issue Oct 31, 2018

@rgabbard

This comment has been minimized.

Copy link
Contributor Author

rgabbard commented Nov 1, 2018

@hynek : PR ready at #459

@hynek hynek closed this in e114561 Nov 24, 2018

bors bot added a commit to mozilla/normandy that referenced this issue Mar 4, 2019

Merge #1784
1784: Scheduled weekly dependency update for week 09 r=peterbe a=pyup-bot






### Update [attrs](https://pypi.org/project/attrs) from **18.2.0** to **19.1.0**.


<details>
  <summary>Changelog</summary>
  
  
   ### 19.1.0
   ```
   -------------------

Backward-incompatible Changes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

- Fixed a bug where deserialized objects with ``cache_hash=True`` could have incorrect hash code values.
  This change breaks classes with ``cache_hash=True`` when a custom ``__setstate__`` is present.
  An exception will be thrown when applying the ``attrs`` annotation to such a class.
  This limitation is tracked in issue `494 &lt;https://github.com/python-attrs/attrs/issues/494&gt;`_.
  `482 &lt;https://github.com/python-attrs/attrs/issues/482&gt;`_


Changes
^^^^^^^

- Add ``is_callable``, ``deep_iterable``, and ``deep_mapping`` validators.

  * ``is_callable``: validates that a value is callable
  * ``deep_iterable``: Allows recursion down into an iterable,
    applying another validator to every member in the iterable
    as well as applying an optional validator to the iterable itself.
  * ``deep_mapping``: Allows recursion down into the items in a mapping object,
    applying a key validator and a value validator to the key and value in every item.
    Also applies an optional validator to the mapping object itself.

  You can find them in the ``attr.validators`` package.
  `425 &lt;https://github.com/python-attrs/attrs/issues/425&gt;`_
- Fixed stub files to prevent errors raised by mypy&#39;s ``disallow_any_generics = True`` option.
  `443 &lt;https://github.com/python-attrs/attrs/issues/443&gt;`_
- Attributes with ``init=False`` now can follow after ``kw_only=True`` attributes.
  `450 &lt;https://github.com/python-attrs/attrs/issues/450&gt;`_
- ``attrs`` now has first class support for defining exception classes.

  If you define a class using ``attr.s(auto_exc=True)`` and subclass an exception, the class will behave like a well-behaved exception class including an appropriate ``__str__`` method, and all attributes additionally available in an ``args`` attribute.
  `500 &lt;https://github.com/python-attrs/attrs/issues/500&gt;`_
- Clarified documentation for hashing to warn that hashable objects should be deeply immutable (in their usage, even if this is not enforced).
  `503 &lt;https://github.com/python-attrs/attrs/issues/503&gt;`_


----
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/attrs
  - Changelog: https://pyup.io/changelogs/attrs/
  - Homepage: https://www.attrs.org/
</details>





### Update [botocore](https://pypi.org/project/botocore) from **1.12.101** to **1.12.106**.


<details>
  <summary>Changelog</summary>
  
  
   ### 1.12.106
   ```
   ========

* api-change:``ec2``: Update ec2 client to latest version
* api-change:``autoscaling-plans``: Update autoscaling-plans client to latest version
   ```
   
  
  
   ### 1.12.105
   ```
   ========

* api-change:``ssm``: Update ssm client to latest version
* api-change:``apigatewayv2``: Update apigatewayv2 client to latest version
* api-change:``alexaforbusiness``: Update alexaforbusiness client to latest version
* api-change:``application-autoscaling``: Update application-autoscaling client to latest version
   ```
   
  
  
   ### 1.12.104
   ```
   ========

* api-change:``waf-regional``: Update waf-regional client to latest version
* api-change:``waf``: Update waf client to latest version
   ```
   
  
  
   ### 1.12.103
   ```
   ========

* api-change:``discovery``: Update discovery client to latest version
* api-change:``organizations``: Update organizations client to latest version
* api-change:``resource-groups``: Update resource-groups client to latest version
* api-change:``opsworkscm``: Update opsworkscm client to latest version
* api-change:``pinpoint``: Update pinpoint client to latest version
* api-change:``mediaconvert``: Update mediaconvert client to latest version
* api-change:``cur``: Update cur client to latest version
   ```
   
  
  
   ### 1.12.102
   ```
   ========

* api-change:``elbv2``: Update elbv2 client to latest version
* api-change:``mediastore``: Update mediastore client to latest version
* api-change:``ce``: Update ce client to latest version
* api-change:``autoscaling``: Update autoscaling client to latest version
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/botocore
  - Changelog: https://pyup.io/changelogs/botocore/
  - Repo: https://github.com/boto/botocore
</details>





### Update [cffi](https://pypi.org/project/cffi) from **1.12.1** to **1.12.2**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/cffi
  - Changelog: https://pyup.io/changelogs/cffi/
  - Docs: http://cffi.readthedocs.org
</details>





### Update [idna](https://pypi.org/project/idna) from **2.6** to **2.8**.


<details>
  <summary>Changelog</summary>
  
  
   ### 2.8
   ```
   ++++++++++++++++

- Update to Unicode 11.0.0.
- Provide more specific exceptions for some malformed labels.
   ```
   
  
  
   ### 2.7
   ```
   ++++++++++++++++

- Update to Unicode 10.0.0.
- No longer accepts dot-prefixed domains (e.g. &quot;.example&quot;) as valid.
  This is to be more conformant with the UTS 46 spec. Users should
  strip dot prefixes from domains before processing.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/idna
  - Changelog: https://pyup.io/changelogs/idna/
  - Repo: https://github.com/kjd/idna
</details>





### Update [pyflakes](https://pypi.org/project/pyflakes) from **2.1.0** to **2.1.1**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pyflakes
  - Changelog: https://pyup.io/changelogs/pyflakes/
  - Repo: https://github.com/PyCQA/pyflakes
</details>





### Update [google-api-core](https://pypi.org/project/google-api-core) from **1.7.0** to **1.8.0**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/google-api-core
  - Repo: https://github.com/GoogleCloudPlatform/google-cloud-python
</details>





### Update [google-cloud-core](https://pypi.org/project/google-cloud-core) from **0.28.1** to **0.29.1**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/google-cloud-core
  - Repo: https://github.com/GoogleCloudPlatform/google-cloud-python
</details>





### Update [protobuf](https://pypi.org/project/protobuf) from **3.6.1** to **3.7.0**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/protobuf
  - Changelog: https://pyup.io/changelogs/protobuf/
  - Repo: https://github.com/protocolbuffers/protobuf/releases
  - Homepage: https://developers.google.com/protocol-buffers/
</details>





### Update [boto3](https://pypi.org/project/boto3) from **1.9.101** to **1.9.106**.


<details>
  <summary>Changelog</summary>
  
  
   ### 1.9.106
   ```
   =======

* api-change:``ec2``: [``botocore``] Update ec2 client to latest version
* api-change:``autoscaling-plans``: [``botocore``] Update autoscaling-plans client to latest version
   ```
   
  
  
   ### 1.9.105
   ```
   =======

* api-change:``ssm``: [``botocore``] Update ssm client to latest version
* api-change:``apigatewayv2``: [``botocore``] Update apigatewayv2 client to latest version
* api-change:``alexaforbusiness``: [``botocore``] Update alexaforbusiness client to latest version
* api-change:``application-autoscaling``: [``botocore``] Update application-autoscaling client to latest version
   ```
   
  
  
   ### 1.9.104
   ```
   =======

* api-change:``waf-regional``: [``botocore``] Update waf-regional client to latest version
* api-change:``waf``: [``botocore``] Update waf client to latest version
   ```
   
  
  
   ### 1.9.103
   ```
   =======

* api-change:``discovery``: [``botocore``] Update discovery client to latest version
* api-change:``organizations``: [``botocore``] Update organizations client to latest version
* api-change:``resource-groups``: [``botocore``] Update resource-groups client to latest version
* api-change:``opsworkscm``: [``botocore``] Update opsworkscm client to latest version
* api-change:``pinpoint``: [``botocore``] Update pinpoint client to latest version
* api-change:``mediaconvert``: [``botocore``] Update mediaconvert client to latest version
* api-change:``cur``: [``botocore``] Update cur client to latest version
   ```
   
  
  
   ### 1.9.102
   ```
   =======

* api-change:``elbv2``: [``botocore``] Update elbv2 client to latest version
* api-change:``mediastore``: [``botocore``] Update mediastore client to latest version
* api-change:``ce``: [``botocore``] Update ce client to latest version
* api-change:``autoscaling``: [``botocore``] Update autoscaling client to latest version
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/boto3
  - Changelog: https://pyup.io/changelogs/boto3/
  - Repo: https://github.com/boto/boto3
</details>





### Update [django-cors-headers](https://pypi.org/project/django-cors-headers) from **2.4.0** to **2.4.1**.


<details>
  <summary>Changelog</summary>
  
  
   ### 2.4.1
   ```
   ------------------

* Fix ``DeprecationWarning`` from importing ``collections.abc.Sequence`` on
  Python 3.7.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/django-cors-headers
  - Changelog: https://pyup.io/changelogs/django-cors-headers/
  - Repo: https://github.com/ottoyiu/django-cors-headers
</details>





### Update [django-mozilla-product-details](https://pypi.org/project/django-mozilla-product-details) from **0.13** to **0.13.1**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/django-mozilla-product-details
  - Repo: https://github.com/mozilla/django-product-details/
</details>





### Update [djangorestframework](https://pypi.org/project/djangorestframework) from **3.9.1** to **3.9.2**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/djangorestframework
  - Changelog: https://pyup.io/changelogs/djangorestframework/
  - Homepage: https://www.django-rest-framework.org/
</details>





### Update [jsonschema](https://pypi.org/project/jsonschema) from **3.0.0** to **3.0.1**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/jsonschema
  - Changelog: https://pyup.io/changelogs/jsonschema/
  - Repo: https://github.com/Julian/jsonschema
</details>





### Update [pytest-django](https://pypi.org/project/pytest-django) from **3.4.7** to **3.4.8**.


<details>
  <summary>Changelog</summary>
  
  
   ### 3.4.8
   ```
   ------------------

Bugfixes
^^^^^^^^

* Fix DB renaming fixture for Multi-DB environment with SQLite (679)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest-django
  - Changelog: https://pyup.io/changelogs/pytest-django/
  - Docs: https://pytest-django.readthedocs.io/
</details>





### Update [google-cloud-storage](https://pypi.org/project/google-cloud-storage) from **1.13.0** to **1.14.0**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/google-cloud-storage
  - Repo: https://github.com/GoogleCloudPlatform/google-cloud-python
</details>







Co-authored-by: pyup-bot <github-bot@pyup.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.