[Checkers] Question about the flow algorithm

[Adam Warski]

Hello,

reading the algorithm a bit more, I think it could be improved.

The finally{} block should I be in fact evaluated with two possible  
annotation sets: in the case control continues after it normally (this  
covers the "alive" catches + the case when there are no exceptions in  
try{}), and the case when the control doesn't continue normally ("non- 
alive" catches).

The two cases can be different. The annos set after the whole try- 
catch-finally construct should correspond to the flows covering only  
the try, alive catches and finally block, not the ones covering the  
non-alive catches. In a special case, this means that if all catches  
are not alive, then the annos set after finally should only cover the  
flow of try completing without exceptions + finally block..

This could be implemented as follows:
We evaluate the try{} normally, and store two annotation sets:
- "annosAfterNormal", which is a copy of "annos", assuming that no  
exceptions were thrown
- "annosBeforeCatch", which is "annos" & tryBits.pop(), which covers  
the possibility of throwing an exception by any method that declares  
thrown exceptions

Then we evaluate each catch, storing after it a pair:  
(annosAfterCatch, catchAlive). The initial annotation set for each  
catch is "annosBeforeCatch".

Then we come to evaluating the finally block; it is evaluated twice,  
with initial annotation sets:
(1) conjunction of all annosAfterCatch where catchAlive == true and  
the "annosAfterNormal" set
(2) conjunction of all annosAfterCatch where catchAlive == false

The annotation set after the whole try-catch-construct should be the  
annotation set after evaluating finally (1).

What do you think? Any bugs that you can see? I haven't implemented it  
yet, but if you won't see any problems, I'll post the code here in  
some time :)

Adam

On Mar 22, 2009, at 11:16 AM, Adam Warski wrote:

> Hello,
>
> I've got a question about the flow algorithm, more specifically  
> about the try-catch-finally handling.
>
> In visitTry(), there's a code block:
>
> if (node.getCatches() != null) {
>        boolean catchAlive = true;
>        for (CatchTree ct : node.getCatches()) {
>            scan(ct, p);
>            catchAlive &= alive;
>        }
>        // Conservative: only if there's no finally
>        if (!catchAlive && node.getFinallyBlock() == null)
>            annos = GenKillBits.copy(annoAfterBlock);
>    }
>
> which, after scanning the try{} block, scans the catch{} blocks, if  
> there are any. Now, the last part says that if the caches aren't  
> alive, that is, control can't "pass" through them, the state of the  
> annotations after the whole try-catch should be the state of the  
> annotations as it was at the end of the catch{} block - which is  
> correct. However, I think this should happen only if ALL catches  
> aren't alive. And right now, the "catchAlive" variable will be false  
> if at least one catch isn't alive. So if one catch is alive, and a  
> second isn't, the state of the annotations will be restored.
>
> So in short, I think that the lines:
>
>            boolean catchAlive = true;
>            catchAlive &= alive;
>
> should be changed to:
>
>            boolean catchAlive = false;
>            catchAlive |= alive;
>
> -- 
> Adam