7
\$\begingroup\$

I wrote a decorator which makes Python 3.5 raise exceptions if the arguments that are passed to a type-hinted function are of the wrong type.

from typing import get_type_hints

def strict_types(f):
    def type_checker(*args, **kwargs):
        hints = get_type_hints(f)

        all_args = kwargs.copy()
        all_args.update(dict(zip(f.__code__.co_varnames, args)))

        for key in all_args:
            if key in hints:
                if type(all_args[key]) != hints[key]:
                    raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))

        res = f(*args, **kwargs)

        if type(res) == hints['return']:
            return res
        else:
            raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))

    return type_checker

It's used like this:

@strict_types
def concatenate_with_spam(text: str) -> str:
    return text + 'spam'

and if something of the wrong type is passed to the decorated function e.g. concatenate_with_spam(42) an exception is raised:

Exception: Type of text is <class 'int'> and not <class 'str'>

The part that I'm worried about is the part that checks the result. It calls the original function (to learn result of what type it returns), but returns a wrapped function type_checker that will be called one more time, so the runtime of function is (at least) multiplied by 2. Are there any other things I have missed? What can be improved here (except for not using this not-pythonic decorator and crucifying me for writing it)?

\$\endgroup\$
  • \$\begingroup\$ FYI, PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention." :) \$\endgroup\$ – Solomon Ucko Apr 16 '18 at 20:50
3
\$\begingroup\$

You should change this part a bit:

    for key in all_args:
        if key in hints:
            if type(all_args[key]) != hints[key]:
                raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))

Here, you are silently ignoring missing type hints, but if the user went as far as to use this decorator, you should raise an exception instead, like this:

    for key in all_args:
        try:
            if type(all_args[key]) != hints[key]:
                raise Exception('Type of {} is {} and not {}'.format(key, type(all_args[key]), hints[key]))
        except IndexError:
                raise TypeError('The formal parameter {} was not given a type'.format(key))

Specific Errors & Exceptions

Exception is hilariously un-informative, use TypeError to convey more information.

Order of conditionals

    if type(res) == hints['return']:
        return res
    else:
        raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))

Here, the real logic is in the else, but I prefer seeing the main logic after the if, like this:

if type(res) != hints['return']:
    raise Exception('Type of result is {} and not {}'.format(type(res), hints['return']))
return res

Now it looks more like a simple perturbation of an almost linear function, than a full-blown logic statement.

Please do not abbreviate

  • res -> result
  • func -> function

It takes 0.5 seconds more to type and a tenth of the time to read.

\$\endgroup\$
  • \$\begingroup\$ The reason I'm ignorring missing type hints is that a partially type-hinted function is a valid thing, so the user will probably expect my decorator to work with it too. Thanks for the idea to use TypeError and other ideas! \$\endgroup\$ – Ilya Peterov Sep 26 '15 at 17:14
  • \$\begingroup\$ @IlyaPeterov it sure is valid sintactically, but a user that really wants strong typing, and even uses a checker like yours for it, would probably like an error for missing type-annotations than the parameter silently not checked \$\endgroup\$ – Caridorc Sep 26 '15 at 17:17
  • \$\begingroup\$ Hello, your try except statement should be reduced, to not mask bugs. Also, as there are two different index operations it's not explicit on the one that you are guarding against, type(all_args[key]) != hints[key]. \$\endgroup\$ – Peilonrayz Sep 26 '15 at 20:32
  • \$\begingroup\$ @JoeWallis Thanks for the tips, will fix it tomorrow asap \$\endgroup\$ – Caridorc Sep 26 '15 at 21:11
1
\$\begingroup\$

So first, you go through all_args, but you don't use items(). This will first, make it so you don't have to type all_args[key], and should run faster.

Also the only times that you use all_args[key] it's wrapped in a type. As I like to promote laziness, you could use a generator comprehension to do this all in the for line, if you wish to.

for key, value in ((i, type(j)) for i, j in all_args.items()):
    if key in hints:
        if value != hints[key]:
            raise Exception('Type of {} is {} and not {}'.format(key, value, hints[key]))

Now the inside of the for loop is simple, for a little added complexity on the for line.


Exception is massive, I never ever ever want to see someone raise Exception. Let's say we have some code that raises almost any error inside a function that is wrapped in your wrapper. What will happen if we want to catch an except for invalid type input? Well the other function is never going to work right. And it will mask so many bugs.

I use the following to 'test' your program.

@strict_types
def should_work_function() -> str:
    return non_existant_value

@strict_types
def should_error_function() -> str:
    raise SystemError('There was a problem.')

# Masks a bug
try:
    should_work_function()
except Exception as e:
    print(e)

# Prevents intended use of the function.
try:
    should_work_function()
except Exception as e:
    print(e)

print('Done without errors apart from input type errors. :D')

So what's the problem? If you look at the output then you will see a problem straight away.

name 'non_existant_value' is not defined
There was a problem.
Done without errors apart from input type errors. :D

To amend this I would recommend subclassing Exception, you could try TypeError, but that might make some confusion as stopping a='abc';a['a'] will stop you exception too.

class StrictTypeError(Exception):pass

When you change your function, and my 'test', to use that exception the output of the above 'test' will show something like:

Traceback (most recent call last):
  File "D:/programs/stack/strictTypes.py", line 35, in <module>
    should_work_function()
  File "D:/programs/stack/strictTypes.py", line 17, in type_checker
    res = f(*args, **kwargs)
  File "D:/programs/stack/strictTypes.py", line 28, in should_work_function
    return non_existant_value
NameError: name 'non_existant_value' is not defined

Which is definitely better.


As this function should be as fast as possible, as you want less overhead when calling all your functions. you should aim at using the fastest methods. Looking at one of your question, Is checking if key is in dictionary and getting it's value in the same “if” safe? There was a small discussion on efficiency. Which was your current method is the fastest, unless most of the key's are in the dict.

If you were to blindly wrap the inner if in a try, then you have the potential of masking bugs. To avoid this use else as it only runs once try finishes successfully. This is if you have another potential IndexError than that will force the except to run.

And so I would recommend changing the for loop to something like:

for key, value in ((i, type(j)) for i, j in all_args.items()):
    try:
        hint = hints[key]
    except IndexError:
        pass # Put @Caridorc's raise if you wish
    else:
        if value != hint:
            raise StrictTypeError('Type of {} is {} and not {}'.format(key, value, hint))

Whilst the above is never going to raise IndexError, apart from on the hints[key] part, it is better to get into the habit of using the else. And your intent will easily be known if there is one statement in the try.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.