How should try-except clauses be modeled?

[bukzor]

From @sigmavirus24

Actually now that I think of a more complex example

def f():
    try:
        function_one(1)
        function_two(arg)
    except TypeA:
        print(2)
    except TypeB:
        print(3)
    else:
        print(4)
    finally:
        if x:
            print(5.1)
        else:
            print(5.2)

I see that actually you could branch to any of the except clauses from either of the statements in the try block. I still don't think that the except branches coming from the try block is correct so maybe something like

           -----------------> except clause 1
          /                 /               \
try -> function_one -> function_two ----> else
          \                 \               /
           -----------------> except clause 2

[bukzor]

If implemented, this change would cause the complexity of try statements to be a multiplication of the complexity of the main block by the number of handlers. This might actually be correct, but would be quite different from the current behavior. It would certainly motivate the best-pratice of only ever putting a single statement in the main try clause.

Also see discussion at #26

[sigmavirus24]

That graph I drew was wrong. I meant for the except clauses to converge on the finally node. Like so

           -----------------> except clause 1
          /                 /               \
try -> function_one -> function_two -> else -> finally
          \                 \               /
           -----------------> except clause 2

Also, I'm okay with prodding people into only writing the try/except blocks around a line or two that could raise the exception(s) they're looking to catch. It would encourage people to properly take advantage of else and finally as well. The other thing to consider is that even in the following case a finally block is executed but may not necessarily lead to any code afterwards:

import sys


def f():
    try:
        raise ValueError('Some message')
    except ValueError:
        sys.exit(1)
    finally:
        print("Yes finally blocks get executed even if the except clause runs sys.exit")
     print("But this will not get executed unless there's no exception raised.")

So really you have the above graph (although simplified) and now you have an edge pointing from finally to the print and you could realistically have an edge leading wherever the body of the specific except clause leads. This might be too much to think about for this tool though, unless we want to unconditionally add an extra edge and node (since they will balance each other out roughly, +1, -1). This might just create confusing graphs in the case where it's missing or where it is unnecessary. Thoughts @bukzor ?

[bukzor]

You should graph out your last idea.

If you're talking about leaving some nodes dangling, I noticed that our calculation doesn't seem to work for trees with more than one leaf:

if -> return
  \-> elif -> return 
  \-> else -> return

E - N + 2 ==> 5 - 6 + 2 = 1

Wikipedia seems to indicate that we need to connect all exit points back to the entry point and add 1 instead of 2:

  /--------------------\
 if -> return -/       |
   \-> elif -> return -|
   \-> else -> return /

8 - 6 + 1 = 3

I suppose this is equivalent to the current practice of making sure all "loose ends" are connected back to a single "bottom" point.

 if -> return --------\
   \-> elif -> return -->  bottom
   \-> else -> return /

8 - 7 + 2 = 3

[zaneb]

IMHO it's a mistake to think that the problem here is exception handlers. The problem is exceptions.

In Python, almost any statement may result in an exception (which effectively means branching to the end of the function, if there's no exception handler), so in theory the graph should have a branch from every node to the end of the function. (In fact, exceptions from signals, like KeyboardInterrupt, can be raised after any opcode, so you might need to add even more nodes.) Clearly actually doing this would turn mccabe into a glorified line counter, which is not a useful tool.

It's reasonable to decide that mccabe, in order to be a useful tool, doesn't take into account the possibility that exceptions may be raised at any point in the code. What is not reasonable IMHO is to decide that it does take that into account, but only inside try blocks. Exceptions may occur anywhere, not just in try blocks.

[sigmavirus24]

@zaneb I think it's clear that we want to fix this. Do you have time to fix this? I'm happy to review and merge a PR that rectifies this (known imperfect) situation.

[zaneb]

@sigmavirus24 did you mean to leave that comment on #27 rather than here? FWIW I have no plans to work on this.

[bukzor]

Yes, I agree with @zaneb above.

The end result is that try/finally counts as +1 complexity for each operation within the try clause.

This is a pretty big change though, and some users will find it quite surprising.
It does agree with my intuition, however; I find that Simple (isn't Easy) code invariably has exactly one operation in the try: block.

[sinback]

This is an old issue but I want to chime in that I think it's a really good idea - I think this would be a really good way to convince developers that try: except blocks should be short and specific.