fix(commons): avoid silent add of __slots__ attrs to __dict__

[rmlibre]

Description

There's a bug in the __init__ of the Namespace class from the commons subpackage. The bug causes the silent addition of attributes that are declared in __slots__ to be added into an instance's __dict__.

This is the offending piece of code:

aiootp/aiootp/commons/namespaces.py

Lines 36 to 46 in 03b7753

	__slots__ = ("__dict__",)
	def __init__(
	self, mapping: t.Mapping[t.Hashable, t.Any] = {}, /, **kw: t.Any
	) -> None:
	"""
	Maps the user-defined mapping & kwargs to the Namespace's
	instance dictionary.
	"""
	self.__dict__.update(mapping) if mapping else 0
	self.__dict__.update(kw) if kw else 0

Here's a demonstration of the impact:

from aiootp.commons.namespaces import Namespace

class HybridNamespace(Namespace):
    __slots__ = ("attr_0", "attr_1")

print(hybrid := HybridNamespace(attr_0=0))
# HybridNamespace()

print(hybrid.attr_0)
# AttributeError: attr_0  # Ok???

print(hybrid.__dict__)
# {'attr_0': 0}   # <-- this is weird!

hybrid.attr_0 = True
print(hybrid.attr_0)
# True

hybrid.attr_1 = 1
print(hybrid.attr_1)
# 1

print(hybrid.__dict__)
# {'attr_0': 0}

# DANGER ZONE:
hybrid.__dict__["attr_2"] = 2
print(hybrid.attr_2)
# 2

print(hybrid.__dict__)
# {'attr_0': 0, 'attr_2': 2}

This is bizarre behavior. However, this trickiness is expected when mixing __slots__ & __dict__.

Expected behavior

If an attribute is defined in its or its subclass' __slots__, then the __init__, & other public interfaces for setting attributes, won't add it to the instance's __dict__. Direct usage of __dict__ should be highly discouraged, as it can lead to this unstable behavior.

This is what the results should have looked like:

from aiootp.commons.namespaces import Namespace

class HybridNamespace(Namespace):
    __slots__ = ("attr_0", "attr_1")

print(hybrid := HybridNamespace(attr_0=0))
# HybridNamespace(
#     attr_0=<omitted><class 'int'>,
# )

print(hybrid.attr_0)
# 0

print(hybrid.__dict__)
# {}

hybrid.attr_0 = True
print(hybrid.attr_0)
# True

hybrid.attr_1 = 1
print(hybrid.attr_1)
# 1

print(hybrid.__dict__)
# {}

# Do Not Specify __dict__:
hybrid.attr_2 = 2
print(hybrid.attr_2)
# 2

print(hybrid.__dict__)
# {'attr_2': 2}

Remediations

• Rely on the __init__ from the Slots superclass, which correctly sets attributes by using the __getitem__ interface.

Slots.__init__(...):

aiootp/aiootp/commons/slots.py

Lines 74 to 75 in 03b7753

	for name, value in {**dict(mapping), **kw}.items(): # flexible. subclasses
	self[name] = value # should prefer specific

The init defers to getitem...

Slots.__getitem__(...):

aiootp/aiootp/commons/slots.py

Lines 133 to 136 in 03b7753

	if name.__class__ is str:
	object.__setattr__(self, name, value)
	else:
	self.__dict__[name] = value

While getitem only sets directly to the dictionary when the name is not a str, which ensures it cannot have been defined in the slots.

• Fix internal code in the packaging module which needs to set values directly into the __dict__ of remade modules.

The code the was able to remain the same, & avoid using discouraged interfaces, in lieu of improved logic in the Slots initializer:

aiootp/aiootp/commons/slots.py

Lines 78 to 83 in 494dc82

	slots = self._slots_set # flexible init. not great
	for name, value in {**dict(mapping), **kw}.items(): # performance. subclasses
	if name in slots: # should prefer specificity
	object.__setattr__(self, name, value) # ie. self.a = a
	else: # self.b = b
	self.__dict__[name] = value # ...

• Add tests to detect this bug.

This was able to detect the bug in a spread of classes:

aiootp/tests/test_namespace_classes.py

Lines 217 to 244 in 494dc82

	try:
	class MisusedSubclass(self._type):
	__slots__ = ("attr_0", "attr_1", "__dict__")
	slots_types = dict(attr_0=int, attr_1=int)
	except TypeError:
	class MisusedSubclass(self._type):
	__slots__ = ("attr_0", "attr_1")
	slots_types = dict(attr_0=int, attr_1=int)
	bad_obj = MisusedSubclass(attr_0=0, attr_1=1)
	assert "attr_0" not in bad_obj.__dict__
	assert "attr_1" not in bad_obj.__dict__
	assert 0 == bad_obj.attr_0
	assert 1 == bad_obj.attr_1
	bad_obj = MisusedSubclass()
	bad_obj.__dict__.update(dict(attr_0=0, attr_1=1))
	assert "attr_0" in bad_obj.__dict__
	assert "attr_1" in bad_obj.__dict__
	assert not hasattr(bad_obj, "attr_0")
	assert not hasattr(bad_obj, "attr_1")
	problem = (
	"Very strange __dict__ / __slots__ interplay didn't manifest?"
	)
	with Ignore(AttributeError, if_else=violation(problem)):
	bad_obj.attr_0
	with Ignore(AttributeError, if_else=violation(problem)):
	bad_obj.attr_1

• Add a warning against directly adding to instance dictionaries.

I think there should be a better warning against slot / dict misuse. Just unsure how / where

aiootp/aiootp/commons/slots.py

Lines 72 to 76 in 494dc82

	Defining classes with slots improves a system's memory efficiency,
	especially when many instances are created. Having an instance
	dictionary offers flexibility, though the interplay with slots
	can cause problems. This initializer avoids setting the names
	declared in `__slots__` within the a potential instance dict.

Validity of assignment styles:

✔ hybrid.attr = "value"                # valid
✔ hybrid["attr"] = "value"             # valid
✔ setattr(hybrid, "attr", "value")     # valid

🚫 hybrid.__dict__["attr"] = "value"    # invalid

---

Commentary

These remediations may break code which depends on the bug. However, a fix is required. Let this PR serve as documentation & place of discussion around alternative or additional remediations.