Deprecate __len__ of Segment

Print a warning message when a deprecated function/property/class is used

6 files changed, 325 insertions, 31 deletions

diff --git a/python/__init__.py b/python/__init__.py
index 6cb7b162..913fde6a 100644
--- a/python/__init__.py
+++ b/python/__init__.py
@@ -75,6 +75,12 @@ from .log import (
     log_to_stdout, log_to_stderr, log_to_file, close_logs
 )
 from .log import log as log_at_level
+from .deprecation import *
+import warnings
+# We must alter the filter settings for DeprecatedWarning. Otherwise, it will never show up.
+# https://docs.python.org/3/library/warnings.html#default-warning-filter
+warnings.filterwarnings('once', '', DeprecatedWarning)
+
 # Only load Enterprise Client support on Enterprise builds
 if core.BNGetProduct() == "Binary Ninja Enterprise Client":
 	from .enterprise import *
diff --git a/python/binaryview.py b/python/binaryview.py
index 917d9e51..16c04279 100644
--- a/python/binaryview.py
+++ b/python/binaryview.py
@@ -68,6 +68,7 @@ from . import workflow as _workflow
 from . import function as _function
 from . import types as _types
 from . import platform as _platform
+from . import deprecation
 
 PathType = Union[str, os.PathLike]
 InstructionsType = Generator[Tuple[List['_function.InstructionTextToken'], int], None, None]
@@ -1306,8 +1307,14 @@ class Segment:
 		x = "x" if self.executable else "-"
 		return f"<segment: {self.start:#x}-{self.end:#x}, {r}{w}{x}>"
 
+	@deprecation.deprecated(details="Use .length instead. Python disallows the length of an object to "
+									">= 0x8000000000000000. See https://bugs.python.org/issue21444.")
 	def __len__(self):
-		return core.BNSegmentGetLength(self.handle)
+		return self.length
+
+	@property
+	def length(self):
+		return int(core.BNSegmentGetLength(self.handle))
 
 	def __eq__(self, other):
 		if not isinstance(other, self.__class__):
@@ -1403,11 +1410,9 @@ class Section:
 	def __repr__(self):
 		return f"<section {self.name}: {self.start:#x}-{self.end:#x}>"
 
-	@decorators.deprecated
+	@deprecation.deprecated(details="Use .length instead. Python disallows the length of an object to "
+									">= 0x8000000000000000. See https://bugs.python.org/issue21444.")
 	def __len__(self):
-		# deprecated - Python does allow the returning of integers >= 0x8000000000000000
-		# please use .length instead
-		# for more information about this limitation in python see https://bugs.python.org/issue21444
 		return self.length
 
 	def __eq__(self, other):
@@ -2036,11 +2041,9 @@ class BinaryView:
 			return f"<BinaryView: '{filename}', {size}>"
 		return f"<BinaryView: {size}>"
 
-	@decorators.deprecated
+	@deprecation.deprecated(details="Use .length instead. Python disallows the length of an object to "
+									">= 0x8000000000000000. See https://bugs.python.org/issue21444.")
 	def __len__(self):
-		# deprecated - Python does allow the returning of integers >= 0x8000000000000000
-		# please use .length instead
-		# for more information about this limitation in python see https://bugs.python.org/issue21444
 		return int(core.BNGetViewLength(self.handle))
 
 	@property
@@ -8699,14 +8702,20 @@ class BinaryWriter:
 		core.BNSeekBinaryWriterRelative(self._handle, offset)
 
 
-@decorators.deprecated
-@dataclass
+@dataclass(init=False)
 class StructuredDataValue(object):
 	type: '_types.Type'
 	address: int
 	value: bytes
 	endian: Endianness
 
+	@deprecation.deprecated(details='StructuredDataValue is deprecated.')
+	def __init__(self, t: '_type.Type', addr: int, val: bytes, e: Endianness):
+		self.type = t
+		self.address = addr
+		self.value = val
+		self.endian = e
+
 	def __str__(self):
 		decode_str = "{}B".format(self.type.width)
 		return ' '.join([f"{x:02x}" for x in struct.unpack(decode_str, self.value)])
@@ -8742,7 +8751,6 @@ class StructuredDataValue(object):
 		return int(self)
 
 
-@decorators.deprecated
 class StructuredDataView(object):
 	"""
 		``class StructuredDataView`` is a convenience class for reading structured binary data.
@@ -8761,6 +8769,7 @@ class StructuredDataView(object):
 			>>>
 
 		"""
+	@deprecation.deprecated(details='StructuredDataView is deprecated. Use TypedDataAccessor or DataVariable.value instead.')
 	def __init__(self, bv: 'BinaryView', structure_name: '_types.QualifiedNameType', address: int):
 		self._bv = bv
 		self._structure_name = structure_name
diff --git a/python/decorators.py b/python/decorators.py
index c6664652..6ac77be0 100644
--- a/python/decorators.py
+++ b/python/decorators.py
@@ -12,20 +12,6 @@ def passive(cls):
 	return cls
 
 
-def deprecated(cls):
-	deprecated_note = '''
-	.. warning:: This object is deprecated.  Please migrate code away from using this class or method.
-
-'''
-
-	if hasattr(cls, "__doc__") and cls.__doc__:
-		cls.__doc__ = deprecated_note + cls.__doc__
-	else:
-		cls.__doc__ = deprecated_note
-
-	return cls
-
-
 def enterprise(cls):
 	enterprise_note = '''
 	.. note: This object is only available in the Enterprise edition of Binary Ninja.
diff --git a/python/deprecation.py b/python/deprecation.py
new file mode 100644
index 00000000..27a67b2e
--- /dev/null
+++ b/python/deprecation.py
@@ -0,0 +1,290 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+import collections
+import functools
+import textwrap
+import warnings
+
+from packaging import version
+from datetime import date
+
+__version__ = "2.0.7"
+
+# This is mostly here so automodule docs are ordered more ideally.
+__all__ = ["deprecated", "message_location", "fail_if_not_removed",
+           "DeprecatedWarning", "UnsupportedWarning"]
+
+#: Location where the details are added to a deprecated docstring
+#:
+#: When set to ``"bottom"``, the details are appended to the end.
+#: When set to ``"top"``, the details are inserted between the
+#: summary line and docstring contents.
+message_location = "bottom"
+
+
+class DeprecatedWarning(DeprecationWarning):
+    """A warning class for deprecated methods
+
+    This is a specialization of the built-in :class:`DeprecationWarning`,
+    adding parameters that allow us to get information into the __str__
+    that ends up being sent through the :mod:`warnings` system.
+    The attributes aren't able to be retrieved after the warning gets
+    raised and passed through the system as only the class--not the
+    instance--and message are what gets preserved.
+
+    :param function: The function being deprecated.
+    :param deprecated_in: The version that ``function`` is deprecated in
+    :param removed_in: The version or :class:`datetime.date` specifying
+                       when ``function`` gets removed.
+    :param details: Optional details about the deprecation. Most often
+                    this will include directions on what to use instead
+                    of the now deprecated code.
+    """
+
+    def __init__(self, function, deprecated_in, removed_in, details=""):
+        # NOTE: The docstring only works for this class if it appears up
+        # near the class name, not here inside __init__. I think it has
+        # to do with being an exception class.
+        self.function = function
+        self.deprecated_in = deprecated_in
+        self.removed_in = removed_in
+        self.details = details
+        super(DeprecatedWarning, self).__init__(function, deprecated_in,
+                                                removed_in, details)
+
+    def __str__(self):
+        # Use a defaultdict to give us the empty string
+        # when a part isn't included.
+        parts = collections.defaultdict(str)
+        parts["function"] = self.function
+
+        if self.deprecated_in:
+            parts["deprecated"] = " as of %s" % self.deprecated_in
+        if self.removed_in:
+            parts["removed"] = " and will be removed {} {}".format("on" if isinstance(self.removed_in, date) else "in",
+                                                                   self.removed_in)
+        if any([self.deprecated_in, self.removed_in, self.details]):
+            parts["period"] = "."
+        if self.details:
+            parts["details"] = " %s" % self.details
+
+        return ("%(function)s is deprecated%(deprecated)s%(removed)s"
+                "%(period)s%(details)s" % (parts))
+
+
+class UnsupportedWarning(DeprecatedWarning):
+    """A warning class for methods to be removed
+
+    This is a subclass of :class:`~deprecation.DeprecatedWarning` and is used
+    to output a proper message about a function being unsupported.
+    Additionally, the :func:`~deprecation.fail_if_not_removed` decorator
+    will handle this warning and cause any tests to fail if the system
+    under test uses code that raises this warning.
+    """
+
+    def __str__(self):
+        parts = collections.defaultdict(str)
+        parts["function"] = self.function
+        parts["removed"] = self.removed_in
+
+        if self.details:
+            parts["details"] = " %s" % self.details
+
+        return ("%(function)s is unsupported as of %(removed)s."
+                "%(details)s" % (parts))
+
+
+def deprecated(deprecated_in=None, removed_in=None, current_version=None,
+               details=""):
+    """Decorate a function to signify its deprecation
+
+    This function wraps a method that will soon be removed and does two things:
+        * The docstring of the method will be modified to include a notice
+          about deprecation, e.g., "Deprecated since 0.9.11. Use foo instead."
+        * Raises a :class:`~deprecation.DeprecatedWarning`
+          via the :mod:`warnings` module, which is a subclass of the built-in
+          :class:`DeprecationWarning`. Note that built-in
+          :class:`DeprecationWarning`s are ignored by default, so for users
+          to be informed of said warnings they will need to enable them--see
+          the :mod:`warnings` module documentation for more details.
+
+    :param deprecated_in: The version at which the decorated method is
+                          considered deprecated. This will usually be the
+                          next version to be released when the decorator is
+                          added. The default is **None**, which effectively
+                          means immediate deprecation. If this is not
+                          specified, then the `removed_in` and
+                          `current_version` arguments are ignored.
+    :param removed_in: The version or :class:`datetime.date` when the decorated
+                       method will be removed. The default is **None**,
+                       specifying that the function is not currently planned
+                       to be removed.
+                       Note: This parameter cannot be set to a value if
+                       `deprecated_in=None`.
+    :param current_version: The source of version information for the
+                            currently running code. This will usually be
+                            a `__version__` attribute on your library.
+                            The default is `None`.
+                            When `current_version=None` the automation to
+                            determine if the wrapped function is actually
+                            in a period of deprecation or time for removal
+                            does not work, causing a
+                            :class:`~deprecation.DeprecatedWarning`
+                            to be raised in all cases.
+    :param details: Extra details to be added to the method docstring and
+                    warning. For example, the details may point users to
+                    a replacement method, such as "Use the foo_bar
+                    method instead". By default there are no details.
+    """
+    # You can't just jump to removal. It's weird, unfair, and also makes
+    # building up the docstring weird.
+    if deprecated_in is None and removed_in is not None:
+        raise TypeError("Cannot set removed_in to a value "
+                        "without also setting deprecated_in")
+
+    # Only warn when it's appropriate. There may be cases when it makes sense
+    # to add this decorator before a formal deprecation period begins.
+    # In CPython, PendingDeprecatedWarning gets used in that period,
+    # so perhaps mimick that at some point.
+    is_deprecated = False
+    is_unsupported = False
+
+    # StrictVersion won't take a None or a "", so make whatever goes to it
+    # is at least *something*. Compare versions only if removed_in is not
+    # of type datetime.date
+    if isinstance(removed_in, date):
+        if date.today() >= removed_in:
+            is_unsupported = True
+        else:
+            is_deprecated = True
+    elif current_version:
+        current_version = version.parse(current_version)
+
+        if (removed_in
+                and current_version >= version.parse(removed_in)):
+            is_unsupported = True
+        elif (deprecated_in
+              and current_version >= version.parse(deprecated_in)):
+            is_deprecated = True
+    else:
+        # If we can't actually calculate that we're in a period of
+        # deprecation...well, they used the decorator, so it's deprecated.
+        # This will cover the case of someone just using
+        # @deprecated("1.0") without the other advantages.
+        is_deprecated = True
+
+    should_warn = any([is_deprecated, is_unsupported])
+
+    def _function_wrapper(function):
+        if should_warn:
+            # Everything *should* have a docstring, but just in case...
+            existing_docstring = function.__doc__ or ""
+
+            # The various parts of this decorator being optional makes for
+            # a number of ways the deprecation notice could go. The following
+            # makes for a nicely constructed sentence with or without any
+            # of the parts.
+
+            # If removed_in is a date, use "removed on"
+            # If removed_in is a version, use "removed in"
+            parts = {
+                "deprecated_in":
+                    " %s" % deprecated_in if deprecated_in else "",
+                "removed_in":
+                    "\n   This will be removed {} {}.".format("on" if isinstance(removed_in, date) else "in",
+                                                              removed_in) if removed_in else "",
+                "details":
+                    " %s" % details if details else ""}
+
+            deprecation_note = (".. deprecated::{deprecated_in}"
+                                "{removed_in}{details}".format(**parts))
+
+            # default location for insertion of deprecation note
+            loc = 1
+
+            # split docstring at first occurrence of newline
+            string_list = existing_docstring.split("\n", 1)
+
+            if len(string_list) > 1:
+                # With a multi-line docstring, when we modify
+                # existing_docstring to add our deprecation_note,
+                # if we're not careful we'll interfere with the
+                # indentation levels of the contents below the
+                # first line, or as PEP 257 calls it, the summary
+                # line. Since the summary line can start on the
+                # same line as the """, dedenting the whole thing
+                # won't help. Split the summary and contents up,
+                # dedent the contents independently, then join
+                # summary, dedent'ed contents, and our
+                # deprecation_note.
+
+                # in-place dedent docstring content
+                string_list[1] = textwrap.dedent(string_list[1])
+
+                # we need another newline
+                string_list.insert(loc, "\n")
+
+                # change the message_location if we add to end of docstring
+                # do this always if not "top"
+                if message_location != "top":
+                    loc = 3
+
+            # insert deprecation note and dual newline
+            string_list.insert(loc, deprecation_note)
+            string_list.insert(loc, "\n\n")
+
+            function.__doc__ = "".join(string_list)
+
+        @functools.wraps(function)
+        def _inner(*args, **kwargs):
+            if should_warn:
+                if is_unsupported:
+                    cls = UnsupportedWarning
+                else:
+                    cls = DeprecatedWarning
+
+                the_warning = cls(function.__name__, deprecated_in,
+                                  removed_in, details)
+                warnings.warn(the_warning, category=DeprecationWarning,
+                              stacklevel=2)
+
+            return function(*args, **kwargs)
+        return _inner
+    return _function_wrapper
+
+
+def fail_if_not_removed(method):
+    """Decorate a test method to track removal of deprecated code
+
+    This decorator catches :class:`~deprecation.UnsupportedWarning`
+    warnings that occur during testing and causes unittests to fail,
+    making it easier to keep track of when code should be removed.
+
+    :raises: :class:`AssertionError` if an
+             :class:`~deprecation.UnsupportedWarning`
+             is raised while running the test method.
+    """
+    # NOTE(briancurtin): Unless this is named test_inner, nose won't work
+    # properly. See Issue #32.
+    @functools.wraps(method)
+    def test_inner(*args, **kwargs):
+        with warnings.catch_warnings(record=True) as caught_warnings:
+            warnings.simplefilter("always")
+            rv = method(*args, **kwargs)
+
+        for warning in caught_warnings:
+            if warning.category == UnsupportedWarning:
+                raise AssertionError(
+                    ("%s uses a function that should be removed: %s" %
+                     (method, str(warning.message))))
+        return rv
+    return test_inner
diff --git a/python/function.py b/python/function.py
index 4a780a67..c35d5650 100644
--- a/python/function.py
+++ b/python/function.py
@@ -43,6 +43,7 @@ from . import variable
 from . import flowgraph
 from . import callingconvention
 from . import workflow
+from . import deprecation
 
 # we define the following as such so the linter doesn't confuse 'highlight' the module with the
 # property of the same name. There is probably some other work around but it eludes me.
@@ -1323,16 +1324,16 @@ class Function:
 				yield i[0], start
 				start += i[1]
 
-	@decorators.deprecated
 	@property
+	@deprecation.deprecated(details="Use LowLevelIlFunction.instructions instead.")
 	def llil_instructions(self) -> Generator['lowlevelil.LowLevelILInstruction', None, None]:
 		"""
 		.. note:: Use :py:meth:`LowLevelIlFunction.instructions` instead.
 		"""
 		return self.llil.instructions
 
-	@decorators.deprecated
 	@property
+	@deprecation.deprecated(details="Use MediumLevelIlFunction.instructions instead.")
 	def mlil_instructions(self) -> Generator['mediumlevelil.MediumLevelILInstruction', None, None]:
 		"""
 		.. note:: Use :py:meth:`MediumLevelIlFunction.instructions` instead.
@@ -1429,12 +1430,12 @@ class Function:
 	def get_comment_at(self, addr: int) -> str:
 		return core.BNGetCommentForAddress(self.handle, addr)
 
-	@decorators.deprecated
+	@deprecation.deprecated(details="Use Function.set_comment_at instead.")
 	def set_comment(self, addr: int, comment: str) -> None:
 		"""
 		.. note:: Use :py:meth:`set_comment_at` instead.
 		"""
-		core.BNSetCommentForAddress(self.handle, addr, comment)
+		self.set_comment_at(addr, comment)
 
 	def set_comment_at(self, addr: int, comment: str) -> None:
 		"""
diff --git a/python/highlevelil.py b/python/highlevelil.py
index 873c7fc7..43a5c68e 100644
--- a/python/highlevelil.py
+++ b/python/highlevelil.py
@@ -45,6 +45,7 @@ from .commonil import (
     BaseILInstruction, Tailcall, Syscall, Localcall, Comparison, Signed, UnaryOperation, BinaryOperation, SSA, Phi,
     Loop, ControlFlow, Memory, Constant, Arithmetic, DoublePrecision, Terminal, FloatingPoint, Intrinsic, Return
 )
+from . import deprecation
 
 TokenList = List['function.InstructionTextToken']
 LinesType = Generator['function.DisassemblyTextLine', None, None]
@@ -2092,13 +2093,14 @@ ILInstruction = {
 }
 
 
-@decorators.deprecated
 class HighLevelILExpr:
 	"""
 	``class HighLevelILExpr`` hold the index of IL Expressions.
 
 	.. note:: Use ExpressionIndex instead
 	"""
+
+	@deprecation.deprecated(details='HighLevelILExpr is deprecated. Use ExpressionIndex instead')
 	def __init__(self, index: ExpressionIndex):
 		self._index = index