#591 False + PositionLiteralsFirstInComparisons

[Ryan Gustafson]

This rule triggers a false positive in cases where
there is a literal value used as an array offset. For
example:

public class Test
{
void method()
{
if (someValue.equals(otherValue[0]))
{
}
}
}

Sorry, I've no suggested fix for it. :( I've tested
this snippet in the designer, and there's nothing
obvious to me in the AST that makes it clear it's an
array index usage. I can also imagine other forms of
literal usages in arbitrary expressions which result in
false positives. It's not clear to me what XPath
would do the trick.

One could try to nail down the match even further,
perhaps instead of a loose match of [..//Literal], one
could use the fully qualified AST subtree matches the
simple foo.equals(literal) expression. This would mean
the rule match one and only one type of usage. Dunno
if that's to strict.

For now, I'll be changing my code to avoid the use of
the literal within the if statement.

gus

[Tom Copeland]

Logged In: YES
user_id=5159
Originator: NO

Assigning away from me in case someone else can look into this...
Tom

[Ryan Gustafson]

Logged In: YES
user_id=342470
Originator: YES

Encountered another false positive yesterday, of the form:

public class Test
{
public static final String CONSTANT = "constant";
public static void main(String[] args)
{
if (CONSTANT.equals(someMethod("literal")))
{
}
}
private static String someMethod(String s)
{
return s;
}
}

After thinking about this some more, one way to make it tighter would be to have a way of identifying when a given expression "collapses" into a literal. And then, it should be a literal of a particular type (String, int, double, etc).

I believe a String literal is in psuedo-BNF:
literal_simple := "some string"
literal_parenthetical := '(' literal ')'
literal_add := literal '+' literal
literal = literal_simple | literal_parenthetical | literal_add

For example, these are String literals:
"a"
("a")
"a" + "b"
("a") + ("a" + "b")

These are not String literals:

"a" + methodCall("b")
array[0]

It's not clear to me how to do such a complex check using XPath alone. I'm sure a Java code could be written to do it. Would one do it using an XPath extension function, or just replace the XPath rule with a Java rule? I'm also curious as to whether we could get this information as extra data from AST Expression nodes, such that XPath rules could use it? Could it be determined while building the AST easily, or would it require another visit pass over the AST? Food for thought.

gus

[Tom Copeland]

Logged In: YES
user_id=5159
Originator: NO

Hi Ryan -

Your comment is right on. It'd be great if we would modify PMD so that it extracted all available type node information. For example, the StringBuffer rules for catching StringBuffer.append("x") don't catch this:

((StringBuffer)foo).append("x");

But all the necessary type information is there; we "just" need to calculate the type and put it up higher in the AST, probably in the Expression node.

So yes, this is a good idea, and no, it's not in PMD, and yes, it'd be great to move towards having it there :-)

Yours,

tom

[Ryan Gustafson]

Logged In: YES
user_id=342470
Originator: YES

Tom, is this related to the type resolution stuff I'm reading on the dev forums? Or is it an orthogonal feature that could be added to PMD (and is not currently in progress)? --Ryan

[Tom Copeland]

Logged In: YES
user_id=5159
Originator: NO

Hi Ryan -

Hm, I think that's mostly about just resolving the types and not so much about evaluating expressions. It does kind of tend to come together though, since once we know the type we can check to see if type.getFoo() returns a StringBuffer, stuff like that.

Yours,

Tom

[Ryan Gustafson]

Logged In: YES
user_id=342470
Originator: YES

Hi Tom,

Ok then, I'll spend some time trying to come up with a solution for this. At a minimum I believe we can get type information on expressions which are composed of compile time constants and certain known Java operations (e.g. + operation between a known String and anything else). Full expression type evaluation though would require knowing return types of methods, etc. which would use the type resolution stuff (if I understand it correctly).

Am I on the right track here? This is a practical improvement to PMD that someone could work on?

Ryan

[Tom Copeland]

Logged In: YES
user_id=5159
Originator: NO

Hi Ryan -

That'd be great, and yup, I think there's a way this can be done partially to catch some of the simple cases. I'm sure there's some sort of compiler theory way of doing this sort of thing, but I don't know what it is :-)

Full expression type evaluation though would
require knowing return types of methods, etc.
which would use the type resolution stuff
(if I understand it correctly).

Exactly.

Am I on the right track here? This is a
practical improvement to PMD that
someone could work on?

Yup, and it can be done somewhat independently of the type resolution stuff too, which is nice. It's kind of like the data flow analysis layer; we can't do interprocedural DFA without type resolution, but we can do intraprocedural DFA to catch things like:

==========================
int x = 2;
// lots of code not using x
x = 5; // first assignment was wasted!
foo(x);
==========================

By the way, we should probably move further discussions to the dev forum so's other folks can chime in,

Yours,

tom