#735 False - on SimplifyBooleanReturns

[Romain PELISSE]

This following code:

public boolean isTrue(Boolean value)
{
boolean result = true;

    if (value.booleanValue())
    {
        return result;
    }
    else
    {
        return ! result;
    }        
}

could be simplify :

public boolean isTrue(Boolean value)
{
return value.booleanValue())
}

A test case has been added to reproduce this bug.

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

can't add the testcase now... i'll do it later...

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hello,

I have a small patch for the SimpleBooleanReturns, which correct this problem.
You can find it there : https://www.08000linux.com/lstm/contribution/patch/101/pmd.patch

Best Regards

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

I added the testcase to the pmd, but it's seems that you patch doesn't correct it. Please, checkout pmd and run $ ant -f bin/build.xml test you'll see:

[concat] Junit test results
...
[concat]
[concat] Test suite DesignRulesTest failed (failures: 0, errors: 1)
[concat]
[concat] Test case unknown failed (failures: 0, errors: 1)
[concat]
[concat] error: "Bug [1843273] False -" failed.

Your patch does not seems to correct this...

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hi,

I think there is a problem in the test case.

There is non definition of a class around the function. If you add one it works ! :)

Best regards

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

Sometime i wish i was a drunk, so i actually have a decent excuse for this kind of stuff... :)

Ok it seems to be working fine now. I'll commit this asap. Thanks for the patch !

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

I commited the fix, thanks !

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Thank you for commiting it :)

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

Hi,

I have to repon this bug. Ryan Gustavson had just commited a new testcase that cause this rules to implode with an NPE.

Would mind check this out if you have the time ?

Thanks !

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hi,

That is a weird test case :)

I'll take a look and try to correct the rule.

Best regards

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

Thanks a lot !

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hi,

The test case that was added is this one :

<expected-problems>0</expected-problems>
<![CDATA
public class Foo {
public void foo() {
if (true) {
return;
} else {
return null;
}
}
}
]>

Should it not fail ? So there should be 1 expected problem.

Regards,

[Ryan Gustafson]

Logged In: YES
user_id=342470
Originator: NO

Sorry, the test case is wrong. I updated it to remove the 'null' in the 2nd return.

<expected-problems>0</expected-problems>
<![CDATA
public class Foo {
public void foo() {
if (true) {
return;
} else {
return;
}
}
}
]>

The point of the test case is to demonstrate how an NPE is occurring. The code is doing a lot of jjtGetChild(0).jjtGetChild(0).... without checking that there is even a child.

A rule should never die for compilable Java code, regardless of how insane that code is.

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hi,

A good night sleep and I (at last) understand what NPE means !

I'll fix the rule ASAP.

Regards,

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

Hi,

Here is the patch : https://www.08000linux.com/lstm/contribution/patch/104/pmd.diff

I have added a few test cases

If a block is like :

if(true|false) {
...
}
else {
...
}

I have added an error

Regards,

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: YES

Thanks for your patch, a sorry for the "we assume that everyone know what NPE means" situation ;) !

I applied and tested your patch without any issue, so i commited it.

Again, thanks for your (quick) contribution.

[Rémy-Christophe Schermesser]

Logged In: YES
user_id=1846409
Originator: NO

No problem !

I you ever had other issues with this rule don't hesitate to yell at me :)

[Xavier Le Vourch]

Logged In: YES
user_id=1373398
Originator: NO

The last commits on this rule are not valid, still a lot of issues with NPE as Ryan indicated. The regress rule also fails.

I'll try to look at the rule later today but extra checks are needed to make sure no exception is raised during the pmd processing as this is not acceptable before the 4.2 release.

The rule should probably be converted to use XPath subexpressions, similar to what's done in UselessOverridingMethod:

type.findChildNodesWithXPath("./Type/ReferenceType/ClassOrInterfaceType[@Image = '" + methodType + "']")

[Ryan Gustafson]

Logged In: YES
user_id=342470
Originator: NO

This NPE dangerous coding style has me thinking of a rule we could implement using Type Resolution.

Foreach non-terminal link in a method call chain
If link method is annotated with CanReturnNull then
Raise Violation
End If
End Foreach

There's two styles code annotation one could use here, CanReturnNull or CannotReturnNull. Depending on the way you wish to sprinkle them in your code, the rule could make the opposite assumption about code lacking the annotation. I prefer the CanReturnNull annotation myself, it's consistent with what people do in @return JavaDocs. Unfortunately CanReturnNull alone can only work if it is used across the entire Java coding universe, and only if used correctly, so it's not likely to happen. :) CannotReturnNull might be the most practical annotation in a highly mixed code base. I'd likly need to have some flexibility in the rule to handle various scenarios.

[Xavier Le Vourch]

Logged In: YES
user_id=1373398
Originator: NO

I fixed the rule so that all test cases now pass and there's no NPE on the pmd and jdk source trees.

I still don't really like the way the rule is written but extra cleanup can be done later on...

We may be ready for a release candidate soon as the "regress" target now works again :)

Xavier

[Nicolas Dordet]

Sorry guys to open again this bug. But I think there is still a problem.
When testing :
public class Foo {
public boolean foo() {
if (true) {
return true;
} else {
return false;
}
}
}
The test find 1 expected problem.ok.

when testing :
public class Foo{
public boolean isTrue()
{
boolean result = true;

    if (true)
    {
        return result;
    }
    else
    {
        return !result;
    }        
}

}
The test fail : find 0 when expecting 1...
I think that the Xpath rule didn't check if a boolean is under a variable...

[Romain PELISSE]

Ndox is right. Sadly I have to reopen this issue.