#1352 Strange behavior of PruneUnconditionalExceptionThrowerEdges

[Tagir Valeev]

Sometimes edge is marked as unconditional exception edge when it's actually not. See PruneUnconditionalExceptionThrowerEdges#execute:

// Ignore abstract and native methods
if (!(xMethod.isFinal() || xMethod.isStatic() || xMethod.isPrivate())) {
    try {
        isExact = false;
        XClass xClass = Global.getAnalysisCache().getClassAnalysis(XClass.class,
                xMethod.getClassDescriptor());
        if (xClass.isAbstract()) {
            continue;
        }
    } catch (CheckedAnalysisException e) {
        AnalysisContext.logError("Unable to resolve class for " + xMethod, e);
    }
}

Here we skip for some reason all abstract classes even though the method in question might not be abstract. This leads to the erroneous behavior in the cases like this:

abstract class Super {
    public void test() {
        System.out.println("ok");
    }
}

class ChildOk extends Super {}

class ChildUnsupported extends Super {
    @Override
    public void test() {
        throw new UnsupportedOperationException();
    }
}

public class PruneExceptionTest {
    @ExpectWarning("NP_NULL_ON_SOME_PATH")
    public void testMethod(String s, Super c) {
        if(s == null) {
            System.out.println("!");
        }
        c.test();
        System.out.println(s.trim());
    }
}

Here we call c.test() which can either throw an exception or correctly output the "ok" string depending on the object passed. However PruneUnconditionalExceptionThrowerEdges ignores the test() method in abstract superclass and marks c.test() call as unconditional thrower. This results in removing the rest of the method from CFG. In particular expected NP_NULL_ON_SOME_PATH warning is not issued. Simply removing the "abstract" modifier on super class makes everything ok.

I think that xMethod.isAbstract should be checked instead. I've read the commits history for PruneUnconditionalExceptionThrowerEdges class. According to the commit 687f623b1 "If class is abstract, ignore unconditional throwing methods unless the method can't be overriden" the idea was to skip abstract classes if the method declared there is unconditional thrower. But we should not skip them if it's normal method. Thus this abstract class test should be performed under if (isUnconditionalThrower) branch.

Will commit the fix after testing.

[Tagir Valeev]

Fixed:
https://code.google.com/p/findbugs/source/detail?r=e18a6e925874d10a0ae5b8fc264936799fcc9a1f