Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix try-else blocks. #26

Merged
merged 2 commits into from
Aug 14, 2014
Merged

fix try-else blocks. #26

merged 2 commits into from
Aug 14, 2014

Conversation

bukzor
Copy link
Contributor

@bukzor bukzor commented Aug 9, 2014

closes #24 and #25

Minus seven lines and two bugs.

@sigmavirus24
Copy link
Member

How does this handle try/finally blocks (and try/else/finally)?

(Otherwise 👍 this looks great. Thanks for catching these)

@bukzor
Copy link
Contributor Author

bukzor commented Aug 10, 2014

tryfinally parses fine without explicit support.
It traverses the finally block after the try blocks, which gives a graph that's semantically correct.

It does weird things in global scope though because the try and anything in the finally are viewed as separate constructs, but most things in global scope are weird currently. IMO that's a separate bug, which I'm working on separately.

@bukzor
Copy link
Contributor Author

bukzor commented Aug 10, 2014

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

mccabe

@bukzor
Copy link
Contributor Author

bukzor commented Aug 10, 2014

... should the tryelse clause actually add to the complexity? I don't think it's linearly independant from the main try clause. In other words, because it's only ever run if none of the except clauses are run, and always after the code in the main clause, it doesn't seem to provide a separate line of execution from any of the other clauses.

@bukzor
Copy link
Contributor Author

bukzor commented Aug 10, 2014

Proposed graph for above code, below. This seems to correctly indicate that the else always runs directly after the main try clause, if it runs, and never in a line of execution that involves the exception handlers. This has 1 less complexity than the above graph.

mccabe

@sigmavirus24
Copy link
Member

I think it should add to the complexity. Here's my reasoning: Either Stmt 3 is run and then Stmt 5, Stmt 7, or Stmt 9 depending on the side effect of Stmt 3. What I think is misleading about the graph is that the try node and Stmt 3 nodes are independent. I would think it would be something like

                   -> "stmt 5" -------
                 /                      \
"f()" -> "try & stmt 3" -> "stmt 9" -> "finally" -> ...
                 \                      /
                   -> "stmt 7"--------

@sigmavirus24
Copy link
Member

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
Copy link
Contributor Author

bukzor commented Aug 11, 2014

Your graphs don't support your assertion that the 'else' should add to the
complexity. Discounting the fact that we don't exactly know where the
handler code will branch off from the main flow, both your graphs show just
three distinct code paths, with a complexity of 3, rather than the four as
currently parsed.

--phone is hard.
On Aug 10, 2014 6:24 PM, "Ian Cordasco" notifications@github.com wrote:

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


Reply to this email directly or view it on GitHub
#26 (comment).

@bukzor
Copy link
Contributor Author

bukzor commented Aug 11, 2014

At any rate, I think we've opened two separate issues:

  1. How should try-else clauses be modeled?
  2. How should try-except clauses be modeled?

I think those issues can be handled separately, and the patch at hand is a strict improvement over master.

@bukzor
Copy link
Contributor Author

bukzor commented Aug 12, 2014

Any blockers?

@sigmavirus24
Copy link
Member

Sorry @bukzor no. I don't think there are any blockers. I'm just going to review this once more before merging.

sigmavirus24 added a commit that referenced this pull request Aug 14, 2014
@sigmavirus24 sigmavirus24 merged commit e3ea38a into PyCQA:master Aug 14, 2014
@bukzor
Copy link
Contributor Author

bukzor commented Aug 14, 2014

No worries.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global try statements are not counted
2 participants