Unify NO_VALUE and NEVER_SET

There's no real difference between these two constants
except they are used in different places and therefore allow
various codepaths to work largely by accident.   These
codepaths should be explicit.   Assign NO_VALUE and NEVER_SET
to the same constant and work towards having just one constant
for "we have no value to return right now".

Fixes: #4696
Change-Id: I7c324967952c1886bf202074d627323a2ad013cc

6 files changed, 78 insertions, 74 deletions

diff --git a/doc/build/changelog/unreleased_14/4696.rst b/doc/build/changelog/unreleased_14/4696.rst
new file mode 100644
index 000000000..c4629db36
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/4696.rst
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4696
+
+    The internal attribute symbols NO_VALUE and NEVER_SET have been unified, as
+    there was no meaningful difference between these two symbols, other than a
+    few codepaths where they were differentiated in subtle and undocumented
+    ways, these have been fixed.
+
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 01f19d991..321ab7d6f 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -28,7 +28,7 @@ from .base import instance_state
 from .base import instance_str
 from .base import LOAD_AGAINST_COMMITTED
 from .base import manager_of_class
-from .base import NEVER_SET
+from .base import NEVER_SET  # noqa
 from .base import NO_AUTOFLUSH
 from .base import NO_CHANGE  # noqa
 from .base import NO_RAISE
@@ -40,7 +40,7 @@ from .base import PASSIVE_NO_INITIALIZE
 from .base import PASSIVE_NO_RESULT
 from .base import PASSIVE_OFF
 from .base import PASSIVE_ONLY_PERSISTENT
-from .base import PASSIVE_RETURN_NEVER_SET
+from .base import PASSIVE_RETURN_NO_VALUE
 from .base import RELATED_OBJECT_OK  # noqa
 from .base import SQL_OK  # noqa
 from .base import state_str
@@ -677,7 +677,7 @@ class AttributeImpl(object):
             key = self.key
             if (
                 key not in state.committed_state
-                or state.committed_state[key] is NEVER_SET
+                or state.committed_state[key] is NO_VALUE
             ):
                 if not passive & CALLABLES_OK:
                     return PASSIVE_NO_RESULT
@@ -692,7 +692,7 @@ class AttributeImpl(object):
                 else:
                     value = ATTR_EMPTY
 
-                if value is PASSIVE_NO_RESULT or value is NEVER_SET:
+                if value is PASSIVE_NO_RESULT or value is NO_VALUE:
                     return value
                 elif value is ATTR_WAS_SET:
                     try:
@@ -708,7 +708,7 @@ class AttributeImpl(object):
                     return self.set_committed_value(state, dict_, value)
 
             if not passive & INIT_OK:
-                return NEVER_SET
+                return NO_VALUE
             else:
                 # Return a new, empty value
                 return self.initialize(state, dict_)
@@ -749,7 +749,7 @@ class AttributeImpl(object):
 
         if self.key in state.committed_state:
             value = state.committed_state[self.key]
-            if value in (NO_VALUE, NEVER_SET):
+            if value is NO_VALUE:
                 return None
             else:
                 return value
@@ -782,7 +782,7 @@ class ScalarAttributeImpl(AttributeImpl):
 
     def delete(self, state, dict_):
         if self.dispatch._active_history:
-            old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET)
+            old = self.get(state, dict_, PASSIVE_RETURN_NO_VALUE)
         else:
             old = dict_.get(self.key, NO_VALUE)
 
@@ -802,6 +802,8 @@ class ScalarAttributeImpl(AttributeImpl):
     def get_history(self, state, dict_, passive=PASSIVE_OFF):
         if self.key in dict_:
             return History.from_scalar_attribute(self, state, dict_[self.key])
+        elif self.key in state.committed_state:
+            return History.from_scalar_attribute(self, state, NO_VALUE)
         else:
             if passive & INIT_OK:
                 passive ^= INIT_OK
@@ -822,7 +824,7 @@ class ScalarAttributeImpl(AttributeImpl):
         pop=False,
     ):
         if self.dispatch._active_history:
-            old = self.get(state, dict_, PASSIVE_RETURN_NEVER_SET)
+            old = self.get(state, dict_, PASSIVE_RETURN_NO_VALUE)
         else:
             old = dict_.get(self.key, NO_VALUE)
 
@@ -920,7 +922,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
         if (
             current is not None
             and current is not PASSIVE_NO_RESULT
-            and current is not NEVER_SET
+            and current is not NO_VALUE
         ):
             ret = [(instance_state(current), current)]
         else:
@@ -931,7 +933,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
             if (
                 original is not None
                 and original is not PASSIVE_NO_RESULT
-                and original is not NEVER_SET
+                and original is not NO_VALUE
                 and original is not current
             ):
 
@@ -998,7 +1000,7 @@ class ScalarObjectAttributeImpl(ScalarAttributeImpl):
             if previous is not value and previous not in (
                 None,
                 PASSIVE_NO_RESULT,
-                NEVER_SET,
+                NO_VALUE,
             ):
                 self.sethasparent(instance_state(previous), state, False)
 
@@ -1109,7 +1111,7 @@ class CollectionAttributeImpl(AttributeImpl):
 
         if self.key in state.committed_state:
             original = state.committed_state[self.key]
-            if original not in (NO_VALUE, NEVER_SET):
+            if original is not NO_VALUE:
                 current_states = [
                     ((c is not None) and instance_state(c) or None, c)
                     for c in current
@@ -1142,7 +1144,7 @@ class CollectionAttributeImpl(AttributeImpl):
         for fn in self.dispatch.append:
             value = fn(state, value, initiator or self._append_token)
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
         if self.trackparent and value is not None:
             self.sethasparent(instance_state(value), state, True)
@@ -1158,7 +1160,7 @@ class CollectionAttributeImpl(AttributeImpl):
         operations (even though set.pop is the one where it is really needed).
 
         """
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
     def fire_remove_event(self, state, dict_, value, initiator):
         if self.trackparent and value is not None:
@@ -1167,13 +1169,13 @@ class CollectionAttributeImpl(AttributeImpl):
         for fn in self.dispatch.remove:
             fn(state, value, initiator or self._remove_token)
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
     def delete(self, state, dict_):
         if self.key not in dict_:
             return
 
-        state._modified_event(dict_, self, NEVER_SET, True)
+        state._modified_event(dict_, self, NO_VALUE, True)
 
         collection = self.get_collection(state, state.dict)
         collection.clear_with_event()
@@ -1386,7 +1388,7 @@ def backref_listeners(attribute, key, uselist):
         if (
             oldchild is not None
             and oldchild is not PASSIVE_NO_RESULT
-            and oldchild is not NEVER_SET
+            and oldchild is not NO_VALUE
         ):
             # With lazy=None, there's no guarantee that the full collection is
             # present when updating via a backref.
@@ -1481,7 +1483,7 @@ def backref_listeners(attribute, key, uselist):
         if (
             child is not None
             and child is not PASSIVE_NO_RESULT
-            and child is not NEVER_SET
+            and child is not NO_VALUE
         ):
             child_state, child_dict = (
                 instance_state(child),
@@ -1549,9 +1551,7 @@ def backref_listeners(attribute, key, uselist):
 
 
 _NO_HISTORY = util.symbol("NO_HISTORY")
-_NO_STATE_SYMBOLS = frozenset(
-    [id(PASSIVE_NO_RESULT), id(NO_VALUE), id(NEVER_SET)]
-)
+_NO_STATE_SYMBOLS = frozenset([id(PASSIVE_NO_RESULT), id(NO_VALUE)])
 
 History = util.namedtuple("History", ["added", "unchanged", "deleted"])
 
@@ -1637,12 +1637,15 @@ class History(History):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
         if original is _NO_HISTORY:
-            if current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), ())
             else:
                 return cls((), [current], ())
         # don't let ClauseElement expressions here trip things up
-        elif attribute.is_equal(current, original) is True:
+        elif (
+            current is not NO_VALUE
+            and attribute.is_equal(current, original) is True
+        ):
             return cls((), [current], ())
         else:
             # current convention on native scalars is to not
@@ -1658,7 +1661,7 @@ class History(History):
                     current = None
             else:
                 deleted = [original]
-            if current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), deleted)
             else:
                 return cls([current], (), deleted)
@@ -1668,11 +1671,11 @@ class History(History):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
         if original is _NO_HISTORY:
-            if current is NO_VALUE or current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), ())
             else:
                 return cls((), [current], ())
-        elif current is original and current is not NEVER_SET:
+        elif current is original and current is not NO_VALUE:
             return cls((), [current], ())
         else:
             # current convention on related objects is to not
@@ -1688,7 +1691,7 @@ class History(History):
                     current = None
             else:
                 deleted = [original]
-            if current is NO_VALUE or current is NEVER_SET:
+            if current is NO_VALUE:
                 return cls((), (), deleted)
             else:
                 return cls([current], (), deleted)
@@ -1697,11 +1700,11 @@ class History(History):
     def from_collection(cls, attribute, state, current):
         original = state.committed_state.get(attribute.key, _NO_HISTORY)
 
-        if current is NO_VALUE or current is NEVER_SET:
+        if current is NO_VALUE:
             return cls((), (), ())
 
         current = getattr(current, "_sa_adapter")
-        if original in (NO_VALUE, NEVER_SET):
+        if original is NO_VALUE:
             return cls(list(current), (), ())
         elif original is _NO_HISTORY:
             return cls((), list(current), ())
diff --git a/lib/sqlalchemy/orm/base.py b/lib/sqlalchemy/orm/base.py
index f809d5891..4d308d26b 100644
--- a/lib/sqlalchemy/orm/base.py
+++ b/lib/sqlalchemy/orm/base.py
@@ -45,13 +45,12 @@ NO_VALUE = util.symbol(
     and flags indicated we were not to load it.
     """,
 )
+NEVER_SET = NO_VALUE
+"""
+Synonymous with NO_VALUE
 
-NEVER_SET = util.symbol(
-    "NEVER_SET",
-    """Symbol which may be placed as the 'previous' value of an attribute
-    indicating that the attribute had not been assigned to previously.
-    """,
-)
+.. versionchanged:: 1.4   NEVER_SET was merged with NO_VALUE
+"""
 
 NO_CHANGE = util.symbol(
     "NO_CHANGE",
@@ -126,15 +125,15 @@ PASSIVE_OFF = util.symbol(
         RELATED_OBJECT_OK | NON_PERSISTENT_OK | INIT_OK | CALLABLES_OK | SQL_OK
     ),
 )
-PASSIVE_RETURN_NEVER_SET = util.symbol(
-    "PASSIVE_RETURN_NEVER_SET",
+PASSIVE_RETURN_NO_VALUE = util.symbol(
+    "PASSIVE_RETURN_NO_VALUE",
     """PASSIVE_OFF ^ INIT_OK""",
     canonical=PASSIVE_OFF ^ INIT_OK,
 )
 PASSIVE_NO_INITIALIZE = util.symbol(
     "PASSIVE_NO_INITIALIZE",
-    "PASSIVE_RETURN_NEVER_SET ^ CALLABLES_OK",
-    canonical=PASSIVE_RETURN_NEVER_SET ^ CALLABLES_OK,
+    "PASSIVE_RETURN_NO_VALUE ^ CALLABLES_OK",
+    canonical=PASSIVE_RETURN_NO_VALUE ^ CALLABLES_OK,
 )
 PASSIVE_NO_FETCH = util.symbol(
     "PASSIVE_NO_FETCH", "PASSIVE_OFF ^ SQL_OK", canonical=PASSIVE_OFF ^ SQL_OK
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index ccf05a783..3c26f7247 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -2715,7 +2715,7 @@ class Mapper(InspectionAttr):
         return self._identity_key_from_state(state, attributes.PASSIVE_OFF)
 
     def _identity_key_from_state(
-        self, state, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
         dict_ = state.dict
         manager = state.manager
@@ -2769,7 +2769,7 @@ class Mapper(InspectionAttr):
         return {prop.key for prop in self._all_pk_props}
 
     def _get_state_attr_by_column(
-        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
         prop = self._columntoproperty[column]
         return state.manager[prop.key].impl.get(state, dict_, passive=passive)
@@ -2790,7 +2790,7 @@ class Mapper(InspectionAttr):
         )
 
     def _get_committed_state_attr_by_column(
-        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NEVER_SET
+        self, state, dict_, column, passive=attributes.PASSIVE_RETURN_NO_VALUE
     ):
 
         prop = self._columntoproperty[column]
diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index 17488f236..8d244c4fa 100644
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -1028,31 +1028,27 @@ class GetNoValueTest(fixtures.ORMTest):
             attributes.PASSIVE_NO_RESULT,
         )
 
-    def test_passive_no_result_never_set(self):
-        attr, state, dict_ = self._fixture(attributes.NEVER_SET)
+    def test_passive_no_result_no_value(self):
+        attr, state, dict_ = self._fixture(attributes.NO_VALUE)
         eq_(
             attr.get(state, dict_, passive=attributes.PASSIVE_NO_INITIALIZE),
             attributes.PASSIVE_NO_RESULT,
         )
         assert "attr" not in dict_
 
-    def test_passive_ret_never_set_never_set(self):
-        attr, state, dict_ = self._fixture(attributes.NEVER_SET)
+    def test_passive_ret_no_value(self):
+        attr, state, dict_ = self._fixture(attributes.NO_VALUE)
         eq_(
-            attr.get(
-                state, dict_, passive=attributes.PASSIVE_RETURN_NEVER_SET
-            ),
-            attributes.NEVER_SET,
+            attr.get(state, dict_, passive=attributes.PASSIVE_RETURN_NO_VALUE),
+            attributes.NO_VALUE,
         )
         assert "attr" not in dict_
 
-    def test_passive_ret_never_set_empty(self):
+    def test_passive_ret_no_value_empty(self):
         attr, state, dict_ = self._fixture(None)
         eq_(
-            attr.get(
-                state, dict_, passive=attributes.PASSIVE_RETURN_NEVER_SET
-            ),
-            attributes.NEVER_SET,
+            attr.get(state, dict_, passive=attributes.PASSIVE_RETURN_NO_VALUE),
+            attributes.NO_VALUE,
         )
         assert "attr" not in dict_
 
@@ -1581,7 +1577,7 @@ class PendingBackrefTest(fixtures.ORMTest):
         )
         eq_(lazy_posts.call_count, 1)
 
-    def test_passive_history_collection_never_set(self):
+    def test_passive_history_collection_no_value(self):
         Post, Blog, lazy_posts = self._fixture()
 
         lazy_posts.return_value = attributes.PASSIVE_NO_RESULT
@@ -1594,10 +1590,10 @@ class PendingBackrefTest(fixtures.ORMTest):
             attributes.instance_dict(b),
         )
 
-        # this sets up NEVER_SET on b.posts
+        # this sets up NO_VALUE on b.posts
         p.blog = b
 
-        eq_(state.committed_state, {"posts": attributes.NEVER_SET})
+        eq_(state.committed_state, {"posts": attributes.NO_VALUE})
         assert "posts" not in dict_
 
         # then suppose the object was made transient again,
@@ -1607,7 +1603,7 @@ class PendingBackrefTest(fixtures.ORMTest):
         p2 = Post("asdf")
         p2.blog = b
 
-        eq_(state.committed_state, {"posts": attributes.NEVER_SET})
+        eq_(state.committed_state, {"posts": attributes.NO_VALUE})
         eq_(dict_["posts"], [p2])
 
         # then this would fail.
@@ -2080,17 +2076,17 @@ class HistoryTest(fixtures.TestBase):
         assert "someattr" not in f.__dict__
         assert "someattr" not in attributes.instance_state(f).committed_state
 
-    def test_collection_never_set(self):
+    def test_collection_no_value(self):
         Foo = self._fixture(uselist=True, useobject=True, active_history=True)
         f = Foo()
         eq_(self._someattr_history(f, passive=True), (None, None, None))
 
-    def test_scalar_obj_never_set(self):
+    def test_scalar_obj_no_value(self):
         Foo = self._fixture(uselist=False, useobject=True, active_history=True)
         f = Foo()
         eq_(self._someattr_history(f, passive=True), (None, None, None))
 
-    def test_scalar_never_set(self):
+    def test_scalar_no_value(self):
         Foo = self._fixture(
             uselist=False, useobject=False, active_history=True
         )
@@ -3483,14 +3479,14 @@ class EventPropagateTest(fixtures.TestBase):
             b = B()
             b.attrib = "foo"
             eq_(b.attrib, "foo")
-            eq_(canary, [("foo", attributes.NEVER_SET)])
+            eq_(canary, [("foo", attributes.NO_VALUE)])
 
             c = C()
             c.attrib = "bar"
             eq_(c.attrib, "bar")
             eq_(
                 canary,
-                [("foo", attributes.NEVER_SET), ("bar", attributes.NEVER_SET)],
+                [("foo", attributes.NO_VALUE), ("bar", attributes.NO_VALUE)],
             )
 
     def test_propagate(self):
@@ -3531,16 +3527,13 @@ class EventPropagateTest(fixtures.TestBase):
             d1 = D()
             b.attrib = d1
             is_(b.attrib, d1)
-            eq_(canary, [(d1, attributes.NEVER_SET)])
+            eq_(canary, [(d1, attributes.NO_VALUE)])
 
             c = C()
             d2 = D()
             c.attrib = d2
             is_(c.attrib, d2)
-            eq_(
-                canary,
-                [(d1, attributes.NEVER_SET), (d2, attributes.NEVER_SET)],
-            )
+            eq_(canary, [(d1, attributes.NO_VALUE), (d2, attributes.NO_VALUE)])
 
     def _test_propagate_fixtures(self, active_history, useobject):
         classes = [None, None, None, None]
diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py
index 8d1e82fb5..0ababc262 100644
--- a/test/orm/test_lazy_relations.py
+++ b/test/orm/test_lazy_relations.py
@@ -1096,9 +1096,9 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
-            attributes.NEVER_SET,
+            attributes.NO_VALUE,
         )
         assert "user_id" not in a1.__dict__
         assert "user" not in a1.__dict__
@@ -1109,7 +1109,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get_history(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             ((), (), ()),
         )
@@ -1174,7 +1174,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             User(name="ed"),
         )
@@ -1185,7 +1185,7 @@ class GetterStateTest(_fixtures.FixtureTest):
             Address.user.impl.get_history(
                 attributes.instance_state(a1),
                 attributes.instance_dict(a1),
-                passive=attributes.PASSIVE_RETURN_NEVER_SET,
+                passive=attributes.PASSIVE_RETURN_NO_VALUE,
             ),
             ((), [User(name="ed")], ()),
         )