Request for comment: Default enablement for InspectionGadget inspections

Oh, yeah, I should say that I particularly want to hear comments from JetBrains on this one. Thanks.

--Dave Griffith

I'd also added the following to the list:

- C-style array declaration
- Boolean constructor call
- Inner class may be static
- Non-final field referenced in hashCode()
- Result of method call ignored (with its default configuration)
- Statement with empty body
- String comparison using ==, instead of .equals()
- Field accessed in boh synchronized and unsynchronized contexts
- Synchronization on a non-final field
- Octal integer

The things I do use myself which wouldn't hirt anyone.

-

Maxim Shafirov
http://www.jetbrains.com
"Develop with pleasure!"

I would also vote for these to be enabled by default:

Empty 'catch' block
Empty 'finally' block
Empty 'try' block
Instantiating object to get Class object
Redundant String constructor call
Redundant String.toString()
Class explicitly extends java.lang.Object

For their educative qualities.

Personally I don't use the 'expression.equals("literal") rather than
"literal".equals(expression)' inspection. In almost all cases any
NullPointerExceptions which occur point to a more severe problem
(expression should never have been null) that I know about sooner
because of the NPE.

Bas

You're the man, but I would disagree on a couple of these:

- Non-final field referenced in hashCode() (and presumably the same inspections for equals() and compareTo())

Unfortunately, Serializable classes that implement their own serialization methods can't really mark their fields as final, but may very well need their own hashCode/equals semantics. That one's caught me enough that I usually run with this off. OTOH, those inspections will find some subtle and horrible bugs, so those might be worthwhile.

-Result of method call ignored

Good one

- C-style array declaration

I was skipping all purely stylistic issues, but if I include any this would be it.

- Field accessed synced and unsynced

Way ballsy to put that one on my default, but it would prevent more threading errors than just about anything.

--Dave Griffith

You're the man, but I would disagree on a couple of these:

- Non-final field referenced in hashCode() (and presumably the same inspections for equals() and compareTo())

Unfortunately, Serializable classes that implement their own serialization methods can't really mark their fields as final, but may very well need their own hashCode/equals semantics. That one's caught me enough that I usually run with this off. OTOH, those inspections will find some subtle and horrible bugs, so those might be worthwhile.

-Result of method call ignored

Good one

- C-style array declaration

I was skipping all purely stylistic issues, but if I include any this would be it.

- Field accessed synced and unsynced

Way ballsy to put that one on my default, but it would prevent more threading errors than just about anything.

--Dave Griffith

Inspections enabled by default:
expression.equals("literal") rather than "literal".equals(expression)

I wouldn't like this. I don't think it's good style. I don't think IDEA should
be promoting what I think is bad style. I understand others think the opposite,
but this is how I feel.

Missorted modifers

I don't think most people put "synchronized" where the JLS suggests.

Manual array copy

I think this is an okay choice, but keep in mind that System.arrayCopy is
slower than a manual copy for small arrays, in pre-5.0 java, I believe.

Infinite loop statement

Do you think this will clash or seem redundant with "unreachable code" highlighting?

Static method referenced via subclass

I disagree with this one; without thinking too hard about it, I think referencing
static methods via subclass is a part of good class design / inheritance.
Do you know if, in the generated bytecode, the subclass is referenced, or
the superclass? If it's the subclass, then I don't see what the problem is.

Unnecessary label on 'break' statement
Unnecessary label on 'continue' statement

I think these two will annoy people. Maybe an unnecessary label is for clarity.

-Keith

>> expression.equals("literal") rather than "literal".equals(expression)

I wouldn't like this. I don't think it's good style. I don't think
IDEA should be promoting what I think is bad style.

I understand the reasoning behind this is mostly that the first expression
may throw NPE's and the second never does. Only code and no style behind
this at all.

I started using the second instead of the first sometime ago, after first
seeing this inspection in idea :)

>> Static method referenced via subclass
>>

Do you know if, in the generated bytecode, the subclass
is referenced, or the superclass? If it's the subclass, then I don't
see what the problem is.

Resolved at compile time: the superclass is always used. This is the same
class of problem as using objectReference.staticMethod().



I don't mind anything in the list.

IMHO (and if I didn't suspect that this would make idea unusable spending
all of it's time inspecting) I would enable by default almost every problem
detecting inspection and style inspection.

If the user doesn't like an inspection he can turn it off, and (after request
idea-1064 is implemented) by turning it off learn where and how inspections
are configurated.

Dave Griffith wrote:

Inspections enabled by default:

Yay! Any improvement in this area is greatly welcome. I wonder if we
should all just submit our editor.codeinsight.xml files and run some
statistics on them, looking for popular on/off settings, and maybe
discussing the 'controversial' ones (then again, maybe NOT discussing
the controversial ones... hehe ;-).

--
Rob Harwood
Software Developer
JetBrains Inc.
http://www.jetbrains.com
"Develop with pleasure!"

Missorted modifers

No! This has no impact on anything, so why not allow people to use whatever order they feel is natural to them?

Static method referenced via subclass

pretty common pattern, so enforcing this would be quite mean.

I never thought the day would come where I'd disagree with maxim on anything, but here goes ;)

- C-style array declaration

Why? Personal style issue.

- Boolean constructor call

Only for JDK 1.3 targetted projects. I'd really like things that are JDK specific to have text in the warning to point out that non-backward compatibility.

- Result of method call ignored (with its default
configuration)

Somewhat annoying. If you're ignoring a result statement then chances are you don't actually care about the return value. It's not the kind of thing that happens by mistake, after all, you'd very quickly notice in the next few lines that there's a value you care about that doesn't seem to exist as a variable!

- Statement with empty body
- String comparison using ==, instead of .equals()

Nope, this makes anything that uses intern be flagged as warnings, not cool for the performance minded.

- Field accessed in boh synchronized and
unsynchronized contexts
- Synchronization on a non-final field

Too esoteric

- Octal integer

Why? It's a perfectly legitimate language construct. It's not common sure, but if someone goes out of their way to use it, chances are they know what they're doing.

>> Static method referenced via subclass

>pretty common pattern, so enforcing this would be quite mean.

Can't say I've ever actually seen this in the wild. This isn't accessing a static in a superclass in a subclass without qualifying it. That's just fine. This is accessing a static in a superclass by qualifing it with a subclasses name.

--Dave Griffith

My cents:

• Class name differs from file name

• Constructor not protected in abstract class

• Static inheritance

• All Cloning inspections

• Assigning Collection from parameter

• Package visible field

• 'instanceof' on 'catch' parameter

• All Finalization inspections

• All Initialization inspections (or most significant 'Overridable method call in constructor')

Thanks!

Wow. Harsh. The initialization inspections, while powerful, are computationally expensive compared to others, probably too much so for default enablement. Some new users may find IDEA slow, but that's damn sure not going to be my fault. Finalization ones and cloning ones are probably good, except for the inspection about not declaring finalize() at all. Too many books/websites out there that still recommend that, and shutdown hooks aren't well enough known yet. Same with static inheritance, although you might be pleased to know that I'm finally getting off my ass and creating a quickfix for that within the next couple of EAP (as I'm stuck maintaining a project that is infested with it).

--Dave

>> - Result of method call ignored (with its default
>> configuration)
HS> Somewhat annoying. If you're ignoring a result statement then
HS> chances are you don't actually care about the return value. It's not
HS> the kind of thing that happens by mistake,

Yes, this is the kind of thing that happens by mistake.

Have you even looked at the inspection? It seems you haven't.

This inspection is enabled by default for classes/methods where ignoring
the returned value is a noop, being in 99.99% of the cases a bug.

The only discussion here is on what classes/methods it should be enabled
by default.

And looking again at the default cases, all calls to *.toString() should
be flagged, instead of only StringBuffer.toString() and StringBUilder.toString().


>> - Octal integer
>>
HS> Why? It's a perfectly legitimate language construct. It's not common
HS> sure, but if someone goes out of their way to use it, chances are
HS> they know what they're doing.

Because this perfectly legitimate construct is a brain dead way of specifying
a different radix giving rise to subtle hard to find bugs.

Enabling this is for the most common case: writing octal numbers by mistake.
The uncommon case is writing 012 and wanting this to mean 10. Anyone wanting
this can turn the inspection off.

And also. Lets all of us create 'Enabled All inspection' for a day of real development. The inspection we don't like, we turn to off. Make this, we can get 'Anti-Default' list - inspection that will be never turned on by default.

My anti-list:
Entire categories

• Class metrics

• Method metrics

• Logging issues

• Serialization issues

Inspections:

• Local variable of concrete class

• Instance variable of concreate class

• Class without constructor

• Class without noarg constructor

• Class without logger

• Class too deep in inheritance tree

• Class without toString()

• Hardcoded string literal

• Implicit call to supper

• Local variable can be final

• Conditional expression

• Constant on right/left side of comparison

• Method return of concrete class

• Single class import

• Feature Envy

• All internalization issues

• Unqualified static usage

• 'instanceof' on concrete class

• 'synchronized' method

• Annonimous inner class

• All J2SDK5 inspections

• Magic number/character

• Overloaded methods with same number of parameters

• Assigment to method parameter

• Field repeatedly accessed in method

• '*' import

• Class name prefixed with package name

• Boolean method must start with question

• Constant declared in interface

• If statement with to many branches

• Assigning to 'null'

• Utility class

• Hardcoded file/line separator

• 'swith' statement

• 'final' class

• 'final' method

• Singleton

• Use of sun.* classes

Thanks!

Oops! :)
I mean that i like exceptions above to be enable by default/ I sent list of inspections that i'm dislike below in this post :)

>>> Static method referenced via subclass
>>>
>> pretty common pattern, so enforcing this would be quite mean.
>>

Can't say I've ever actually seen this in the wild. This isn't
accessing a static in a superclass in a subclass without qualifying
it. That's just fine. This is accessing a static in a superclass by
qualifing it with a subclasses name.

I do this all the time. I decide wheter to use the superclass or the subclass
based on which class it makes more sense for, conceptually.

Hani Suleiman wrote:
>> - Boolean constructor call

Only for JDK 1.3 targetted projects. I'd really like things that are
JDK specific to have text in the warning to point out that
non-backward compatibility.

Maybe I need more sleep, but how is it jdk specific?

Bas

>>> - Boolean constructor call
>>>
>>
BL> Maybe I need more sleep, but how is it jdk specific?

IIRC,the inspection replaces new Boolean(String) with Boolean.valueOf(String)
and this method only appeared on jdk 1.3.

Then I think the implementation of the inspection was changed somewhere along the way to replace with the more portable Boolean.TRUE and Boolean.FALSE, which is what it does on my machine. And I know those have been there since jdk 1.1 and probably before that.

Last I tried it it switched to using valueOf, which is a JDK 1.4 only method.

I think that defining a default set is going to be a futile since many of the inspections represent different philosophies to writing code. So you will only be favouring one style/philosphy over an other.

Why not group the inspections by Philosophy/Methodolgy/Style and present them as sets that can be enabled/disabled.

Florian

Then you'll be glad to know it now uses Boolean.TRUE and Boolean.FALSE. As far as I can see it has been this way since November.

Bas

Will we be able to save various sets of inspections and then swap them.

For example, if I'm a contractor working for two different companys with
two different coding standards, it would be nice to be able to set up
two sets of inspections and then swap them in and out.

Also would be nice to be able to let one person in the company set up a
set and share the set with team members instead of each person having to
set these up individually.

All of the features you describe are already present in the Irida EAP release, and work great.

--Dave Griffith

Hello Bas,

BL> Then you'll be glad to know it now uses Boolean.TRUE and
BL> Boolean.FALSE.


That's for new Boolean(true).

new Boolean("true") is changed to Boolean.valueOf("true").

Yes, but that is valid jdk 1.1 code.

Bas

Hello Bas,

BL> Yes, but that is valid jdk 1.1 code.

Yes, you're right and I'm wrong. Boolean.valueOf(boolean) is the "new" method
in 1.4 and is no longer used by the inspection.


Ha! Got you:

new Boolean(Boolean.getBoolean("xxx"));

and after simplifying:

Boolean.valueOf(Boolean.getBoolean("xxx"));


I knew I couldn't always loose every argument with you :)

Ah, well spotted.
I could modify the inspection to not show in this case when a jdk version less than 1.4 is used. Unfortenately I know of no way in the openapi to easily get the version of the jdk used.
You win:-)

I do. Can someone summarize the best semantics for this one, based on JDK level? I'm afraid I've lost track.

--Dave Griffith