Operation K. Looking for bugs in the IntelliJ IDEA code

In this article, we check the IntelliJ IDEA Community Edition project for errors and send the fixes to its developers. A large project, an open-source base, and a static analyzer that helps the creators develop their IDE. This is a difficult task for PVS-Studio.

About the project

Every Java developer knows about IDEA. This is the first IDE for many developers. Some programmers might have found this IDE to be the best one after having used others. Anyway, let's not underestimate how much IntelliJ has contributed to Java developers.

The Java programming language was used to develop IntelliJ IDEA. The first version was released in 2001. In 2009, JetBrains released the source code under the Apache license, which allows to use and modify the Community version of the product. The developers use Qodana — a proprietary static code analysis tool — to check the project code base.

Open-source IntelliJ IDEA contributors can access YouTrack — a project management and knowledge base. This combination makes it easier to analyze the code. It should help us determine the sources and history of the issues we find in the code.

This is a serious challenge for PVS-Studio, since the development team is large and uses its own static analyzer! Stock up on cookies and let's dive into the world of analyzer warnings.

Checking the project

Beyond reality

Can you find an error in the following code?

private static void doTest(@Nullable JBCefClient client) {
  ....
  try {
    browser = new JCEFHtmlPanel(client, "about:blank");
  } catch (RuntimeException ex) {
    fail("Exception occurred: " + ex.getMessage());
    ex.printStackTrace(); // <=
  }
  ....
}

It doesn't seem that there are issues in the fragment above. So, we need to understand what the fail method actually does to find the errors:

public static void fail(String message) {
    if (message == null) {
        throw new AssertionError();
    }
    throw new AssertionError(message);
}

We have unreachable code on the horizon! Take a look at the ex.printStackTrace() function in the first fragment. We try to display the stack trace to the console after an exception is thrown, so the code never gets control. The following warning proves that:

V6019 Unreachable code detected. It is possible that an error is present. IDEA283718Test.java(55)

Since the code is in unit tests, the stack trace output would be very useful for debugging the crash of the test.

After analyzing the commit history, I can conclude that the bug has been around since the code was first written in 2021.

Let's move the stack trace output up in the code — before the exception is thrown:

private static void doTest(@Nullable JBCefClient client) {
  ....
  try {
    browser = new JCEFHtmlPanel(client, "about:blank");
  } catch (RuntimeException ex) {
    ex.printStackTrace();
    fail("Exception occurred: " + ex.getMessage());
  }
  ....
}

Copy-paste wonders

How often do you copy code instead of rewriting it? However, inattention creeps up on you and leaves its marks. Marks in the form of errors. The following typos occur because of this programming practice:

private boolean areHomogenous(MouseWheelEvent e1, 
                              MouseWheelEvent e2) {
    if (e1 == null || e2 == null) return false;

    double distance = ....;
    return e1.getComponent() == e2.getComponent() &&
           e1.getID() == e2.getID() &&
           e1.getModifiersEx() == e2.getModifiersEx() &&
           e1.isPopupTrigger() == e1.isPopupTrigger() && // <=
           e1.getScrollType() == e2.getScrollType() &&
           distance < TOLERANCE;
}

In the method, the programmer uses the wrong variable and compares the value of isPopupTrigger to itself. That's not surprising, since this is the most common example of a copy-paste error, which the analyzer detects perfectly well, by the way. We have articles on this topic :)

And here is the unfortunate warning:

V6001 There are identical sub-expressions 'e1.isPopupTrigger()' to the left and to the right of the '==' operator. JBCefOsrComponent.java(459)

Take a look at another copy-paste error for you:

protected int locateUnicodeEscapeSequence(int start, int i) {
    ....
    if (StringUtil.isOctalDigit(c)) {
        if (i + 2 < myBufferEnd && 
            StringUtil.isOctalDigit(myBuffer.charAt(i + 1)) &&
            StringUtil.isOctalDigit(myBuffer.charAt(i + 1))) {
            return i + 3;
        }
    }
    ....
}

This situation is similar to the previous one: the developers made some copy-paste errors. Apparently, it should be i + 2 in the second check, but who knows how it supposed to work?

The analyzer issued a warning:

V6001 There are identical sub-expressions 'StringUtil.isOctalDigit(myBuffer.charAt(i + 1))' to the left and to the right of the '&&' operator. JavaStringLiteralLexer.java(64), JavaStringLiteralLexer.java(64)

Do or do not

Let's look at the following code fragment:

public String buildBeanClass() {
    if (recordsAvailable) {
        out.append("(");
        fields.stream().map(param -> param.getType()
.getCanonicalText(true) + " " + param.getName());

        StringUtil.join(fields, param -> {
          return ....;
        }, ", ", out);
        out.append("){}");
    } else ....
}

Can you spot the line with the error? The warning issued by the analyzer can serve as a clue:

V6010 The return value of function 'map' is required to be utilized. ParameterObjectBuilder.java(103)

Explanation time. We create stream from the fields collection, and:

1. we don't apply a termination operation, we use only an intermediate map operation (no result is obtained without the termination operation);
2. we don't use the results of the operation.

We could literally delete that line and no one would remember it.

Integer division magic

Introducing the division:

public class RectanglePainter2DTest extends AbstractPainter2DTest {
    private static final int RECT_SIZE = 10;
    private static final double ARC_SIZE = RECT_SIZE / 3;
    ...
}

The division is not simple but integer, so we return an integer value without decimals. Though we have the double variable. And wherever it is used, it's represented as a double variable.

Let's take a look at the analyzer warning:

V6094 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. RectanglePainter2DTest.java(24)

Can I assure you that the triggering is definitely positive? I think I can't. However, we do detect such things, and the analyzer is right about the loss of values after the decimal point. On the one hand, this isn't a big deal considering that the code is used in a unit test. On the other hand, it's an inaccurate dataset — not cool. In any case, we can fix it like this:

private static final double ARC_SIZE = RECT_SIZE / 3f;

Refactoring that leads to redundancy

Have you ever thought that sometimes we waste our time on something unjustified? Well, that's exactly what we do here:

protected List<....> getEncodeReplacements(CharSequence input) {
  for (int i = 0; i < input.length(); ++i) {
    if (input.charAt(i) == '\n') {
      final String replacement;
      if (i + 1 >= input.length() ||
        YAMLGrammarCharUtil.isSpaceLike(input.charAt(i + 1)) ||
        input.charAt(i + 1) == '\n' || 
        currentLineIsIndented) 
      {
        replacement = "\n" + indentString;
      }
      else 
      {
        replacement = "\n" + indentString;
      }
      ....
      continue;
    }
    ....
  }
}

Let's look at the warning:

V6004 The 'then' statement is equivalent to the 'else' statement. YAMLScalarTextImpl.java(116), YAMLScalarTextImpl.java(119)

The same method body is executed regardless of the condition. The most unsettling thing is that the check makes the code unreadable.

Looking at the commit history, I can say that the branch block once looked like this:

if (i + 1 >= input.length() ||
    YAMLGrammarCharUtil.isSpaceLike(input.charAt(i + 1)) ||
    input.charAt(i + 1) == '\n' || 
    currentLineIsIndented) {
    replacement = "\n" + indentString;
}
else {
    replacement = "\n\n" + indentString;
}

At some point, the developers removed a character and left a comment: Yaml: 'YamlScalarTextEvaluator' extracted to implement old 'getTextValue' behavior. And we don't want to argue. Let's just fix the code, remove the check, and leave only what's necessary:

for (int i = 0; i < input.length(); ++i) {
    if (input.charAt(i) == '\n') {
        final String replacement = "\n" + indentString;
        ....
        continue;
    }
    ....
}

Shenanigans of a constant

Here's the shortened code:

@Override
protected void customizeComponent(JList list, E value, boolean isSelected) {
    ....
    boolean nextStepButtonSelected = false; // <=
    ....
    myMnemonicLabel.setForeground(isSelected && 
                                  isSelectable && 
                                  !nextStepButtonSelected 
                                  ? getSelectionForeground() 
                                  : foreground);
    ....
    myShortcutLabel.setForeground(
      isSelected && 
      isSelectable && 
      !nextStepButtonSelected
      ? UIManager.getColor("MenuItem.acceleratorSelectionForeground")
      : UIManager.getColor("MenuItem.acceleratorForeground"));
    ....
    boolean selected = isSelected && isSelectable && !nextStepButtonSelected;

    setForegroundSelected(myValueLabel, selected);
    ....
}

What do you think is wrong here? The nextStepButtonSelected variable is always false.

"But there's a lot of behavior tied to it," you may say. "It doesn't matter," I'll answer.

V6007 Expression '!nextStepButtonSelected' is always true. PopupListElementRenderer.java(330)

V6007 Expression '!nextStepButtonSelected' is always true. PopupListElementRenderer.java(384)

V6007 Expression '!nextStepButtonSelected' is always true. PopupListElementRenderer.java(373)

Three warnings at once isn't a good thing. After analyzing the code, I can say that the developers wanted to remove the behavior, but didn't delete the variable. We have the false constant which leads to poor readability and possible issues when changing the code in the future.

Conclusion

I think it's time to wrap it up. We've made a pull request to the IDEA developers, and I've accomplished the tasks I set out to do. I'm really happy to help the developers of my favorite IDE.

An excellent check of an existing project that already uses static analysis.

Would you like to try static analysis, too? You can always introduce it in your project with the help of PVS-Studio. Just download the tool here.

Comments

[Fyodor]

Impressive 👍 glad that there’s actually not much of a disaster there 😅