Don't emit warnings on descriptor access

This commit is revising 5162f2bc5fc0ac239f26a76fc9f0c2, which
when I did it felt a little rushed but I couldn't find anything
wrong.  Well here we are :).

Fixed issue where a :class:`.RemovedIn20Warning` would erroneously emit
when the ``.bind`` attribute were accessed internally on objects,
particularly when stringifying a SQL construct.

Alter the deprecated() decorator so that we can use it just to add
docstring warnings but not actually warn when the function is
accessed, adding new argument enable_warnings that can be
set to False.

Added a safety feature to deprecated_20() that will disallow an
":attr:" from proceeding if enable_warnings=False isn't present,
unless there's an extra flag
warn_on_attribute_access, since we want Session.transaction to
emit a deprecation warning.  This is a little hacky but it's essentially
modifying the decorator to require a positive assertion that
a deprecation decorator on a descriptor should actually warn
on access.

Remove the warning filter for session.transaction and get
tests to pass to ensure this is not also being called
internally.

Added tests to ensure that common places .bind can be passed
as a parameter definitely warn as I was not able to find
this otherwise.

Fixes: #5717
Change-Id: Ia586b4f9ee6b212f3a71104b1caf40b5edd399e2

5 files changed, 33 insertions, 6 deletions

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 2fc2ad68c..b89edca44 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1053,6 +1053,7 @@ class Session(_SessionClassMethods):
         ":meth:`_orm.Session.begin`.  To access "
         "the current root transaction, use "
         ":meth:`_orm.Session.get_transaction`.",
+        warn_on_attribute_access=True,
     )
     def transaction(self):
         """The current active or inactive :class:`.SessionTransaction`.
@@ -1065,6 +1066,9 @@ class Session(_SessionClassMethods):
 
 
         """
+        return self._legacy_transaction()
+
+    def _legacy_transaction(self):
         if not self.future:
             self._autobegin()
         return self._transaction
diff --git a/lib/sqlalchemy/sql/base.py b/lib/sqlalchemy/sql/base.py
index 999169f90..ff44ab27c 100644
--- a/lib/sqlalchemy/sql/base.py
+++ b/lib/sqlalchemy/sql/base.py
@@ -909,6 +909,7 @@ class Executable(Generative):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`
diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py
index 086cef48a..d60afdbac 100644
--- a/lib/sqlalchemy/sql/selectable.py
+++ b/lib/sqlalchemy/sql/selectable.py
@@ -1238,6 +1238,7 @@ class Join(roles.DMLTableRole, FromClause):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Return the bound engine associated with either the left or right
@@ -3559,6 +3560,7 @@ class CompoundSelect(HasCompileState, GenerativeSelect):
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`
@@ -5443,6 +5445,7 @@ class Select(
     @util.deprecated_20(
         ":attr:`.Executable.bind`",
         alternative="Bound metadata is being removed as of SQLAlchemy 2.0.",
+        enable_warnings=False,
     )
     def bind(self):
         """Returns the :class:`_engine.Engine` or :class:`_engine.Connection`
diff --git a/lib/sqlalchemy/testing/warnings.py b/lib/sqlalchemy/testing/warnings.py
index 2a0be42c1..b230bad6f 100644
--- a/lib/sqlalchemy/testing/warnings.py
+++ b/lib/sqlalchemy/testing/warnings.py
@@ -63,7 +63,6 @@ def setup_filters():
         #
         r"The MetaData.bind argument is deprecated",
         r"The ``bind`` argument for schema methods that invoke SQL ",
-        r"The Executable.bind attribute is considered legacy ",
         r"The Function.bind argument",
         r"The select.bind argument",
         #
@@ -131,7 +130,6 @@ def setup_filters():
         r".*object is being merged into a Session along the backref "
         "cascade path",
         r"Passing bind arguments to Session.execute\(\) as keyword arguments",
-        r"The Session.transaction attribute",
         r"The merge_result\(\) method is superseded by the "
         r"merge_frozen_result\(\)",
         r"The Session.begin.subtransactions flag is deprecated",
diff --git a/lib/sqlalchemy/util/deprecations.py b/lib/sqlalchemy/util/deprecations.py
index f46374601..a4c9d9d0e 100644
--- a/lib/sqlalchemy/util/deprecations.py
+++ b/lib/sqlalchemy/util/deprecations.py
@@ -112,7 +112,11 @@ def deprecated_20_cls(
 
 
 def deprecated(
-    version, message=None, add_deprecation_to_docstring=True, warning=None
+    version,
+    message=None,
+    add_deprecation_to_docstring=True,
+    warning=None,
+    enable_warnings=True,
 ):
     """Decorates a function and issues a deprecation warning on use.
 
@@ -157,7 +161,12 @@ def deprecated(
 
     def decorate(fn):
         return _decorate_with_warning(
-            fn, warning, message % dict(func=fn.__name__), version, header
+            fn,
+            warning,
+            message % dict(func=fn.__name__),
+            version,
+            header,
+            enable_warnings=enable_warnings,
         )
 
     return decorate
@@ -189,6 +198,16 @@ def deprecated_20(api_name, alternative=None, becomes_legacy=False, **kw):
         )
     )
 
+    if ":attr:" in api_name:
+        attribute_ok = kw.pop("warn_on_attribute_access", False)
+        if not attribute_ok:
+            assert kw.get("enable_warnings") is False, (
+                "attribute %s will emit a warning on read access.  "
+                "If you *really* want this, "
+                "add warn_on_attribute_access=True.  Otherwise please add "
+                "enable_warnings=False." % api_name
+            )
+
     if alternative:
         message += " " + alternative
 
@@ -347,7 +366,7 @@ def _decorate_cls_with_warning(
 
 
 def _decorate_with_warning(
-    func, wtype, message, version, docstring_header=None
+    func, wtype, message, version, docstring_header=None, enable_warnings=True
 ):
     """Wrap a function with a warnings.warn and augmented docstring."""
 
@@ -363,7 +382,9 @@ def _decorate_with_warning(
 
     @decorator
     def warned(fn, *args, **kwargs):
-        skip_warning = kwargs.pop("_sa_skip_warning", False)
+        skip_warning = not enable_warnings or kwargs.pop(
+            "_sa_skip_warning", False
+        )
         if not skip_warning:
             _warn_with_version(message, version, wtype, stacklevel=3)
         return fn(*args, **kwargs)