tantale / deprecated

Python @deprecated decorator to deprecate old python classes, functions or methods.
MIT License
301 stars 32 forks source link

Refer to a function caller not wrapt/wrappers.py #13

Closed MHendricks closed 4 years ago

MHendricks commented 4 years ago

When using @deprecated.deprecated on a module function, the raised warning reports the problem in "wrapt\wrappers.py" instead of the file that called the decorated function.

test.py:

from deprecated import deprecated
@deprecated
def foo1():
    pass

class Test(object):
    @deprecated
    def foo2(self):
        pass

if __name__ == '__main__':
    foo1()

    t = Test()
    t.foo2()

When running python test.py with the un-patched deprecated module results in:

C:\Program Files\Python37\lib\site-packages\wrapt\wrappers.py:564: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  args, kwargs)
C:\Program Files\Python37\lib\site-packages\wrapt\wrappers.py:603: DeprecationWarning: Call to deprecated method foo2.
  args, kwargs)

This makes it hard to track down the code that is calling the deprecated method.

With the patch:

test.py:12: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
test.py:15: DeprecationWarning: Call to deprecated method foo2.
  t.foo2()

This makes it easy to find and fix the call to a deprecated method and I assume is the desired behavior.

MHendricks commented 4 years ago

Hmm, I guess ~windows has a different stacklevel than linux does.~ I guess I'll need to figure that out.

coveralls commented 4 years ago

Coverage Status

Coverage decreased (-1.7%) to 94.444% when pulling cd8050396383aea85f46b2f97f7fb68c36fb5ae1 on blurstudio:stacklevel-fix into 6145b59cb797a8ed6df1bbb9a53d06f1315e24e9 on tantale:master.

MHendricks commented 4 years ago

I'm not sure what is different between the appveyor windows python and my windows python setups, but for some reason it changes the stacklevel requirement.

Windows 7 Python 2.7.3 requires a stack level of 3. Windows 10 Python 2.7.16 requires a stack level of 3. Centos 7.6.1810 Python 2.7.5 requires a stack level of 2. The tests seem to require a stack level of 2 except for python 3.4 64bit, and Seem to be running on windows.

test.py:

from deprecated import deprecated
@deprecated
def foo1():
    pass

if __name__ == '__main__':
    foo1()
diff --git a/deprecated/classic.py b/deprecated/classic.py
index f214ec0..9086e91 100644
--- a/deprecated/classic.py
+++ b/deprecated/classic.py
@@ -235,6 +235,10 @@ def deprecated(*args, **kwargs):
                     else:
                         stacklevel = 2
                     warnings.warn(msg, category=category, stacklevel=stacklevel)
+                    import traceback
+                    print('-'*50)
+                    traceback.print_stack()
+                    print('-'*51)
                 return wrapped_(*args_, **kwargs_)

             return wrapper_function(wrapped)

If I make this change to show the stack when the warning is raised I get different results. Windows:

test.py:7: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
--------------------------------------------------
  File "test.py", line 7, in <module>
    foo1()
  File "C:\temp\depr_test\lib\site-packages\wrapt\wrappers.py", line 564, in __call__
    args, kwargs)
  File "c:\blur\dev\deprecated\deprecated\classic.py", line 240, in wrapper_function
    traceback.print_stack()
---------------------------------------------------

Centos:

test.py:7: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
--------------------------------------------------
  File "test.py", line 7, in <module>
    foo1()
  File "/home/ad.blur.com/mikeh/checkouts/deprecated/deprecated/classic.py", line 240, in wrapper_function
    traceback.print_stack()
---------------------------------------------------

I think this may be a issue with wrapt, but so far I've been unable to follow the code of wrapt to figure out how it could be evaluating differently in the different environments.

Do you have any suggestions on a better way to ensure the correct stacklevel is used in the deprecation warning, or any idea why the appveyor tests seem to have different results?

tantale commented 4 years ago

Thank you for your contribution.

On Windows 7 Professional, I have:

(py-tmp) D:\Laurent\Projets\workspace\tmp>python --version
Python 2.7.16

(py-tmp) D:\Laurent\Projets\workspace\tmp>python test.py
test.py:17: DeprecationWarning: Call to deprecated function (or staticmethod) foo1.
  foo1()
test.py:19: DeprecationWarning: Call to deprecated method foo2.
  t.foo2()

So, I can't reproduce the problem on this platform.

But, you are right, the problem comes from the Wrapt library which perform some code modification at runtime, and uses C extension…

tantale commented 4 years ago

Can you try to add a unit test which check that the __file__ is in the stack trace of a decorated function?

Here is a starting point:

import traceback
import wrapt

@wrapt.decorator
def pass_through(wrapped, instance, args, kwargs):
    return wrapped(*args, **kwargs)

@pass_through
def function():
    stack = traceback.format_stack()
    assert __file__ in stack[-1]

if __name__ == '__main__':
    function()

That way we can make sure that this is a Wrapt problem and find a way to work aroud it…

MHendricks commented 4 years ago

I figured it out. We don't have Microsoft Visual C++ Compiler for Python 2.7 installed.

Wrapt pip installs even if it can't compile the _wrappers.pyd file. After installing the C++ compiler and reinstalling wrapt, I'm getting the expected stack level you are getting.

You should be able to replicate the problem yourself by renaming your wrapt\_wrappers.pyd file. I've updated the merge request with a way to detect and correctly set the stacklevel.

Given that the py34 test had the problem I'm guessing it doesn't have _wrappers.pyd on AppVeyor.

I haven't been able to figure out a way make a unit test that covers both if _wrappers.pyd exists and doesn't exist, but I'll think about it over the weekend. Let me know if you have any suggestions or don't think its worth testing.