Enable most IntelliJ 'Probable bugs' inspections #4353

leventov · 2017-06-02T03:58:08Z

Enable "Error" severity for most IntelliJ inspections of "Probable bugs" category which don't catch anything at the moment. Inspections, which have a lot of matches in the codebase, are left on "Warning" level, to keep this PR small. The latter inspections should be moved to "Error" level one-by-one in separate PRs.

A few real (but minor) defects are fixed in the code:

Fix IndexSpec.equals() and hashCode() to include longEncoding
calling toString() on objects of classes which don't override toString()
instance fields which should really be static final fields
unnecessary use of | and & instead of || and &&.

…pections

gianm · 2017-06-07T00:55:47Z

api/src/main/java/io/druid/data/input/impl/TimestampSpec.java

  // this value should never be set for production data
  private final DateTime missingValue;
+  private final transient Function<Object, DateTime> timestampConverter;


I thought transient just marks fields that shouldn't be serialized. But this class isn't Serializable, so what's the point?

It was for indication that this field is not checked in equals() and hashCode(). But that was a bad way, removed transient and added a simple comment.

gianm · 2017-06-07T00:56:48Z

common/src/main/java/io/druid/timeline/partition/IntegerPartitionChunk.java

@@ -27,7 +27,7 @@
 */
 public class IntegerPartitionChunk<T> implements PartitionChunk<T>
 {
-  Comparator<Integer> comparator = Ordering.<Integer>natural().nullsFirst();
+  private static final Comparator<Integer> comparator = Ordering.<Integer>natural().nullsFirst();


static field should be all-caps.

Removed field

gianm · 2017-06-07T00:56:59Z

common/src/main/java/io/druid/timeline/partition/LinearPartitionChunk.java

@@ -25,7 +25,7 @@

 public class LinearPartitionChunk <T> implements PartitionChunk<T>
 {
-  Comparator<Integer> comparator = Ordering.<Integer>natural().nullsFirst();
+  private static final Comparator<Integer> comparator = Ordering.<Integer>natural().nullsFirst();


static field should be all-caps.

Removed field

gianm · 2017-06-07T01:00:36Z

processing/src/main/java/io/druid/segment/IndexMerger.java

@@ -1349,10 +1349,12 @@ boolean needConversion(int index)
    {
      IntBuffer readOnly = conversions[index].asReadOnlyBuffer();
      readOnly.rewind();
-      for (int i = 0; readOnly.hasRemaining(); i++) {
+      int i = 0;
+      while (readOnly.hasRemaining()) {


What prompted this change?

There is an inspection that checks that loop variable is checked in condition part of for loop. This occasion was a false positive, and I think it's better to reformat code than add @SuppressWarnings

gianm · 2017-06-07T01:01:54Z

processing/src/main/java/io/druid/segment/data/BitmapCompressedIndexedInts.java

@@ -32,7 +32,7 @@
 */
 public class BitmapCompressedIndexedInts implements IndexedInts, Comparable<ImmutableBitmap>
 {
-  private static Ordering<ImmutableBitmap> comparator = new Ordering<ImmutableBitmap>()
+  private static final Ordering<ImmutableBitmap> comparator = new Ordering<ImmutableBitmap>()


comparator should be all-caps.

gianm · 2017-06-07T03:50:05Z

Looks good to me, 👍 after CI passes.

…ntelliJ-specific inspections

leventov · 2017-06-07T05:47:16Z

Decided to use IntelliJ-style noinspection comments to suppress IntelliJ-specific inspections, instead of @SuppressWarnings on a method, because the scope of suppression is narrower and it's obvious why suppression is added -- the reason is on the next line. It doesn't matter that those comments are "non-standard", because no other tools or IDEs like Eclipse are able to understand the suppression reasons anyway.

drcrallen · 2017-06-07T16:11:39Z

processing/src/main/java/io/druid/segment/serde/DictionaryEncodedColumnPartSerde.java

@@ -506,16 +506,18 @@ public void read(ByteBuffer buffer, ColumnBuilder builder, ColumnConfig columnCo
      )
      {
        switch (version) {
-          case UNCOMPRESSED_MULTI_VALUE:
+          case UNCOMPRESSED_MULTI_VALUE: {


I don't think we've ever had this rule before, where is it enforced?

This change is a workaround for false positive inspection (due to a defect in the inspection itself). The inspection tracks for "false indentation", e. g.

for (int i : array) actionOne(); actionTwo();

Because I may think that both actionOne() and actionTwo() are executed in the loop, and I forgot braces.

In this case, inspection thinks that block

default: throw new IAE("Unsupported multi-value version[%s]", version); throw new IAE("Unsupported multi-value version[%s]", version);

should have deeper indentation, because the previous case doesn't have break; in the end. But if fails to see that all brances in if-else chain break the control flow (two returns and one throw).

drcrallen · 2017-06-07T16:13:57Z

processing/src/test/java/io/druid/query/cache/CacheKeyBuilderTest.java

-        }
+    for (int i = 0; i < keys.size(); i++) {
+      for (int j = i + 1; j < keys.size(); j++) {
+        assertFalse(Arrays.equals(keys.get(i), keys.get(j)));


this logic changes if I'm reading it correctly. Previously if k1 and k2 were the same object, this would not fail. But now it will fail.

was the prior way just a hack to get over the fact that this was doing an n^2 search?

As far as I understand, the previous version was looking for duplicates in the keys list. Condition k1 != k2 was filtering "the diagonal" in n^2 search, but it was unreliable. Anyway, I had to change this piece of code because inspection about array reference comparison.

drcrallen · 2017-06-07T16:18:29Z

server/src/main/java/io/druid/segment/realtime/FireHydrant.java

@@ -120,7 +120,7 @@ public String toString()
  {
    return "FireHydrant{" +
           "index=" + index +
-           ", queryable=" + adapter +
+           ", queryable=" + adapter.getIdentifier() +


why this change?

Because ReferenceCountingSegment doesn't override Object.toString(), so FireHydrant's String form looks like .., queryable=ReferenceCountingSegment@hash, .. - not useful.

Enable most IntelliJ 'Probable bugs' inspections

79a7791

leventov added the Improvement label Jun 2, 2017

leventov added 2 commits June 2, 2017 01:03

Fix in RemoteTestNG

5cb8dba

Fix IndexSpec's equals() and hashCode() to include longEncoding

da614b0

leventov mentioned this pull request Jun 2, 2017

Adds long compression methods #3148

Merged

leventov added 2 commits June 6, 2017 00:40

Merge remote-tracking branch 'upstream/master' into more-intellij-ins…

a704c06

…pections

Fix inspection errors

28ac0b4

gianm reviewed Jun 7, 2017

View reviewed changes

leventov added 2 commits June 6, 2017 22:39

Extract global isntance of natural().nullsFirst(); address comments

09de925

Fix

c647f42

leventov added the WIP label Jun 7, 2017

leventov added 2 commits June 7, 2017 00:23

Use noinspection comments instead of SuppressWarnings on method for I…

5a76315

…ntelliJ-specific inspections

Prohibit Ordering.natural().nullsFirst() using Checkstyle

224d16a

leventov removed the WIP label Jun 7, 2017

drcrallen reviewed Jun 7, 2017

View reviewed changes

drcrallen approved these changes Jun 7, 2017

View reviewed changes

drcrallen merged commit 63a897c into apache:master Jun 7, 2017

drcrallen deleted the more-intellij-inspections branch June 7, 2017 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable most IntelliJ 'Probable bugs' inspections #4353

Enable most IntelliJ 'Probable bugs' inspections #4353

leventov commented Jun 2, 2017 •

edited

Loading

gianm Jun 7, 2017

leventov Jun 7, 2017

gianm Jun 7, 2017

leventov Jun 7, 2017

gianm Jun 7, 2017

leventov Jun 7, 2017

gianm Jun 7, 2017

leventov Jun 7, 2017

gianm Jun 7, 2017

gianm Jun 7, 2017

leventov Jun 7, 2017

gianm commented Jun 7, 2017

leventov commented Jun 7, 2017

drcrallen Jun 7, 2017

leventov Jun 7, 2017

drcrallen Jun 7, 2017

drcrallen Jun 7, 2017

leventov Jun 7, 2017

drcrallen Jun 7, 2017

leventov Jun 7, 2017

Enable most IntelliJ 'Probable bugs' inspections #4353

Enable most IntelliJ 'Probable bugs' inspections #4353

Conversation

leventov commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Jun 7, 2017

leventov commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leventov commented Jun 2, 2017 •

edited

Loading