strict attribute setting issue (#1092)

package/CHANGELOG

@@ -30,6 +30,8 @@ Enhancements
   * added support for Tinker TXYZ and ARC files
   * libmdaxdr and libdcd classes can now be pickled (PR #1680)
   * speed up atomgroup.rotate when point = (0, 0, 0) (Issue #1707)
+  * Universe.add_TopologyAttr now accepts strings to add a given attribute
+    to the Universe (Issue #1092, PR #1186)
 
 Deprecations
 
@@ -81,6 +83,8 @@ Changes
     tuple (filename_or_array, trajectory_type) (Issue #1613)
   * docs: URLs to (www|docs).mdanalysis.org now link to SSL-encrypted site
     (see issue MDAnalysis/MDAnalysis.github.io#61)
+  * attributes can not be assigned to AtomGroups (and similar objects) unless
+    they are part of the Universe topology (Issue #1092 PR #1186)
 
 
 06/29/17 richardjgowers, rathann, jbarnoud, orbeckst, utkbansal
@@ -965,7 +969,7 @@ Fixes
 
 04/01/14  orbeckst, jandom, zhuyi.xue, xdeupi, tyler.je.reddy,
           manuel.nuno.melo, danny.parton, sebastien.buchoux, denniej0,
-	  rmcgibbo, richardjgowers, lennardvanderfeltz, bernardin.alejandro
+    rmcgibbo, richardjgowers, lennardvanderfeltz, bernardin.alejandro
           matthieu.chavent
 
   * 0.8.1
@@ -1480,7 +1484,7 @@ Fixes
     alternative atoms and insertion codes that are not needed for
     handling MD simulation data).
       - The default behaviour of MDAnalysis  can be set through the flag
-	MDAnalysis.core.flag['permissive_pdb_reader']. The default is True.
+  MDAnalysis.core.flag['permissive_pdb_reader']. The default is True.
       - One can always manually select the PDB reader by providing the
         permissive keyword to Universe; e.g. Universe(...,permissive=False)
         will read the input file with the Bio.PDB reader. This might be

package/MDAnalysis/__init__.py

@@ -164,6 +164,8 @@
 _MULTIFRAME_WRITERS = {}
 _PARSERS = {}
 _SELECTION_WRITERS = {}
+# Registry of TopologyAttributes
+_TOPOLOGY_ATTRS = {}
 
 # Storing anchor universes for unpickling groups
 import weakref

package/MDAnalysis/core/groups.py

@@ -153,15 +153,14 @@ def make_classes():
 
     # The 'GBase' middle man is needed so that a single topologyattr
     #  patching applies automatically to all groups.
-    GBase = bases[GroupBase] = _TopologyAttrContainer._subclass()
+    GBase = bases[GroupBase] = _TopologyAttrContainer._subclass(is_group=True)
     for cls in groups:
-        bases[cls] = GBase._subclass()
-
-    # In the current Group-centered topology scheme no attributes apply only
-    #  to ComponentBase, so no need to have a 'CB' middle man.
-    #CBase = _TopologyAttrContainer(singular=True)
+        bases[cls] = GBase._subclass(is_group=True)
+    # CBase for patching all components
+    CBase = bases[ComponentBase] = _TopologyAttrContainer._subclass(
+        is_group=False)
     for cls in components:
-        bases[cls] = _TopologyAttrContainer._subclass(singular=True)
+        bases[cls] = CBase._subclass(is_group=False)
 
     # Initializes the class cache.
     for cls in groups + components:
@@ -184,27 +183,37 @@ class _TopologyAttrContainer(object):
       The mixed subclasses become the final container classes specific to each
       :class:`Universe`.
     """
-    _singular = False
-
     @classmethod
-    def _subclass(cls, singular=None):
+    def _subclass(cls, is_group):
         """Factory method returning :class:`_TopologyAttrContainer` subclasses.
 
         Parameters
         ----------
-        singular : bool
-            The :attr:`_singular` of the returned class will be set to
-            *singular*. It controls the type of :class:`TopologyAttr` addition.
+        is_group : bool
+            The :attr:`_is_group` of the returned class will be set to
+            *is_group*. This is used to distinguish between Groups (AtomGroup
+            etc.) and Components (Atom etc.) in internal methods when
+            considering actions such as addition between objects, adding
+            TopologyAttributes to them.
 
         Returns
         -------
         type
             A subclass of :class:`_TopologyAttrContainer`, with the same name.
         """
-        if singular is not None:
-            return type(cls.__name__, (cls,), {'_singular': bool(singular)})
+        newcls = type(cls.__name__, (cls,), {'_is_group': bool(is_group)})
+        if is_group:
+            newcls._SETATTR_WHITELIST = {
+                'positions', 'velocities', 'forces', 'dimensions',
+                'atoms', 'residue', 'residues', 'segment', 'segments',
+            }
         else:
-            return type(cls.__name__, (cls,), {})
+            newcls._SETATTR_WHITELIST = {
+                'position', 'velocity', 'force', 'dimensions',
+                'atoms', 'residue', 'residues', 'segment',
+            }
+
+        return newcls
 
     @classmethod
     def _mix(cls, other):
@@ -249,12 +258,26 @@ def getter(self):
         def setter(self, values):
             return attr.__setitem__(self, values)
 
-        if cls._singular:
-            setattr(cls, attr.singular,
-                    property(getter, setter, None, attr.singledoc))
-        else:
+        if cls._is_group:
             setattr(cls, attr.attrname,
                     property(getter, setter, None, attr.groupdoc))
+            cls._SETATTR_WHITELIST.add(attr.attrname)
+        else:
+            setattr(cls, attr.singular,
+                    property(getter, setter, None, attr.singledoc))
+            cls._SETATTR_WHITELIST.add(attr.singular)
+
+    def __setattr__(self, attr, value):
+        # `ag.this = 42` calls setattr(ag, 'this', 42)
+        if not (attr.startswith('_') or  # 'private' allowed
+                attr in self._SETATTR_WHITELIST or  # known attributes allowed
+                hasattr(self, attr)):  # preexisting (eg properties) allowed
+            raise AttributeError(
+                "Cannot set arbitrary attributes to a {}".format(
+                    'Group' if self._is_group else 'Component'))
+        # if it is, we allow the setattr to proceed by deferring to the super
+        # behaviour (ie do it)
+        super(_TopologyAttrContainer, self).__setattr__(attr, value)
 
 
 class _MutableBase(object):
@@ -429,7 +452,7 @@ def __hash__(self):
         return hash((self._u, self.__class__, tuple(self.ix.tolist())))
 
     def __len__(self):
-        return len(self._ix)
+        return len(self.ix)
 
     def __getitem__(self, item):
         # supports
@@ -2780,7 +2803,7 @@ def __init__(self, base_group, selections, strings):
         # its check, no self.attribute access can be made before this line
         self._u = base_group.universe
         self._selections = selections
-        self.selection_strings = strings
+        self._selection_strings = strings
         self._base_group = base_group
         self._lastupdate = None
         self._derived_class = base_group._derived_class
@@ -2857,8 +2880,9 @@ def _ensure_updated(self):
 
     def __getattribute__(self, name):
         # ALL attribute access goes through here
-        # If the requested attribute isn't in the shortcut list, update ourselves
-        if not name in _UAG_SHORTCUT_ATTRS:
+        # If the requested attribute is public (not starting with '_') and
+        # isn't in the shortcut list, update ourselves
+        if not (name.startswith('_') or name in _UAG_SHORTCUT_ATTRS):
             self._ensure_updated()
         # Going via object.__getattribute__ then bypasses this check stage
         return object.__getattribute__(self, name)
@@ -2869,13 +2893,13 @@ def __reduce__(self):
         # - recreate UAG as created through select_atoms (basegroup and selstrs)
         # even if base_group is a UAG this will work through recursion
         return (_unpickle_uag,
-                (self._base_group.__reduce__(), self._selections, self.selection_strings))
+                (self._base_group.__reduce__(), self._selections, self._selection_strings))
 
     def __repr__(self):
         basestr = super(UpdatingAtomGroup, self).__repr__()
-        if not self.selection_strings:
+        if not self._selection_strings:
             return basestr
-        sels = "'{}'".format("' + '".join(self.selection_strings))
+        sels = "'{}'".format("' + '".join(self._selection_strings))
         # Cheap comparison. Might fail for corner cases but this is
         # mostly cosmetic.
         if self._base_group is self.universe.atoms:
@@ -2884,7 +2908,7 @@ def __repr__(self):
             basegrp = "another AtomGroup."
         # With a shorthand to conditionally append the 's' in 'selections'.
         return "{}, with selection{} {} on {}>".format(basestr[:-1],
-                    "s"[len(self.selection_strings)==1:], sels, basegrp)
+                    "s"[len(self._selection_strings)==1:], sels, basegrp)
 
 # Define relationships between these classes
 # with Level objects