Repository: pydantic/pydantic
Pull request: Use `__pydantic_private__` for storing private attributes
Closes https://github.com/pydantic/pydantic/issues/3523

Requires https://github.com/pydantic/pydantic-core/pull/640.

The only weird thing remaining is that the private attributes are no longer member descriptors. I don't know how much that matters.. But the test of `__set_name__` functionality didn't require modifications.. 🤷 



Selected Reviewer: @hramezani

Problem statement:
Layout conflict in ModelMetaclass when mixing in two models with private variables.
# Bug
Disclaimer: I have checked out stackoverflow, issue tracker, docs, google etc. for resources on this, but didnt found any solution. 

Output of `python -c "import pydantic.utils; print(pydantic.utils.version_info())"`:
```
             pydantic version: 1.8.2
            pydantic compiled: True
                 install path: /home/igor/code/issues-reports/pydantic-private-mixins/venv/lib/python3.9/site-packages/pydantic
               python version: 3.9.9 (main, Nov 16 2021, 03:08:02)  [GCC 9.3.0]
                     platform: Linux-5.11.0-34-generic-x86_64-with-glibc2.31
     optional deps. installed: ['typing-extensions']

```

Following code looks innocent, It just uses mixin models to merge some private attributes in one bigger model:

```py
import pydantic
import typing as t


class ModelMixin(pydantic.BaseModel):
    _mixin_private: t.Optional[str] = pydantic.PrivateAttr(None)


class Model(pydantic.BaseModel):
    public: str = "default"
    _private: t.Optional[str] = pydantic.PrivateAttr(None)


class NewModel(ModelMixin, Model):
    pass

```
But quite surprisingly it raises a `TypeError` on a class creation time:

```
Traceback (most recent call last):
  File "/code/issues-reports/pydantic-private-mixins/report.py", line 14, in <module>
    class NewModel(ModelMixin, Model):
  File "pydantic/main.py", line 367, in pydantic.main.ModelMetaclass.__new__
  File "/usr/lib/python3.9/abc.py", line 106, in __new__
    cls = super().__new__(mcls, name, bases, namespace, **kwargs)
TypeError: multiple bases have instance lay-out conflict
```
It seems that **at least two** models with private attributes will trigger this exception. When private attributes are defined in only one of the models, everything works great. 

I can dig into this, but probably not this week. But please confirm that this is an issue, not the intended behavior. Also, if You have any ideas/hints where to start, it would be appreciated!

Hints from issue discussion:
Okay, some update. 
It seems like problem with `__slots__` and multiple inheritance. Pydantic's documentation states directly, that creating a private attribute will also create a `__slots__` entry:
> [Upon class creation pydantic constructs __slots__ filled with private attributes.](https://pydantic-docs.helpmanual.io/usage/models/#private-model-attributes)

This behaviour can be confirmed when inspecting metaclass runtime details for all three models from my code example:

```
ModelMixin:
    Bases + mro:
        (<class 'pydantic.main.BaseModel'>, <class 'pydantic.utils.Representation'>, <class 'object'>)

    Namespace:
{       '__annotations__': {'_mixin_private': typing.Optional[str]},
        '__class_vars__': set(),
        '__config__': <class '__main__.Config'>,
        '__custom_root_type__': False,
        '__fields__': {},
        '__hash__': None,
        '__json_encoder__': <staticmethod object at 0x7f9745885460>,
        '__module__': '__main__',
        '__post_root_validators__': [],
        '__pre_root_validators__': [],
        '__private_attributes__': {'_mixin_private': ModelPrivateAttr()},
        '__qualname__': 'ModelMixin',
        '__schema_cache__': {},
        '__slots__': {'_mixin_private'},
        '__validators__': {}}

Model:
    Bases + mro:
        (<class 'pydantic.main.BaseModel'>, <class 'pydantic.utils.Representation'>, <class 'object'>)

    Namespace:
{       '__annotations__': {       '_private': typing.Optional[str],
                                   'public': <class 'str'>},
        '__class_vars__': set(),
        '__config__': <class '__main__.Config'>,
        '__custom_root_type__': False,
        '__fields__': {       'public': ModelField(name='public', type=str, required=False, default='default')},
        '__hash__': None,
        '__json_encoder__': <staticmethod object at 0x7f9744f29dc0>,
        '__module__': '__main__',
        '__post_root_validators__': [],
        '__pre_root_validators__': [],
        '__private_attributes__': {'_private': ModelPrivateAttr()},
        '__qualname__': 'Model',
        '__schema_cache__': {},
        '__slots__': {'_private'},
        '__validators__': {}}

NewModel:
    Bases + mro:
        (<class '__main__.ModelMixin'>, <class 'pydantic.main.BaseModel'>, <class 'pydantic.utils.Representation'>, <class 'object'>)
        (<class '__main__.Model'>, <class 'pydantic.main.BaseModel'>, <class 'pydantic.utils.Representation'>, <class 'object'>)

    Namespace:
{       '__class_vars__': set(),
        '__config__': <class '__main__.Config'>,
        '__custom_root_type__': False,
        '__fields__': {       'public': ModelField(name='public', type=str, required=False, default='default')},
        '__hash__': None,
        '__json_encoder__': <staticmethod object at 0x7f9744f29d30>,
        '__module__': '__main__',
        '__post_root_validators__': [],
        '__pre_root_validators__': [],
        '__private_attributes__': {       '_mixin_private': ModelPrivateAttr(),
                                          '_private': ModelPrivateAttr()},
        '__qualname__': 'NewModel',
        '__schema_cache__': {},
        '__slots__': {'_mixin_private', '_private'},
        '__validators__': {}}

```
`ModelMixin` has `'__slots__': {'_mixin_private'}` and `Model` has `'__slots__': {'_private'}`. The child class that derives from both, seems to have slots merged, but I am afraid that this is not enough to convince python interpreter to work. I think that slots should be removed from base classes before calling the `type.__new__()` otherwise it will still fail.

I will post update when I will have something new.
Okay, I can confirm that this is a problem with `__slot__` and multiple inheritance. There is several solutions, but actualy only one that I personally prefer. The main issue is that **"You cant have conflicting slot definitions on multiple inheritance"**. It is just a dogma at this time.

### 1. Create subclasses of all bases on the fly with slots without private attributes.

This is the most sophisticated solution. Subclassing is required in my opinion. You can't just modify user defined classes directly (at least not in such brutal way). Without subclassing, user defined classes sometimes will have slots with private attrs, sometimes they wont (i.e. they wont always when subclass is used as a base class). 

But subclassing on the fly is even worse. You are introducing highly implicit, not always transparent brick in whole user MRO. It is highly confusing and disgusting solution. In my opinion, it is absolutly anti-pythonic way of doing things, breaking many rules of the zen of python (just as not subclassing).

### 2. Do not create slots at all in pydantic private attrs.

It is okay solution, as long as You do not care about performance and development quality. This minor case of mixing in private attributes would then impact all other pydantic infrastructure. For me, it is step back for a project.  This is still better than 1., but worse than just stating in documentation that mixing in many models with private attributes is just not supported.

### 3. Be explicit: allow user to control slot inserts on any model in config.

For me, it is the best solution. It does not impact usage of pydantic in any way, as long as You state Your demands explicit. It also satisfies the zen of python `Explicit is better than implicit`. It also does not impact runtime performance, it just adds very minor overhead at a class creation time. If user knows that some model is just a mixin and will not be used alone, it **does not** introduce **any** disadvantages. If user wants to use mixin model alone (well, then is should not be named mixin), and still be able to derive from such model, he still can do that, with the cost of using `__dict__` attribute storage instead of `__slot__` (but only when model is used alone). When model is mixed in, the slots are created just normal. Config attribute that controls the behavior of slot inserts should not be inherited from base models (this should be stated directly in docs to inform an user). This way the deriving models will still create slots for all derived private attrs. Not inheriting some very specific config attributes should be okay, since `inherit_config` already is doing some magic merging on  `json_encoders` attribute.
    

Number 3. is the best solution in my opinion. I have already played with it and it seems to work just fine. I have to run pydantic test suite however, just to be sure that it does not brake anything. I can submit PR if You give me some time. It will include:
     
   - New config attribute (e.g. `exclude_private_from_slots`)
   - Special handling of this attribute in `inherit_config`
   - Conditional rendering of the `__slots__` in `ModelMetaclass.__new__`
   - Additional test cases covering this issue
   - Documentation on:
     - New config attribute
     - `PrivateAttr` section update, probably with example

I am not a kind of `mkdocs` guy, never touched it before, so it can take me some time. But please @samuelcolvin review this proposal and tell me if I can work on number 3. solution. If not, tell me how You imagine to solve this issue, and I will try to cover Your needs.
  
The test suite runs flawlessly (however master HEAD does not have 100% DC at this time). I have played with this fix in our project, where we are doing some intensive and serious things (including playing with pydantic metaclasses etc.), and I recognized one minor problem to address (actually two, but one is unrelated to this issue, I will open another issue for it). The problem was with `BaseModel.__repr_args__`. It's implementation was assuming that *all keys present in a* `__dict__` *should also be present in a* `__fields__`, which is not true after implementing *no 3.* fix. So the solution that I am proposing will also include `BaseModel.__repr_args__` modification to guard against `KeyError`. 

Waiting for attention, cheers.

@samuelcolvin Does anybody bothers to reply in this issue? Two months has passed and still no response. As I said, I will submit the PR, but I need to have an agreement on proposed solution. By the time the issue was investigated, I've implemented the feature that fixes it (in my forked branch), and it is working. The only things left to do is to write documentation on new config attribute that this fix introduces. I will write it but actually do not want to do the job that will be rejected, so please, give some feedback.
It seems to have been fixed in https://github.com/samuelcolvin/pydantic/pull/2989
This issue is not fixed for me in [#2989](https://github.com/samuelcolvin/pydantic/pull/2989) . According to the [changelogs](https://pydantic-docs.helpmanual.io/changelog/) #2989 should be included in v1.9.0a1, but the same bug appears when I run v1.9.0a1, v1.9.0 and v1.9.1. @samuelcolvin @PrettyWood @hmvp 
Just chiming in because this issue affects me and possibly other users who want to build models using the mixin pattern.
me too
me too
We often sort issue by upvote to decide priority, I also actively ignore issues with "me too" comments.

I strongly suggest therefore, that you use the built in feature to register your desire that this gets tackled, not comments.

Commit message: Use __pydantic_private__ for storing private attributes

Patch to review:
diff --git a/pydantic/_internal/_model_construction.py b/pydantic/_internal/_model_construction.py
index 5111bf2daf3..72a31908b8e 100644
--- a/pydantic/_internal/_model_construction.py
+++ b/pydantic/_internal/_model_construction.py
@@ -61,7 +61,7 @@ def __setitem__(self, k: str, v: object) -> None:
 
 @dataclass_transform(kw_only_default=True, field_specifiers=(Field,))
 class ModelMetaclass(ABCMeta):
-    def __new__(
+    def __new__(  # noqa: C901
         mcs,
         cls_name: str,
         bases: tuple[type[Any], ...],
@@ -95,9 +95,6 @@ def __new__(
                 namespace, config_wrapper.ignored_types, class_vars, base_field_names
             )
             if private_attributes:
-                slots: set[str] = set(namespace.get('__slots__', ()))
-                namespace['__slots__'] = slots | private_attributes.keys()
-
                 if 'model_post_init' in namespace:
                     # if there are private_attributes and a model_post_init function, we handle both
                     original_model_post_init = namespace['model_post_init']
@@ -116,8 +113,10 @@ def wrapped_model_post_init(self: BaseModel, __context: Any) -> None:
             namespace['__class_vars__'] = class_vars
             namespace['__private_attributes__'] = {**base_private_attributes, **private_attributes}
 
-            if config_wrapper.extra == 'allow':
-                namespace['__getattr__'] = model_extra_getattr
+            if config_wrapper.extra == 'allow' or namespace['__private_attributes__']:
+                namespace['__getattr__'] = model_extra_private_getattr
+            if namespace['__private_attributes__']:
+                namespace['__delattr__'] = model_private_delattr
 
             if '__hash__' not in namespace and config_wrapper.frozen:
 
@@ -192,6 +191,15 @@ def hash_func(self: Any) -> int:
             # this is the BaseModel class itself being created, no logic required
             return super().__new__(mcs, cls_name, bases, namespace, **kwargs)
 
+    def __getattr__(self, item: str) -> Any:
+        """
+        This is necessary to keep attribute access working for class attribute access
+        """
+        private_attributes = self.__dict__.get('__private_attributes__')
+        if private_attributes and item in private_attributes:
+            return private_attributes[item]
+        raise AttributeError(item)
+
     @classmethod
     def __prepare__(cls, *args: Any, **kwargs: Any) -> Mapping[str, object]:
         return _ModelNamespaceDict()
@@ -226,10 +234,12 @@ def init_private_attributes(self: BaseModel, __context: Any) -> None:
 
     It takes context as an argument since that's what pydantic-core passes when calling it.
     """
+    pydantic_private = {}
     for name, private_attr in self.__private_attributes__.items():
         default = private_attr.get_default()
         if default is not Undefined:
-            object_setattr(self, name, default)
+            pydantic_private[name] = default
+    object_setattr(self, '__pydantic_private__', pydantic_private)
 
 
 def inspect_namespace(  # noqa C901
@@ -505,11 +515,17 @@ def __getattr__(self, item: str) -> None:
         raise PydanticUserError(self._error_message, code=self._code)
 
 
-def model_extra_getattr(self: BaseModel, item: str) -> Any:
+def model_extra_private_getattr(self: BaseModel, item: str) -> Any:
     """
     This function is used to retrieve unrecognized attribute values from BaseModel subclasses which
-    allow (and store) extra
+    allow (and store) extra and/or private attributes
     """
+    if item in self.__private_attributes__:
+        try:
+            # Note: self.__pydantic_private__ is guaranteed to not be None if self.__private_attributes__ has items
+            return self.__pydantic_private__[item]  # type: ignore
+        except KeyError as exc:
+            raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}') from exc
     if self.__pydantic_extra__ is not None:
         try:
             return self.__pydantic_extra__[item]
@@ -517,3 +533,12 @@ def model_extra_getattr(self: BaseModel, item: str) -> Any:
             raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}') from exc
     else:
         raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}')
+
+
+def model_private_delattr(self: BaseModel, item: str) -> Any:
+    # Note: self.__pydantic_private__ is guaranteed to not be None if this method is set as __delattr__
+    assert self.__pydantic_private__ is not None
+    if item in self.__pydantic_private__:
+        del self.__pydantic_private__[item]
+    else:
+        raise AttributeError(f'{type(self).__name__!r} object has no attribute {item!r}')
diff --git a/pydantic/deprecated/copy_internals.py b/pydantic/deprecated/copy_internals.py
index a40d56ac2c6..3c8516657d7 100644
--- a/pydantic/deprecated/copy_internals.py
+++ b/pydantic/deprecated/copy_internals.py
@@ -12,7 +12,6 @@
     _typing_extra,
     _utils,
 )
-from .._internal._fields import Undefined
 
 if typing.TYPE_CHECKING:
     from .. import BaseModel
@@ -101,6 +100,7 @@ def _copy_and_set_values(
     values: dict[str, Any],
     fields_set: set[str],
     extra: dict[str, Any] | None = None,
+    private: dict[str, Any] | None = None,
     *,
     deep: bool,  # UP006
 ) -> Model:
@@ -108,18 +108,14 @@ def _copy_and_set_values(
         # chances of having empty dict here are quite low for using smart_deepcopy
         values = deepcopy(values)
         extra = deepcopy(extra)
+        private = deepcopy(private)
 
     cls = self.__class__
     m = cls.__new__(cls)
     _object_setattr(m, '__dict__', values)
     _object_setattr(m, '__pydantic_extra__', extra)
     _object_setattr(m, '__pydantic_fields_set__', fields_set)
-    for name in self.__private_attributes__:
-        value = getattr(self, name, Undefined)
-        if value is not Undefined:
-            if deep:
-                value = deepcopy(value)
-            _object_setattr(m, name, value)
+    _object_setattr(m, '__pydantic_private__', private)
 
     return m
 
diff --git a/pydantic/main.py b/pydantic/main.py
index 23d61880c14..d57a40799ce 100644
--- a/pydantic/main.py
+++ b/pydantic/main.py
@@ -98,6 +98,7 @@ class BaseModel(metaclass=_model_construction.ModelMetaclass):
         __class_vars__: typing.ClassVar[set[str]]
         __pydantic_fields_set__: set[str] = set()
         __pydantic_extra__: dict[str, Any] | None = None
+        __pydantic_private__: dict[str, Any] | None = None
         __pydantic_generic_metadata__: typing.ClassVar[_generics.PydanticGenericMetadata]
         __pydantic_parent_namespace__: typing.ClassVar[dict[str, Any] | None]
         __pydantic_custom_init__: typing.ClassVar[bool]
@@ -113,7 +114,7 @@ class BaseModel(metaclass=_model_construction.ModelMetaclass):
         )
 
     model_config = ConfigDict()
-    __slots__ = '__dict__', '__pydantic_fields_set__', '__pydantic_extra__'
+    __slots__ = '__dict__', '__pydantic_fields_set__', '__pydantic_extra__', '__pydantic_private__'
     __pydantic_complete__ = False
     __pydantic_root_model__: typing.ClassVar[bool] = False
 
@@ -313,7 +314,10 @@ def __setattr__(self, name: str, value: Any) -> None:
                 f'If you want to set a value on the class, use `{self.__class__.__name__}.{name} = value`.'
             )
         elif name.startswith('_'):
-            _object_setattr(self, name, value)
+            if self.__pydantic_private__ is None or name not in self.__private_attributes__:
+                _object_setattr(self, name, value)
+            else:
+                self.__pydantic_private__[name] = value
             return
         elif self.model_config.get('frozen', None):
             error: pydantic_core.InitErrorDetails = {
@@ -336,20 +340,21 @@ def __setattr__(self, name: str, value: Any) -> None:
             self.__pydantic_fields_set__.add(name)
 
     def __getstate__(self) -> dict[Any, Any]:
-        private_attrs = ((k, getattr(self, k, Undefined)) for k in self.__private_attributes__)
+        private = self.__pydantic_private__
+        if private:
+            private = {k: v for k, v in private.items() if v is not Undefined}
         return {
             '__dict__': self.__dict__,
             '__pydantic_extra__': self.__pydantic_extra__,
             '__pydantic_fields_set__': self.__pydantic_fields_set__,
-            '__private_attribute_values__': {k: v for k, v in private_attrs if v is not Undefined},
+            '__pydantic_private__': private,
         }
 
     def __setstate__(self, state: dict[Any, Any]) -> None:
         _object_setattr(self, '__dict__', state['__dict__'])
         _object_setattr(self, '__pydantic_fields_set__', state['__pydantic_fields_set__'])
         _object_setattr(self, '__pydantic_extra__', state['__pydantic_extra__'])
-        for name, value in state.get('__private_attribute_values__', {}).items():
-            _object_setattr(self, name, value)
+        _object_setattr(self, '__pydantic_private__', state['__pydantic_private__'])
 
     def model_dump(
         self,
@@ -474,10 +479,7 @@ def model_construct(cls: type[Model], _fields_set: set[str] | None = None, **val
         if cls.model_config.get('extra') == 'allow':
             _extra = {}
             for k, v in values.items():
-                if k in cls.model_fields:
-                    fields_values[k] = v
-                else:
-                    _extra[k] = v
+                _extra[k] = v
         else:
             fields_values.update(values)
         _object_setattr(m, '__dict__', fields_values)
@@ -485,8 +487,14 @@ def model_construct(cls: type[Model], _fields_set: set[str] | None = None, **val
             _fields_set = set(values.keys())
         _object_setattr(m, '__pydantic_fields_set__', _fields_set)
         _object_setattr(m, '__pydantic_extra__', _extra)
+
         if cls.__pydantic_post_init__:
             m.model_post_init(None)
+        else:
+            # Note: if there are any private attributes, cls.__pydantic_post_init__ would exist
+            # Since it doesn't, that means that `__pydantic_private__` should be set to None
+            _object_setattr(m, '__pydantic_private__', None)
+
         return m
 
     @classmethod
@@ -599,21 +607,12 @@ def __eq__(self, other: Any) -> bool:
             self_type = self.__pydantic_generic_metadata__['origin'] or self.__class__
             other_type = other.__pydantic_generic_metadata__['origin'] or other.__class__
 
-            if self_type != other_type:
-                return False
-
-            if self.__dict__ != other.__dict__:
-                return False
-
-            if self.__pydantic_extra__ != other.__pydantic_extra__:
-                return False
-
-            # If the types and field values match, check for equality of private attributes
-            for k in self.__private_attributes__:
-                if getattr(self, k, Undefined) != getattr(other, k, Undefined):
-                    return False
-
-            return True
+            return (
+                self_type == other_type
+                and self.__dict__ == other.__dict__
+                and self.__pydantic_private__ == other.__pydantic_private__
+                and self.__pydantic_extra__ == other.__pydantic_extra__
+            )
         else:
             return NotImplemented  # delegate to the other item in the comparison
 
@@ -653,10 +652,14 @@ def __copy__(self: Model) -> Model:
         _object_setattr(m, '__dict__', copy(self.__dict__))
         _object_setattr(m, '__pydantic_extra__', copy(self.__pydantic_extra__))
         _object_setattr(m, '__pydantic_fields_set__', copy(self.__pydantic_fields_set__))
-        for name in self.__private_attributes__:
-            value = getattr(self, name, Undefined)
-            if value is not Undefined:
-                _object_setattr(m, name, value)
+
+        if self.__pydantic_private__ is None:
+            _object_setattr(m, '__pydantic_private__', None)
+        else:
+            _object_setattr(
+                m, '__pydantic_private__', {k: v for k, v in self.__pydantic_private__.items() if v is not Undefined}
+            )
+
         return m
 
     def __deepcopy__(self: Model, memo: dict[int, Any] | None = None) -> Model:
@@ -670,10 +673,16 @@ def __deepcopy__(self: Model, memo: dict[int, Any] | None = None) -> Model:
         # This next line doesn't need a deepcopy because __pydantic_fields_set__ is a set[str],
         # and attempting a deepcopy would be marginally slower.
         _object_setattr(m, '__pydantic_fields_set__', copy(self.__pydantic_fields_set__))
-        for name in self.__private_attributes__:
-            value = getattr(self, name, Undefined)
-            if value is not Undefined:
-                _object_setattr(m, name, deepcopy(value, memo=memo))
+
+        if self.__pydantic_private__ is None:
+            _object_setattr(m, '__pydantic_private__', None)
+        else:
+            _object_setattr(
+                m,
+                '__pydantic_private__',
+                deepcopy({k: v for k, v in self.__pydantic_private__.items() if v is not Undefined}, memo=memo),
+            )
+
         return m
 
     def __repr_args__(self) -> _repr.ReprArgs:
@@ -986,6 +995,11 @@ def copy(
             ),
             **(update or {}),
         )
+        if self.__pydantic_private__ is None:
+            private = None
+        else:
+            private = {k: v for k, v in self.__pydantic_private__.items() if v is not Undefined}
+
         if self.__pydantic_extra__ is None:
             extra: dict[str, Any] | None = None
         else:
@@ -1007,7 +1021,7 @@ def copy(
         if exclude:
             fields_set -= set(exclude)
 
-        return _deprecated_copy_internals._copy_and_set_values(self, values, fields_set, extra, deep=deep)
+        return _deprecated_copy_internals._copy_and_set_values(self, values, fields_set, extra, private, deep=deep)
 
     @classmethod
     @typing_extensions.deprecated('The `schema` method is deprecated; use `model_json_schema` instead.')
diff --git a/tests/test_config.py b/tests/test_config.py
index eb4921db0e4..ecd9f2c8cfd 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -180,7 +180,7 @@ class Model(BaseModel):
 
             model_config = ConfigDict(extra='allow')
 
-        assert Model.__slots__ == {'_foo'}
+        assert set(Model.__private_attributes__) == {'_foo'}
         m = Model(_foo='field')
         assert m._foo == 'private_attribute'
         assert m.__dict__ == {}
diff --git a/tests/test_private_attributes.py b/tests/test_private_attributes.py
index 2bf8e2e5ecf..55e4d688f77 100644
--- a/tests/test_private_attributes.py
+++ b/tests/test_private_attributes.py
@@ -1,5 +1,4 @@
 import functools
-import platform
 from typing import ClassVar, Generic, TypeVar
 
 import pytest
@@ -14,11 +13,11 @@ def test_private_attribute():
     class Model(BaseModel):
         _foo = PrivateAttr(default)
 
-    assert Model.__slots__ == {'_foo'}
-    if platform.python_implementation() == 'PyPy':
-        repr(Model._foo).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
+    assert set(Model.__private_attributes__) == {'_foo'}
+    # if platform.python_implementation() == 'PyPy':
+    #     repr(Model._foo).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
 
     m = Model()
     assert m._foo == default
@@ -54,11 +53,10 @@ def factory():
     class Model(BaseModel):
         _foo = PrivateAttr(default_factory=factory)
 
-    assert Model.__slots__ == {'_foo'}
-    if platform.python_implementation() == 'PyPy':
-        repr(Model._foo).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
+    # if platform.python_implementation() == 'PyPy':
+    #     repr(Model._foo).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
 
     assert Model.__private_attributes__ == {'_foo': PrivateAttr(default_factory=factory)}
 
@@ -80,11 +78,10 @@ class Model(BaseModel):
 
         _foo: str
 
-    assert Model.__slots__ == {'_foo'}
-    if platform.python_implementation() == 'PyPy':
-        repr(Model._foo).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
+    # if platform.python_implementation() == 'PyPy':
+    #     repr(Model._foo).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
     assert Model.__private_attributes__ == {'_foo': PrivateAttr(Undefined)}
     assert repr(Model.__doc__) == "'The best model'"
 
@@ -115,11 +112,10 @@ class Model(BaseModel):
         _foo: str = 'abc'
         _bar: ClassVar[str] = 'cba'
 
-    assert Model.__slots__ == {'_foo'}
-    if platform.python_implementation() == 'PyPy':
-        repr(Model._foo).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
+    # if platform.python_implementation() == 'PyPy':
+    #     repr(Model._foo).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'Model' objects>"
     assert Model._bar == 'cba'
     assert Model.__private_attributes__ == {'_foo': PrivateAttr('abc')}
 
@@ -144,7 +140,7 @@ class Model(BaseModel):
 
         model_config = ConfigDict(extra='allow')
 
-    assert Model.__slots__ == {'_foo'}
+    assert set(Model.__private_attributes__) == {'_foo'}
     m = Model(_foo='field')
     assert m._foo == 'private_attribute'
     assert m.__dict__ == {}
@@ -245,17 +241,21 @@ class ParentBModel(GrandParentModel):
     class Model(ParentAModel, ParentBModel):
         _baz = PrivateAttr(default)
 
-    assert GrandParentModel.__slots__ == {'_foo'}
-    assert ParentBModel.__slots__ == {'_bar'}
-    assert Model.__slots__ == {'_baz'}
-    if platform.python_implementation() == 'PyPy':
-        assert repr(Model._foo).startswith('<member_descriptor object at')
-        assert repr(Model._bar).startswith('<member_descriptor object at')
-        assert repr(Model._baz).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'GrandParentModel' objects>"
-        assert repr(Model._bar) == "<member '_bar' of 'ParentBModel' objects>"
-        assert repr(Model._baz) == "<member '_baz' of 'Model' objects>"
+    # if platform.python_implementation() == 'PyPy':
+    #     assert repr(Model._foo).startswith('<member_descriptor object at')
+    #     assert repr(Model._bar).startswith('<member_descriptor object at')
+    #     assert repr(Model._baz).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'GrandParentModel' objects>"
+    #     assert repr(Model._bar) == "<member '_bar' of 'ParentBModel' objects>"
+    #     assert repr(Model._baz) == "<member '_baz' of 'Model' objects>"
+    assert GrandParentModel.__private_attributes__ == {
+        '_foo': PrivateAttr(default),
+    }
+    assert ParentBModel.__private_attributes__ == {
+        '_foo': PrivateAttr(default),
+        '_bar': PrivateAttr(default),
+    }
     assert Model.__private_attributes__ == {
         '_foo': PrivateAttr(default),
         '_bar': PrivateAttr(default),
@@ -339,3 +339,28 @@ class A(BaseModel):
     a = A()
     a._x = None
     assert a._x is None
+
+
+def test_layout_compatible_multiple_private_parents():
+    import typing as t
+
+    import pydantic
+
+    class ModelMixin(pydantic.BaseModel):
+        _mixin_private: t.Optional[str] = pydantic.PrivateAttr(None)
+
+    class Model(pydantic.BaseModel):
+        public: str = 'default'
+        _private: t.Optional[str] = pydantic.PrivateAttr(None)
+
+    class NewModel(ModelMixin, Model):
+        pass
+
+    assert set(NewModel.__private_attributes__) == {'_mixin_private', '_private'}
+    m = NewModel()
+    m._mixin_private = 1
+    m._private = 2
+
+    assert m.__pydantic_private__ == {'_mixin_private': 1, '_private': 2}
+    assert m._mixin_private == 1
+    assert m._private == 2


Code review comments:
diff_hunk: @@ -245,17 +241,21 @@ class ParentBModel(GrandParentModel):
     class Model(ParentAModel, ParentBModel):
         _baz = PrivateAttr(default)
 
-    assert GrandParentModel.__slots__ == {'_foo'}
-    assert ParentBModel.__slots__ == {'_bar'}
-    assert Model.__slots__ == {'_baz'}
-    if platform.python_implementation() == 'PyPy':
-        assert repr(Model._foo).startswith('<member_descriptor object at')
-        assert repr(Model._bar).startswith('<member_descriptor object at')
-        assert repr(Model._baz).startswith('<member_descriptor object at')
-    else:
-        assert repr(Model._foo) == "<member '_foo' of 'GrandParentModel' objects>"
-        assert repr(Model._bar) == "<member '_bar' of 'ParentBModel' objects>"
-        assert repr(Model._baz) == "<member '_baz' of 'Model' objects>"
+    # if platform.python_implementation() == 'PyPy':
+    #     assert repr(Model._foo).startswith('<member_descriptor object at')
+    #     assert repr(Model._bar).startswith('<member_descriptor object at')
+    #     assert repr(Model._baz).startswith('<member_descriptor object at')
+    # else:
+    #     assert repr(Model._foo) == "<member '_foo' of 'GrandParentModel' objects>"
+    #     assert repr(Model._bar) == "<member '_bar' of 'ParentBModel' objects>"
+    #     assert repr(Model._baz) == "<member '_baz' of 'Model' objects>"
path: tests/test_private_attributes.py
text: @author:
With this change, instead of `"<member '_foo' of 'GrandParentModel' objects>"` you get `"ModelPrivateAttr(default={'a': {}})"`.

This seems to me to be the biggest potential "issue" remaining with this change.

As far as I can tell, these checks of `repr` aren't really checking any deeper behavior, but I don't know if this is likely reflective of deeper issues caused by the fact that `Model._foo` would no longer be a member object.

I'm thinking _maybe_ there's something we could do in the `ModelMetaclass.__getattr__` or similar but I'm in a bit over my head and this stuff is not super well documented.

But `tests.test_create_model.test_private_attr_set_name` and `tests.test_create_model.test_private_attr_set_name` are still passing which makes me hopeful that this is not having significant impacts on usage.