#551 Rule to require for each style looping rather than iterator

[BlueDevilDan]

I wrote this rule which is pretty close to what is needed, but I think would need to be class based to be perfectly correct to ensure that the variable being iterated is really an iterator. But maybe the xpath information in this would help to kick start that logic.

<rule name="CustomPreferForEachLooping"
since="3.3"
message="An iterating loop that does not need visibility to the iterator should be written using the for each style syntax"
class="net.sourceforge.pmd.rules.XPathRule"

<description>

Explicit iterator looping should only be used when access to the iterator within the loop is required. Valid examples would be to
remove() elements from the list during the iteration, or to check whether this is the final element (hasNext()) to handle some special case logic.
For, While, and Do loops are verified.
</description>
<priority>3</priority>
<properties>
<property name="xpath">
<value>
<![CDATA
//ForStatement
[not(Type)
[Expression/PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.hasNext')]][PrimarySuffix/Arguments[count(child::*)=0]]]
[not(./Statement//PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.remove') or ends-with(@Image,'.hasNext') or ends-with(@Image,'previousIndex') or ends-with(@Image,'nextIndex')]][PrimarySuffix/Arguments[count(child::*)=0]])]
[ancestor::ClassOrInterfaceBodyDeclaration//LocalVariableDeclaration/Type[ends-with(@TypeImage,'Iterator')]]
|
//WhileStatement
[Expression/PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.hasNext')]][PrimarySuffix/Arguments[count(child::*)=0]]]
[not(./Statement//PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.remove') or ends-with(@Image,'.hasNext') or ends-with(@Image,'previousIndex') or ends-with(@Image,'nextIndex')]][PrimarySuffix/Arguments[count(child::*)=0]])]
[ancestor::ClassOrInterfaceBodyDeclaration//LocalVariableDeclaration/Type[ends-with(@TypeImage,'Iterator')]]
|
//DoStatement
[Expression/PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.hasNext')]][PrimarySuffix/Arguments[count(child::*)=0]]]
[not(./Statement//PrimaryExpression[PrimaryPrefix/Name[ends-with(@Image,'.remove') or ends-with(@Image,'.hasNext') or ends-with(@Image,'previousIndex') or ends-with(@Image,'nextIndex')]][PrimarySuffix/Arguments[count(child::*)=0]])]
[ancestor::ClassOrInterfaceBodyDeclaration//LocalVariableDeclaration/Type[ends-with(@TypeImage,'Iterator')]]
]]>
</value>
</property>
</properties>
<!-- This is an explanation of what the Xpath selection is choosing
// All For Loops such that
// Not a for each type loops
// and conditional expression includes a hasNext() method call
// and loop body does not contain a remove(), hasNext(), previousIndex() or nextIndex() call

// All While Loops such that
// and conditional expression includes a hasNext() method call
// and loop body does not contain a remove(), hasNext(), previousIndex() or nextIndex() call

// All Do-While Loops such that
// and conditional expression includes a hasNext() method call
// and loop body does not contain a remove(), hasNext(), previousIndex() or nextIndex() call
//
// also, each checks that there is also a local variable iterator in the same method.
// This is to exclude false positives on looping things like readers that have iterator like interfaces
// This is not fully perfect, ideally would like to compare the name on that variable to the one having
// the hasNext in the loop expression, but that can't easily be done in an XPath.
// This also presumes that method arguments and class members generally won't be iterators as looping
// on those would not be caught. Seems fair enough to reduce more false positives.
-->
<example>
<![CDATA[
List<String> list = new ArrayList<String>();

    // Allowed
    for (String item : list){
        System.out.println(item);
    }

    Iterator<String> iter;

    // Not Allowed
    iter = list.iterator();
    for (;iter.hasNext();){
        System.out.println(iter.next());
    }

    // Allowed
    iter = list.iterator();
    for (;iter.hasNext();){
        iter.remove();
    }

]]>
</example>
</rule>