IG - does it find this?

Dave,
Is there an inspection that would catch this bug/unexecutable code?
if (expression) {
// some code
} else if (expression) { // same expression
// some other code
}
I just ran into it.
Thanks,
Jon

19 comments
Comment actions Permalink

Not IG, but IDEA's own inspection called Local Code Analysis | Constant conditions
& exceptions though for very limited cases of expression.

For example:

if (a instanceof JComponent) {
} else if (a instanceof JComponent) {
}

would be catched while
if ( a > 0) {
} else if ( a > 0) {}
won't be.

Anyway, the expression should not have any method calls inside since IDEA
takes a pessimistic assumption methods could return different values even
if called against same parameters.

What was your 'expression' by the way?

Dave,
Is there an inspection that would catch this bug/unexecutable code?
if (expression) {
// some code
} else if (expression) { // same expression
// some other code
}
I just ran into it.
Thanks,
Jon



0
Comment actions Permalink

What was your 'expression' by the way?


The expression was:
(outbound == Outbound.REQUESTS_OUTBOUND)

Jon

0
Comment actions Permalink

I suppose what I was looking for in a new Inspection is a check to see whether the else expression is identical regardless of whether the evaulation of it might change due to multi-threading.

Jon

0
Comment actions Permalink

This one should be catched. Enable that inspection. It mostly reports very
useful stuff indeed.

>> What was your 'expression' by the way?
>>

The expression was:
(outbound == Outbound.REQUESTS_OUTBOUND)
Jon



0
Comment actions Permalink

Hi,

You can find this patterns by structural search with:
if ($condition$) {
$statements$;
} else if ($condition$) {
$statements2$;
}

statements, statements2 occurences 0..MaxInt

Jon Steelman wrote:

I suppose what I was looking for in a new Inspection is a check to see whether the else expression is identical regardless of whether the evaulation of it might change due to multi-threading.

Jon



--
Maxim Mossienko
JetBrains, Inc
http://www.jetbrains.com
"Develop with pleasure!"

0
Comment actions Permalink

Hmm, doesn't seem to be working. I enabled it from Errors > Local Code Analysis > Constant conditions & exception. Tried it both severities just in case. I'm on 3177.


Thanks,
Jon

0
Comment actions Permalink

And just for a sanity check, I confirmed that your example is caught by the Inspection.

0
Comment actions Permalink

Is that Outbound.REQUESTS_OUTBOUND a final static field? If it isn't final
its value could be changed by another thread we're trying to be aware of.
The other probable reason could be the method is a way to long/complex to
be analyzed. We're breaking the analysis if it takes too much time.

Hmm, doesn't seem to be working. I enabled it from Errors > Local Code
Analysis > Constant conditions & exception. Tried it both severities
just in case. I'm on 3177.

 if (outbound == Outbound.REQUESTS_OUTBOUND) {
> OUTBOUND_MESSAGE_TYPES = REQUEST_TYPE_MAP;
> } else if (outbound == Outbound.REQUESTS_OUTBOUND) {
> OUTBOUND_MESSAGE_TYPES = RESPONSE_TYPES_BY_ACTION;
> }
> ]]>


Thanks,
Jon



0
Comment actions Permalink

Outbound is an enum and REQUESTS_OUTBOUND is one of its enums.

Jon

0
Comment actions Permalink

Thanks a lot! It seem we missed this particular role of constants when enums
appear in 1.5. To be fixed really soon.

Outbound is an enum and REQUESTS_OUTBOUND is one of its enums.

Jon



0
Comment actions Permalink

Maxim Shafirov wrote:
> For example:
>
> if (a instanceof JComponent) {
> } else if (a instanceof JComponent) {
> }
>
> would be catched while
> if ( a > 0) {
> } else if ( a > 0) {} won't be.

Ok, here's my stupid question for today. Where's the difference between

(a > 0) and (a == 0) ?

Would it be possible to have an option that always flags identical conditions
in if/else if branches, regardless of threading and side effect issues? False
positives shouldn't be an issue with the new possibilities to suppress certain
inspections, but this would greatly help to identify bad code as well as code
smells.

Sascha

0
Comment actions Permalink

identical conditions in if/else if branches


This particual inspection's algoritm is just much smarter than that. It doesn't
check conditions for equality but tries to evaluate as many as possible paths
of the control flow, which lead to the different states of the JVM memory.
For example:

int x;

if (someExpr) {
x = 2;
}
else {
x = 3;
}

if (x == 4) {
}

'x == 4' condition will be identified as always false cause there's no paths
of execution goes into the if ever.
>, <, >=, <= conditions are just a bit harder to implement in this respect
since they require interval logic memory states.

Max

0
Comment actions Permalink

Fixed.

Outbound is an enum and REQUESTS_OUTBOUND is one of its enums.

Jon



0
Comment actions Permalink

Maxim Shafirov wrote:

This particual inspection's algoritm is just much smarter than that. It
doesn't check conditions for equality but tries to evaluate as many as
possible paths of the control flow, which lead to the different states
of the JVM memory. For example:


I knew about that one,

, <, >=, <= conditions are just a bit harder to implement in this respect


since they require interval logic memory states.


but didn't know about the algorithm and the implications behind it. Thanks for
the explanation.

Anyway, don't you think that an additional inspection that just detects identical
if-conditions would make sense (even though it wouldn't be as smart as this one)?

Sascha

0
Comment actions Permalink

Anyway, don't you think that an additional inspection that just
detects identical if-conditions would make sense (even though it
wouldn't be as smart as this one)?

Makes sense. Really simple one.

0
Comment actions Permalink

You're welcome. I look forward to the fix. What release # has it?

Thanks,
Jon

P.S. I appreciate the high level of interaction that has occurred in this thread.

0
Comment actions Permalink

Sascha,

Are you going to create an SCR or JIRA entry for this so Maxim can track it conveniently?

Jon

0
Comment actions Permalink

Don't bother, I've already coded it up. Can't ship it, because the've moved my repository to subversion, and I can't get the subversion integration to work, but that's the sort of thing that makes EAP such an adventure.

--Dave Griffith

0

Please sign in to leave a comment.