Irida feedback on Inspections / Error Highlighting (as of 3235)

I have trying out the new enhancements to Inspect Code/
Error Hilighting heavily.

Overall, everything has been working great.
The biggest problem I know of in build 3235 is that the
missorted modifiers quickfix doesn't work when applied en
masse from inspeciton results window.

I have filed a few enhancements on the inspection results
window.

IDEADEV-270 Inspection Results window should display
full description when clicking on Inspection
name

IDEADEV-269 Inspection Results should have "Edit Options"
choice for each inspection.

IDEADEV-265 Shortcut for running Inspect Code on current
file with current ErrorHilighting Profile

I will make additional posts on comments/questions/ideas
about individual inspections.

22 comments
Comment actions Permalink

I see Local Code Analysis->Suspicious collections method
calls, "Under Construction"

Is this only for non-parametized collections?
Doesn't parametized collections already catch most of these
problems? For example:

// parametized, compiler catches the problems
{
Map map = new HashMap(); map.put(new Integer(0),new String("asdf")); map.put(new Integer(0),new Integer(0)); //error for (Iterator i = map.keySet().iterator(); i.hasNext(); ) { Double d = (Double)i.next(); // error } Long l = (Long) map.get("asdfdsf"); // error } // non-parametized, compiler useless { Map map = new HashMap(); map.put(new Integer(0),new String("asdf")); map.put(new Integer(0),new Integer(0)); for (Iterator i = map.keySet().iterator(); i.hasNext(); ) { Double d = (Double)i.next(); } Long l = (Long) map.get("asdfdsf"); } It would be great if the inspection uses the same algorithm from Refactor->Generify does to determine the type(s) of the objects stored versus the type(s) of objects retrieved, and then flag suspicious/errors from that. This inspection would be very helpful in a JDK 1.4 codebase. I dont' know how many times I've seen errors related to this, esp. with non-trival data structures like Map>>]]>.

One related bug I have squashed multiple times is
accidentially calling add() to add a Collection instead of
addAll(). (This is in JDK 1.4 codebase)

0
Comment actions Permalink

How about an inspection to move the declaration of
variables as closeas possible smallest scope possible. Call
it "Local Code Analysys -> Local Variable declaration scope
wider than necessary"

I sometimes stumble onto code like this:

Vector v = null;
Integer id = null;
Object data = null;
..
..
..
for (...) {
id = ...;
v = ...;
for (...) {
data = ...;
}
}

Some people have this code style where they declare all
local variables at the top of the method which I find annoying!

Quickfix could change it to:
for (...) {
Integer id = ...;
Vector v = ...;
for (...) {
Object data = ...;
}
}


It would be nice if Quickfix had special logic
for for/while loops. It could reduce the scope
of a local iterator or loop counter by
converting while loop to a for loop.

Iterator i = collection.iterator();
while (i.hasNext()) {
Object o = (Object) i.next();
}

to:
(JDK1.4)
for (Iterator i = collection.iterator(); i.hasNext(); ) {
Object o = (Object) i.next();
}

or:
(JDK1.5)
foreach (Object o: collection) {
...
}

Actually, the JDK 1.5 case is different because the iterator
is refactored out. (Scope reduced to nothingness). This last
one overlaps with the inspection Verbose or redundant code
constructs->'for' loop may be replaced by 'for each' (
J2SDK 5.0 only)

I like to reduce the scope of the iterator variable so that
I can reuset the variable name when I do several multiple
iterations in the same scope,e.g.

for (Iterator i = collection1.iterator(); i.hasNext(); ) {
Object o = (Object) i.next();
}

for (Iterator i = collection2.iterator(); i.hasNext(); ) {
Object o = (Object) i.next();
}

0
Comment actions Permalink

Related to last post, I found that when I'm in JDK 5.0
codebase, that the inspection 'for' loop replacable by
'for each' (J2SDK 5.0 only) does not handle the most
common form of iteration/enumeration on a collection.


For example:

static void test(Collection c) {

// case 1
Iterator iterator = c.iterator();
//...possible statements before while....
while (iterator.hasNext()) {
Object o = (Object) iterator.next();
}

// case 2
for (Iterator i = c.iterator(); i.hasNext(); ) {
Object o = (Object) i.next();
}
}

In code I have worked on, case 1 is most common for
iterating/enumerating over a colleciton, although I
prefer the case 2 idiom.
Unfortunately, the inspection will only convert the
case 2 to the new JDK 5.0 "for each" construct.

The one twist in case 1 is that in order to convert it to
a 'for each', you need to first make sure you can reduce
the scope of the Iterator. It could be referenced
somewhere else in the block... although if it was smart
it could even handle cases like:

Iterator iterator = c.iterator();
//...possible statements before while....
while (iterator.hasNext()) {
Object o = (Object) iterator.next();
}

// reusing the iterator
iterator = c.iterator();
//...possible statements before while....
while (iterator.hasNext()) {
Object o = (Object) iterator.next();
}

This inspection would be much more useful if it
handled the while loop case. That code is very
common in our codebase, even more common than
a regular array iteration.


0
Comment actions Permalink


Is it possible to provide inspection to flag unnecessary
checking for null?

I have been working on some code which is driving me crazy
because there are checks for null everywhere and each many
of the methods return null in some cases. The person who
wrote this code hasn't heard of the NullObject pattern.

public static Collection test(Collection data) {

if (null == data && data.size() == 0) {
return null;
}
// ...
Collection result = new ArrayList();
for (Iterator i = c.iterator(); i.hasNext(); ) {
Integer value = (Integer) i.next();
Integer newValue = Calculate(value);
if (newValue != null) {
result.add(newValue);
}
}
return result;
}

I want to remove the checks for null, but in order to do it
I have to find all usages of the method, then find all
usages of the Collection parameter being passed into the
function and make sure it can never be set to null.

It would be nice if there was an inspection that could do
that with a quickfix to remove the unnecessary check for
null.

I would like to change the code so it looks like this:

public static Collection test(Collection data) {
Collection result = new ArrayList();
for (Iterator i = c.iterator(); i.hasNext(); ) {
Integer value = (Integer) i.next();
Integer newValue = Calculate(value);
result.add(newValue);
}
return result;
}

0
Comment actions Permalink

Naming conventions

I think the minimum and maximum lengths
should only apply to the dynamic part of the identifier,
e.g. for name "m_id", the dynamic part is "id".
If you do that, then a minimum length of 2 makes sense.

I would change all the minimum lengths to 2
except local variables stay at 1.
Also I would change the maximum lengths to 64.

In my own experience, I haven't found anything useful from
the minimum and maximum length warnings, except in one class
where a class has single letter field identifiers, e.g. "i"
and "s". That was the only time I changed the code. The rest
of the time it was just noise, esp. with the minimum length
of 8 for class and interface names.

Also, I think you should allow variables, constants, and
methods to be alphanumeric by default. e.g.

[A-Za-z0-9] instead of [A-Za-z]

Several times the warning has popped up for things like
"row1" and "row2" which seem perfectly fine to me. As long
as the first letter doesn't start with a number. Classes and
Interfaces shouldn't have numbers in general, so leave them
as is.

Here are the current defaults for your reference.

Class naming convention
8-64 [A-Za-z]*

Constant naming convention
5-32 *

Instance method naming convention
4-32 [A-Za-z]*

Instance variable naming convention
5-32 m_[a-z][A-Za-z]*

Interface naming convention
8-64 [A-Za-z]*

Local vairable naming convention
1-20

Method parameter naming convention
1-20 [A-Za-z]*

static method naming convention
4-32 [A-Za-z]*

Static variable naming convention
5-32 s_[a-z][A-za-z]*

Suggested Defaults:

Class naming convention
2-64 [A-Za-z]*

Constant naming convention
2-64 *

Instance method naming convention
2-64 [A-Za-z0-9]*

Instance variable naming convention
2-64 m_[a-z][A-Za-z0-9]*

Interface naming convention
2-64 [A-Za-z]*

Local variable naming convention
1-32 [A-Za-z0-9]*

Method parameter naming convention
1-32 [A-Za-z0-9]*

static method naming convention
2-64 [A-Za-z0-9]*

Static variable naming convention
2-64 s_[a-z][A-za-z0-9]*

0
Comment actions Permalink

I had tried the "Standard variable name inspection".
Here is the description:

"This inspection reports on any variables with 'standard'
names which are of unexpected types. Such names may be
confusing. Standard names and types are as follows:

i, j, k, m, n - int
f - float
d - double
b - byte
c, ch - char
l - long
s, str - String"

I didn't find this too useful because there were so many
exceptions, and since I couldn't customize it, I turned it
off.

But this got me thinking about a related problem. I find it
harder to read code where someone uses a long name for the
iterator or enumeration of a Collection class, e.g.

List dataL;
for (Iterator dataIterator = dataL.iterator(); dataIterator.hasNext(); ) {
Object o = (Object) dataIterator.next();
}

I prefer to use simple names for the Iterator,e.g.

for (Iterator i = collections.iterator(); i.hasNext(); ) {
Object o = (Object) i.next();
}

It doesn't make sense to give some long name to the Iterator
or Enumeration. In JDK 5.0, the Iterator/Enumeration is
factored out completely with the "for each" statement.

I'm not sure if this is an inspection or a refactoring,
but it would be nice to be able to make a list of standard
names for Iterator and Enumeration,e.g.

Iterator i,j,k,m,n
Enumeration e,f,g,h

And then have an inspection/refactoring be able to change
all Iterators and Enumerations to use those names. In the
case of nested Iterators/Enumerations, the names would be
used in order, e.g. i then j then k, etc..

You could also do it for int counters in a for loop,
but I would only do it for integers used as loop counters, e.
g.
int[] array = new int[100];
for (int index=0,selected=0; index < array.length; index++) {
//...
}
I would only want to change the name of "index" to "i". The
rule would be if the int variable was declared in the for
loop initialization statement, compared in the exit
condition statment, and updated in the for loop update
statement, plus not updated in the body of the for loop.

0
Comment actions Permalink

I noticed that when editing the Inspection&Highlighting
Profiles from the Errors panel in options dialog, it
doesn't show inspections which are only avaiable in
Tools->Inspect Code.

For example, General->Unused declaration is only for
Inspections while Error Hilighting has more Unused symbol.

I would rather see all of them no matter what. Just mark
the Inspect-Only with an inspection icon and the
ErrorHighlighting-Only with a different icon. Or I guess
you could just disable the ones that aren't applicable in
the current context (but still letting use look at the
description for them.)

I would prefer to just be able to always edit any of the
inspections with the understanding that only the applicable
ones will be applied.



Attachment(s):
intellij_error_options_profile_filter_unused.gif
intellij_inspect_code_profile_filter_unused.gif
0
Comment actions Permalink

I agree that replacing the while loop case would be very useful, and it shouldn't be any more difficult to code the transformation that the current for-loop case. That said, the for-loop case is about a thousand lines of snarling code checking a truly vast number of edge cases, so don't expect anything soon. "Convert while to for-each" is now on my TODO list.

--Dave Griffith

0
Comment actions Permalink

I believe this inspection is to catch problems with the few non-genericized methods left on the collections API. Set.remove(), for instance, takes an arbitrary Object, irrespective of the type parameter of the Set. Thus it is legal to remove an object of type Bar from a Set]]>, even if Foo is not assignable from Bar. This inspection is meant to catch stuff like that. (Note that this is a core IDEA inspection, not InspectionGadgets, so anything I say is not definitive.)

--Dave Griffith

0
Comment actions Permalink

I agree that the naming conventions inspections aren't that useful, and I normally run with them turned off. They were mostly included for completeness (all the other static analysis products out there include them) and because naming conventions often show up in internal coding standards.

I tend to agree with you about numbers in names, but only find them acceptable at the end, so instead of


Local variable naming convention
1-32 [A-Za-z0-9]*

I would probably weaken it to

Local variable naming convention
1-32 [A-Za-z]

Thoughts?

--Dave Griffith

0
Comment actions Permalink


>Overall, everything has been working great.
The biggest problem I know of in build 3235 is that the
missorted modifiers quickfix doesn't work when applied en
masse from inspeciton results window.

Working on it, but it's surprisingly tricky to get right with the current API. Hope for some success within a couple of EAP cycles.

--Dave Griffith

0
Comment actions Permalink


A "Reduce variable scope" refactoring is supposedly scheduled for Irida, but I too would love to see an inspection. I've suggested this, and a few other dataflow-oriented inspections http://jetbrains.net/jira/browse/IDEA-547.

--Dave Griffith

0
Comment actions Permalink


Making the "Standard variable names" configurable is on my TODO list. I like your idea of an "Acceptable variable names" inspection as well, and will put it on the TODO list.

--Dave Griffith

0
Comment actions Permalink

Dave

>The biggest problem I know of in build 3235 is that the
>missorted modifiers quickfix doesn't work when applied en
>masse from inspeciton results window.
>

>

It doesn't work either when applied on 1 single error, from the same
inspection result windows.

Alain

0
Comment actions Permalink

I'd like to explicitly allow/disallow the underscore. Or even better: It
would be nice if I had a quick fix to rename "foo_bar" to "fooBar". Yet
some other coding conventions might want just the reverse ...

Mike

I agree that the naming conventions inspections aren't that useful, and I normally run with them turned off. They were mostly included for completeness (all the other static analysis products out there include them) and because naming conventions often show up in internal coding standards.

I tend to agree with you about numbers in names, but only find them acceptable at the end, so instead of


Local variable naming convention
1-32 [A-Za-z0-9]*

I would probably weaken it to

Local variable naming convention
1-32 [A-Za-z]

Thoughts?

--Dave Griffith

0
Comment actions Permalink


Alain Ravet wrote:

Dave

>
>>The biggest problem I know of in build 3235 is that the
>>missorted modifiers quickfix doesn't work when applied en
>>masse from inspeciton results window.


It doesn't work either when applied on 1 single error, from the same
inspection result windows.


Doesn't work for me in the editor either.
http://www.intellij.net/tracker/idea/viewSCR?publicId=43854

Bas

0
Comment actions Permalink


Keith Lea has been working on something like this. He uses annotations to flag parameters and method returns as @NotNull, and then uses those annotations to drive inspections and generated checking code. Don't know where he is on it.

Without annotations, the remove null-checking problem probably is too hard for general solution.

--Dave Griffith

0
Comment actions Permalink

From IDEA-547:

"Local variable may be moved to first use: Trigger this
when an uninitialized local or local initialized to a
constant is declared before use. As a quickfix, move it's
declaration as close to the first use as possible."

Yes, this is exactly what I want.

Whenever I come upon code like this, it is all over the
place. I don't what to do a refactor on each variable in a
10 or 20 page source file. I want to do Inspect Code->File,
and then quick fix all of the code with a couple clicks.

0
Comment actions Permalink

I don't think it is worth the effort to make "Standard
variable names" configurable.

Even if it was configurable, I'm not sure any of it makes
sense. There are too many exceptions to the rules.

i, j, k, m, n - int
f - float
d - double
b - byte
c, ch - char
l - long
s, str - String

For example,

Side a =...;
Side b = ...;
compareSides(a,b);

Collection c = ...;
result.addAll(c);

for (Iterator i = collection.iterator(); i.hasNext(); ) {
...
}

I can see if you have a variable named "str" or "string"
and it isn't type String, then that is bad.

I've seen code where people changed the data structure from
Vector to ArrayList, but left the variable name "v" or "
vector" or "dataV", etc. But I'm not sure I want an
inspection to try to detect these cases. In order to do
that you would need regex like:

ArrayList v, vector, .?V, .?Vector

I'm not sure how useful something like that would be,
esp. because I'm sure there wouldn't be a quickfix for that.

The Iterator / Enumeration / for loop counter standard name
inspection is more appealing because there shouldn't be any
exceptions and quickfix can be applied using a simply list
of names.

I was trying to think of other classes where generic names
can be applied, and the only other one I can think of is
Exception names in try{}catch{} blocks.

0
Comment actions Permalink

Actually, I find the naming inspections very useful
(except for class names and interface names because
usually there aren't any problems there).
It is just the length check I don't find very useful,
so I was suggesting you loosen that check.

Re: numbers only at the end. I think it is better to keep
the regex simple and go with [A-Za-z0-9]*

I mentioned row1, row2. Well, the same file has row1Style
and row2Style.

I think the main purpose of the name check is to find cases
where names are not following the naming convention, e.g.
if the convention is m[a-z][A-Za-z0-9], then flag m_name,
mName, name, etc. as errors.

It is a lesser problem if the name has a number in it. I
would probably only raise an eyebrow if the name had two
numbers in it, e.g. row99 or data12. But I'm not sure how
you specify that in a regex or even if it's worth doing it.

Actually, that reminds me I have seen some autogenerated
gui could which had names like label1 up to label56, panel1
up to panel10, etc. But that kind of file I don't want to
report inspections on anyway.

In summary, the name inspections are checking a number of
things all at once:

(1) checking that name starts with lowercase or uppercase character
(2) checking that name starts with prefix (if specified)
(3) checking that name contains only reocommended characters
(4) checking that name's length is between minimum and maximum lengths

My main point is that (1) and (2) are much more important. The
most important part of (3) is catching places where underscore ('_')
is used. The use of numbers isn't that big a deal because it is
usually rare and usually done for a reason. (4) is not very
important.

The name inspections would become much more useful if they
had quickfixes to make the names conform to the naming
convention.

I have some ideas about that and will make a separate
post on that.

0
Comment actions Permalink

There already is such an inspection, "Constant conditions & exceptions."
It will highlight any boolean values which will always evaluate to the same
value. You say you want to remove "unnecessary" null checks, but all of the
null checks in your example appear to be necessary; the compiler cannot tell,
without interprocedural analysis, something Java is designed to avoid, whether
the values being checked may be null.

Structural Search & Replace will probably help you with this.

I'm working on a plugin which uses Java 5.0 annotations to deal with this
problem by annotating which values can and cannot be null, without the need
for explicit checks.

-Keith

Is it possible to provide inspection to flag unnecessary checking for
null?

I have been working on some code which is driving me crazy because
there are checks for null everywhere and each many of the methods
return null in some cases. The person who wrote this code hasn't heard
of the NullObject pattern.

public static Collection test(Collection data) {

if (null == data && data.size() == 0) {
return null;
}
// ...
Collection result = new ArrayList();
for (Iterator i = c.iterator(); i.hasNext(); ) {
Integer value = (Integer) i.next();
Integer newValue = Calculate(value);
if (newValue != null) {
result.add(newValue);
}
}
return result;
}
I want to remove the checks for null, but in order to do it I have to
find all usages of the method, then find all usages of the Collection
parameter being passed into the function and make sure it can never be
set to null.

It would be nice if there was an inspection that could do that with a
quickfix to remove the unnecessary check for null.

I would like to change the code so it looks like this:

public static Collection test(Collection data) {
Collection result = new ArrayList();
for (Iterator i = c.iterator(); i.hasNext(); ) {
Integer value = (Integer) i.next();
Integer newValue = Calculate(value);
result.add(newValue);
}
return result;
}




0
Comment actions Permalink

Keith Lea has been working on something like this. He uses
annotations to flag parameters and method returns as @NotNull, and
then uses those annotations to drive inspections and generated
checking code. Don't know where he is on it.


Right now it's very limited; it only works on method parameters. I haven't
had much time to work on it, but it's part of a school project, so either
it will be done by May or I won't graduate. :)

-Keith


0

Please sign in to leave a comment.