[Ann] InspectorGeneral: Advanced automatic code improvement tools for IDEA

It would help significantly, if you could provide examples where inspections
will occur and what will happen on invocation.

Especially "jumps to jumps" sounds new to me.

Tom

PS: When we can expect Refactor-J with full Change Type feature?

Hello Tom,

PS: When we can expect Refactor-J with full Change Type feature?

If I understand correctly, that refactoring will appear in Selena.

Some quick comments:
1) Getter used in loop->Hoist getter
This one should automatically make the introduced variable final if the "hoisted"
usages were in an anonymous class context.

2) Parameter 'x' is always cast...
If the parameter comes from an interface/abstract class that is defined in
a library (not part of my source) I don't want to know about it.

For example, Spring MVC has
AbstractCommandController#handle(
HttpServletRequest request,
HttpServletResponse response,
Object command,
BindException errors)

"Object command" is always cast to some user supplied command class.

-tt

We are working on significantly expanding the documentation for InspectorGeneral, including full descriptions of all rules invoked and screencasts of InspectorGeneral in action, but we felt it wiser to ship it rather than waiting on documentation.

"Jumps to Jumps" are a family of rules, simplifying loop and switch statement exit. There are no equivalent inspections in IDEA for this functionality. A simple example would be to simplify

while(true){
//do some stuff,
if(bar){
break;
}
// do some other stuff
}
return baz;

to

while(true){
//do some stuff,
if(bar){
return baz;
}
// do some other stuff
}

More complex examples include conversion of while to do-while, replacing chains of breaks and continues with labeled breaks/continues, and removal of unnecessary control-flag variables.

>When we can expect Refactor-J with full Change Type feature?

We are anticipating a 2.5 release of Refactor-J within the next two months. Full "Change Type" is on the roadmap, along with further improvements to Introduce Parameter Object and Wrap Return Value, and other goodies.

--Sixth and Red River Software

Good ones. We'll get out a fix shortly.

Sixth and Red River Software
"Code with Grace and Verve"

Hello Sixth and Red River Software,

Good ones. We'll get out a fix shortly.

Perhaps (2) should be an config option, not sure..

Some more random feedback:
I did a test drive on some small projects, and so far:
-it "feels" like inspections/intentions, but operatating not on statements,
but on a "wider scope".
-it surprised me a few times, flagging stuff that (while not broken) indeed
deserved attention
-infer nullity could offer to add "annotations.jar" as a library
-when operating only on current file, the transformations available from
the "Code" menu could highlight touched regions (similar to "inline" I think)
-At the moment it feels a bit like shooting in the dark and hoping nothing
breaks - perhaps after a while I'll let it operate on whole packages. Perhaps
some kind of preview could be included for the less courageous?

Regards,
-tt

>infer nullity could offer to add "annotations.jar" as a library
>when operating only on current file, the transformations available from
the "Code" menu could highlight touched regions (similar to "inline" I think)
>At the moment it feels a bit like shooting in the dark and hoping nothing
breaks - perhaps after a while I'll let it operate on whole packages. Perhaps
some kind of preview could be included for the less courageous?

All of these are planned for 2.0 (although the highlighting will be a challenge). Also planned for 2.0 is the option to only operate on the current selection, instead of the current file or directory.

>it "feels" like inspections/intentions, but operatating not on statements,
but on a "wider scope".
>it surprised me a few times, flagging stuff that (while not broken) indeed
deserved attention

We've been using it for the better part of six months now, and it still surprises us.

Sixth and Red River Software
"Code with Grace and Verve"

Fixed in 1.0.1, now available via the plugin manager.

Sixth and Red River Software
"Code with Grace and Verve"

Using Selena #6981, InspectorGeneral 1.0.1
After running mudule-wide analysis, I choose to quickfix multiple occurences
of 'Duplicate call to getter in method' in a single class.
-

Error during dispatching of java.awt.event.InvocationEvent[INVOCATION_DEFAULT,runnable=com.intellij.ui.popup.PopupFactoryImpl$ActionPopupStep$1@1e43461,notifier=null,catchExceptions=false,when=1180220991225]
on sun.awt.windows.WToolkit@9b6976
java.lang.NullPointerException
at com.intellij.refactoring.IntroduceHandlerBase.invoke(IntroduceHandlerBase.java:7)
at com.sixrr.ig.inspections.ib.a(ib.java:13)
at com.sixrr.ig.inspections.ib.applyFix(ib.java:9)
at com.intellij.codeInspection.ex.QuickFixAction$1$1.run(QuickFixAction.java:15)
at com.intellij.openapi.application.impl.ApplicationImpl$13.compute(ApplicationImpl.java:1)
at com.intellij.psi.impl.source.PostprocessReformattingAspect.postponeFormattingInside(PostprocessReformattingAspect.java:84)
at com.intellij.openapi.application.impl.ApplicationImpl.runWriteAction(ApplicationImpl.java:292)
at com.intellij.codeInspection.ex.QuickFixAction$1.run(QuickFixAction.java:1)
at com.intellij.openapi.command.impl.CommandProcessorImpl.executeCommand(CommandProcessorImpl.java:10)
at com.intellij.openapi.command.impl.CommandProcessorImpl.executeCommand(CommandProcessorImpl.java:120)
at com.intellij.codeInspection.ex.QuickFixAction.a(QuickFixAction.java:74)
at com.intellij.codeInspection.ex.QuickFixAction.actionPerformed(QuickFixAction.java:25)
at com.intellij.ui.popup.PopupFactoryImpl$ActionPopupStep$1.run(PopupFactoryImpl.java:2)
at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
at java.awt.EventQueue.dispatchEvent(EventQueue.java:461)
at com.intellij.ide.IdeEventQueue.b(IdeEventQueue.java:159)
at com.intellij.ide.IdeEventQueue.a(IdeEventQueue.java:71)
at com.intellij.ide.IdeEventQueue.dispatchEvent(IdeEventQueue.java:86)
at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java:242)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:163)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:157)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:149)
at java.awt.EventDispatchThread.run(EventDispatchThread.java:110)

Hello Sixth and Red River Software,

Good ones. We'll get out a fix shortly.

In addition, would it be possible to "hoist variables out of a loop"?
Take a variable that's declared and initialized inside of a loop.
I think it might be possible if:
-the initializing expression depends only on variables outside of the loop
-the dependent variables are not mutated inside the loop

-tt

Nice!

Some initial comments:

• "Infer nullity assertions" seems to spend some time trying to infer

assertions for plain text files.

• The status dialog for "Infer nullity assertions" shows the pass

number, but not how many passes there are.

• "Local variable always cast on read access" triggers when you use a

double variable to do some floating-point calculations and then cast it
to int to truncate it to an integer:

double foo = someCalculations();
if (condition) foo += something;
int bar = (int) foo;

In this case the only ways of using the right type from the beginning
seem to be (a) using a ?: construct to inline the condition or (b) using
different variables:

double foo = someCalculations();
int bar;
if (condition) {
bar = (int) (foo + something);
} else {
bar = (int) foo;
}

I'm not sure these alternative versions are better. (But maybe they
are, and I'm aware that these inspections are intentionally constructed
so that they will cover many potential problems at the cost of a few
false positives... just thought I'd mention it anyway to see if this is
a common pattern worth special-casing or whether it's just something one
should live with.)

• "Local variable always cast on read access" triggers on foreach loops

where it is impossible to insert a cast in the actual declaration of the
variable:

for (final Object o : myNonGenericList) {
final String str = (String) o;
...
}

Good one. Should be quite possible. We're already doing most of those calculations for hoisting duplicate statements out of if-then-else statements.

Sixth and Red River Software
"Code with Grace and Verve"

In addition, some error submission functionality would be welcome.

Hm, I guess I'll wait until 2.0 is released. The very first run (only "Rationalize Control Flow")
made my code uncompilable and introduced some oddities and bugs.
Most of all it needs a preview. It is much too much hassle to go through the change set after the fact.
Also there need to be more detailed settings, e.g. I do not want the order of my if/else-clause switched
only because that way a "!=" can be replaced by a "==". I wrote them that way so that regular processing
happens first and error handling in "else" clause.

1)

This code
public int[] getWorkingHours() {
int[] workingHours = {_weekHours1, _weekHours2, _weekHours3, _weekHours4, _weekHours5};
return workingHours;
}
was replaced with this faulty one:
public int[] getWorkingHours() {
return {_weekHours1, _weekHours2, _weekHours3, _weekHours4, _weekHours5};
}

2)
In other places it seems that IG tried to remove redundant variables, but
after replacing the variable with the expression it left the expression in the code.
E.g. this code
boolean equalGuids = _guid.equals(targetTime.getGuid());
return equalGuids;
was replaced by
boolean equalGuids = _guid.equals(targetTime.getGuid());
return _guid.equals(targetTime.getGuid());
Only an annoyance in thbis special case, but in other places it was outright dangerous.
Besides I do want variables for values that are returned (and the Idea inspection has an option to configure that).

3) In several places IG introduced unnecessary parentheses, e.g. it changed
8.0 * partTimeFactor / 100.0;
to
(8.0 * partTimeFactor) / 100.0;

4) IG inlined variables that were used in compound assignment, e.g. vacationDays in
double vacationDays = amount * holidayAccountFactor / workingHoursPerDay;
totalVacationDays += vacationDays;
Idea inspection correctly does not treat such variables as redundant. They give a name to a complex expression.

5) When introducing "return", IG did not remove unnecessary "else", e.g. iot changed
if ( sNo == null ) {
sResult = start == 1 ? sPrefix + "0001" : sPrefix + Integer.toString(start);
}
else {
to
if(sNo == null) {
return start == 1 ? sPrefix + "0001" : sPrefix + Integer.toString(start);
}
else {

6) IG deleted valuable code - it changed this
@AroundInvoke
public Object log(InvocationContext ctx)
throws Exception
{
String target = ctx.getClass().getName() + "." + ctx.getMethod().getName() + "()";

long start = System.currentTimeMillis();
LOGGER.info("Invoking " + target);
try {
return ctx.proceed();
}
catch(Exception e) {
throw e;
}
finally {
long time = System.currentTimeMillis() - start;
LOGGER.info("Method " + target + " took " + time + " ms to execute");
}
}
into
@AroundInvoke
public Object log(InvocationContext ctx)
throws Exception
{
String target = ctx.getClass().getName() + "." + ctx.getMethod().getName() + "()";

long start = System.currentTimeMillis();
LOGGER.info("Invoking " + target);
return ctx.proceed();
}

Jonas,

Thanks for the report. These are all fixed in version 1.0.3, available shortly.

Sixth and Red River Software
"Code with Grace and Verve"

Stephen,

Thanks greatly for the report. 1), 2), 4), and 6) are indeed bugs, and are fixed in version 1.0.3, available shortly. 3) is expected behaviour, intended to increase readability. 5) is interesting. In order to maintain parallel constructions, we only replace assignments at the end of if statements with their following returns if all branches of the if statement end with replaceable assignments. Then, if all of the branches of an if statement return, we don't flatten the "else", again to preserve parallel construction (the existing inspection does this as well). I'm pretty sure this is the readability-optimizing behavior, but it's a close call, and I could be convinced otherwise. If you are seeing other behavior, please let us know. Without knowing more of your code context, it's tough to tell.

We look forward to having preview as well, but were simply unable to fit it into the 1.0 schedule. Hope we haven't lost you completely for a possible 2.0 license.

Sixth and Red River Software
"Code with Grace and Verve"

Sixth and Red River Software wrote:

Sixth and Red River Software is pleased to announce the release of
InspectorGeneral, a suite of automated code improvement and heuristic
bug detection extensions to IntellIJ IDEA.

Great effort, and looking very interesting, although for a product of
this scope I really would suggest running some sort of EAP program
before you release commercially and lock down the license to paying
custies only. It's looking probable that it might not be stable enough
within the first 14 day evaluation period for me, or others, to punt the
cash for this straight away, but I will gladly pay for it when it is
ready to be used in a production environment. The thing is, how will I
know it's stable enough if my evaluation license runs out before this point?

Tighten access modifiers: encapsulation is key to software
development sanity, and InspectorGeneral makes encapsulation trivial
-Annotate classes, methods, and fields with @Override and
@Deprecated, as appropriate

A bug report for you: I just ran this process, and it didn't add
@Override annotations to any overridden methods.

Cheers,
N.

Thanks for the quick answer and fixes.
Concerning issue 5) either I don't understand your answer or you didn't understand the issue.
IG did change the code to
if(sNo == null) {
return start == 1 ? sPrefix + "0001" : sPrefix + Integer.toString(start);
}
else {
but it could have removed the "else" ("unwrapping" the contained block), because after a "return"
there is no point in an "else" clause.

I think your approach of having very few options won't work. For example after using IG I would have
to remove all those unnecessary parentheses (according to our code style) and restore those if/else
where IG negated the condition and flipped the then/else blocks. The first I could do by running
Idea's inspection but that's not possible for the "if"-problem. No way I am going to manually undo
hundreds of these code changes.


Sixth and Red River Software wrote:

Stephen,

Thanks greatly for the report. 1), 2), 4), and 6) are indeed bugs, and are fixed in version 1.0.3, available shortly. 3) is expected behaviour, intended to increase readability. 5) is interesting. In order to maintain parallel constructions, we only replace assignments at the end of if statements with their following returns if all branches of the if statement end with replaceable assignments. Then, if all of the branches of an if statement return, we don't flatten the "else", again to preserve parallel construction (the existing inspection does this as well). I'm pretty sure this is the readability-optimizing behavior, but it's a close call, and I could be convinced otherwise. If you are seeing other behavior, please let us know. Without knowing more of your code context, it's tough to tell.

We look forward to having preview as well, but were simply unable to fit it into the 1.0 schedule. Hope we haven't lost you completely for a possible 2.0 license.

Sixth and Red River Software
"Code with Grace and Verve"

Another nitpick, during 'Tighten Access Modifiers' IG made a protected
final method private, but didn't also remove the final modifier, which
now triggers the "'private' method declared 'final' inspection".

Cheers,
N.

Sixth and Red River Software wrote:

Additionally, InspectorGeneral includes sixteen heuristic code
anomaly detectors, implemented as inspections. Code anomaly
detectors go beyond simple inspections, which find code that is
obviously buggy or breaks style guidelies. Anomaly detectors can
search your code base for the small redundancies and slightly broken
symmetries that indicate worrisome code. Exploiting a deep
understanding of not just how Java works, but how it is commonly
used, InspectorGeneral's anomaly detectors can point you to bugs that
other static analysis tools couldn't possibly detect.

Another request... Could you possibly add an option so that the
duplicate code inspections only count duplicates if they are within the
same scope? For instance, I keep getting the inspection triggered for
getProperty() in cases like this:

if (flag) {
getProperty().setValue(something);
} else {
getProperty() setValue(other);
}

Here I don't really need to extract getProperty() as a local variable as
it is only going to be called once in the method, whichever code path is
followed.

Also, I seem to recall the common consensus is that it is only worth
caching a return value of a property 'get' to a local variable if it is
called more than twice in a given scope (not sure where I read this
though, maybe Effective Java?). So if an option could be added to only
show duplicates above a specified threshold, that would also be very useful.

Cheers,
N.

I've just given it a whirl over a portion of one of our simpler projects and I had the following problems:

- I'm a little confused as to the setting "Prefer early returns" - no matter whether it's ticked or not, it prefers them! Our code standard forces a single return point from methods, so I really need to turn this off.

- The "Tighten access modifiers" inspection is taking an absolute age. In my initial test it ran for 15 minutes and was on its 4th class. I killed IDEA in the end and tried it on a single random class and it took 9 minutes. Also, if you run it on individual classes, you don't get any progress bar and it just looks like the IDE's locked. If this inspection really should take this long to run, it might be worth highlighting this before it starts as it would have to be run overnight for our code-base, or maybe over-month :).

- "Tighten access modifiers" produced uncompilable code, where an abstract class had an abstract method with package visibility, with the implementations' methods also having package visibility. The super class method was changed to protected, but the classes in which the method was implemented remained at package visibility. It also threw a stacktrace - where would you like these reported to?

- The "Normalize References and Declarations" inspection appeared to move a number of files into a state of being "modified", where they were flagged as unsaved by IDEA (showed a little asterisk in the tab) and highlighted as changed against the VCS copy. However, no changes had been made to the files and the only way to revert them back to normal was to restart IDEA.

The "Infer Nullity Assertions" inspection ran fine - in fact it was great!

Just for reference, I'm using IDEA 6.0.5 with JDK1.6U1 on Windows XP.

Sixth and Red River Software wrote:

Normalize references and declarations: a grab-bag of automated code cleanups and improvements, to bring your code to the highest level of polish. One-click improvements include
-Replace fully qualified names with imports
-Remove unnecessary reference qualifiers

Another prob... Normalize references appears to strip off the outer
class qualifier when referring to an inner class member of a foreign
class, e.g.:

Outer.Inner.STATIC_FIELD gets changed to Inner.STATIC_FIELD

This obviously breaks compilation.

Cheers,
N.

ps. it also doesn't add the correct import to the file when replacing
the reference to a static field that is defined in a superclass which is
referred to via a subclass, i.e.:
converting
value = Subclass.STATIC_VARIABLE;
to
value = Superclass.STATIC_VARIABLE;

Cheers,
N.

Nathan Brown wrote:

Sixth and Red River Software wrote:

>> Normalize references and declarations: a grab-bag of automated code
>> cleanups and improvements, to bring your code to the highest level of
>> polish. One-click improvements include
>> -Replace fully qualified names with imports
>> -Remove unnecessary reference qualifiers

Another prob... Normalize references appears to strip off the outer
class qualifier when referring to an inner class member of a foreign
class, e.g.:

Outer.Inner.STATIC_FIELD gets changed to Inner.STATIC_FIELD

This obviously breaks compilation.

Cheers,
N.

I have a scanner class whose getNextToken() and getLookahead() methods
trigger the "call to getter in loop" anomaly detector. Any ideas on how
best to avoid this?

0) Live with it; it's just a heuristic

1) Suppress warnings (but this means ignoring all getters within a lot
of parser loops, not just these two, which may be too strong)

2) Introduce an annotation to add to getNextToken() and getLookahead()
stating that these are not pure ordinary getters

3) Rename them; maybe they shouldn't be called getFoo() if they aren't
pure getters?

4) ...other suggestions?

Stephen,

Okay, I think I understand now. We've loosened up some of the "unnecessary else" logic in 1.0.4 (now available), and it should do what you are looking for. We've also added a configurations option for not adding explanatory parentheses (although that seems a very odd code style requirement), and tightened the logic for when conditions will be flipped. Thanks for the comments.

Sixth and Red River Software
"Code with Grace and Verve"

Fixed in 1.0.4.

Sixth and Red River Software
"Code with Grace and Verve"

I'm a little confused as to the setting "Prefer early returns" - no matter whether it's ticked or not, it prefers them! Our code standard forces a single return point from methods, so I really need to turn this off.

Looks like we missed a couple of edge cases. This should be fixed in 1.0.4, now available.

- The "Tighten access modifiers" inspection is taking an absolute age

That doesn't match what we've seen at all, and we can't seem to reproduce it. Is there anything in the console that might indicate a deeper IDEA problem?

+
"Tighten access modifiers" produced uncompilable code, where an abstract class had an abstract method with package visibility, with the implementations' methods also having package visibility. The super class method was changed to protected, but the classes in which the method was implemented remained at package visibility. It also threw a stacktrace - where would you like these reported to?+

This should be fixed in 1.0.4. Stacktraces can either be reported to this forum, or mailed to support@sixthandredriver.com. We'll be updating our products with automatic reporting functionality over the next couple of months.

Sixth and Red River Software
"Code with Grace and Verve"

For similar issues, we've been relying on combinations of 0, 1, and 3, where appropriate. I like the idea of 2), though, and will be implementing it in an upcoming release.

Sixth and Red River Software
"Code with Grace and Verve"

+Great effort, and looking very interesting, although for a product of
this scope I really would suggest running some sort of EAP program
before you release commercially and lock down the license to paying
custies only.+

In retrospect, this probably would have been a good idea, but we're a bit late now. In recompense, and to reward those who've helped us through this rocky first week, we'd like to award free licenses to you, Taras, Stephen, Simon and Jonas. Drop us a line at sales@sixthandredriver.com specifying a user name, and we'll mail you out a complementary license with our sincere thanks.

As an additional little bonus, we've moved forward some functionality slated for the 2.0 release to 1.0.4. "Optimize String Operations" is now available either via the Code menu or the Checkin panel, automatically improving the structure of String/StringBuffer/StringBuilder operations. Give it a try.

Sixth and Red River Software
"Code with Grace and Verve"

Seems to me Nathan should be included in that list?

"Sixth and Red River Software" <no_reply@jetbrains.com> wrote in message
news:32645120.1180964667393.JavaMail.itn@is.intellij.net...

+Great effort, and looking very interesting, although for a product of
this scope I really would suggest running some sort of EAP program
before you release commercially and lock down the license to paying
custies only.+

>

In retrospect, this probably would have been a good idea, but we're a bit
late now. In recompense, and to reward those who've helped us through
this rocky first week, we'd like to award free licenses to you, Taras,
Stephen, Simon and Jonas. Drop us a line at sales@sixthandredriver.com
specifying a user name, and we'll mail you out a complementary license
with our sincere thanks.

>

As an additional little bonus, we've moved forward some functionality
slated for the 2.0 release to 1.0.4. "Optimize String Operations" is now
available either via the Code menu or the Checkin panel, automatically
improving the structure of String/StringBuffer/StringBuilder operations.
Give it a try.

>

Sixth and Red River Software
"Code with Grace and Verve"