The check of the Rhino JavaScript engine or how the unicorn met the rhino
Tags: java, javascript, opensource, codequality
Published: 2022-10-07 07:21:38

Among the wide variety of programming languages, what our users want the most is that the PVS\-Studio analyzer to start supporting JavaScript\. The Rhino engine is a project that our team can use to create a PVS\-Studio analyzer for JavaScript\. In this article, we will use the PVS\-Studio analyzer to check the source code of Rhino\.

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/7dad90ey4t7iy0r4q0yo.png)

## Beginning

The PVS\-Studio analyzer has gradually evolved and supported new programming languages\. The first additional language \(to the original C and C\+\+\) was C\#, then we supported Java\. Users of the analyzer often wonder if PVS\-Studio will support a new language in the future\. And if so, what language will it be? 

I'm one of those developers who occasionally explore something work\-related in my spare time\. Writing a static analyzer for the JavaScript language is one of my current areas of interest\.

However, it all started with another language — Kotlin\. One day, I decided to spend my free time learning a new programming language\. Since I am familiar with Java and C\#, my choice fell on Kotlin\. In my opinion, it combines the features of both Java and C\#\. In the process of studying Kotlin, I learned that there is a possibility to transpile the program in Kotlin not only into Java, but also into JavaScript\.


> Transpiler is a type of translator that takes the source code of a program written in one programming language and produces an equivalent source code in a different programming language\.



This got me very excited so I opened GitHub to study the sources of a transpiler that translates from Kotlin into JavaScript\. As it turned out, developers of the transpiler did not create their own parser and classes for [AST \(abstract syntactic tree\)](https://pvs-studio.com/en/blog/terms/0004/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) — they reused the code from the Rhino engine\. So, the Rhino engine became the next project to explore\. 

The Rhino engine has several interesting features:

* It is written in Java;
* Its first version was released in 1997 \(25 years ago\), when Java didn't even have generics \(they appeared in Java 5\.0 in 2004\);
* The engine works in both compiled and interpreted mode\. "How is it possible to compile JavaScript?" you may ask\. It's simple: code written in JavaScript is converted to Java bytecode;
* Rhino in compiler mode often produces more performance than the C implementation of JavaScript;
* Rhino does not have browser integration by default;
* You can use Rhino to write application's business logic in the simpler JavaScript language instead of Java\. This function allows to write business logic even for experts with little knowledge of programming\.

At the same time as studying the [Rhino](https://github.com/mozilla/rhino)'s source code, I downloaded its source files and ran the analysis using the [PVS\-Studio plugin for IntelliJ IDEA](https://plugins.jetbrains.com/plugin/12263-pvs-studio-for-idea-and-android-studio)\. The analyzer found quite a few warnings for a 25\-year\-old project \(they are listed in descending order of certainty\):

* High: 79
* Medium: 158
* Low: 208

After a brief glance at warnings, it seemed to me that some of them turned out to be [false positives](https://pvs-studio.com/en/blog/terms/6461/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990)\. Here are the most striking ones:

* In my opinion, all [V6022](https://pvs-studio.com/en/docs/warnings/v6022/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) diagnostic warnings turned out to be false positives\. They appeared due to the API change of some methods and constructors\. [V6022](https://pvs-studio.com/en/docs/warnings/v6022/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) looks for arguments that are not used inside the method's or constructor's body\. That is, the analyzer issues a warning when nothing is assigned to arguments, no properties/methods are called, arguments are not passed to any method/constructor and are not assigned to any properties/fields;
* About 25% of the [V6008](https://pvs-studio.com/en/docs/warnings/v6008/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) diagnostic rules are of a low certainty level \(Low\)\. The diagnostic looks for places where zero\-link access can occur\. The warnings appeared because the *XMLList\.getXML* method can potentially return *null* if the passed index turns out to be negative\. However, this will never happen, because when this method is called, an array from the *\_annos* field is always passed as the first argument\. Inside the *XMLList\.getXML* method, the *XMLList\.length* method is called, which is also related to the *\_annos* field\. Therefore, the method will never return a negative value:

```cpp
class XMLList extends XMLObjectImpl implements Function {
  ....
  private XmlNode.InternalList _annos;
  ....
  XMLList(XMLLibImpl lib, Scriptable scope, XMLObject prototype) {
    super(lib, scope, prototype);
    _annos = new XmlNode.InternalList();
  }
  ....
  private XML getXML(XmlNode.InternalList _annos, int index) {
    if (index >= 0 && index < length()) {
      return xmlFromNode(_annos.item(index));
    } else {
      return null;
    }
  }
  ....
  @Override
  int length() {
    int result = 0;

    if (_annos != null) {
      result = _annos.length();
    }

    return result;
  }
  ....
  private XML getXmlFromAnnotation(int index) {
    return getXML(_annos, index);               //<=
  }
  ....
}
```

I quickly found all false positive warnings and filtered them out using filters of the plugin's interface — I removed false positives from the table with the analysis results so that they would not distract me when I view other warnings\. As a result, the number of warnings in the table decreased:

* High: 79 \(did not change\)
* Medium: 47 \(there were 158\)
* Low: 154 \(there were 208\)

After that, I reviewed other warnings and selected the most interesting ones\. Let's not waste a second and look at the warnings that the PVS\-Studio analyzer found in the Rhino project's code\.

## Forgotten field \+ a typo

[V6001](https://pvs-studio.com/en/docs/warnings/v6001/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) There are identical sub\-expressions 't2Docked' to the left and to the right of the '&&' operator\. SwingGui\.java\(2718\), SwingGui\.java\(2718\)

```cpp
class ContextWindow extends JPanel implements ActionListener {
  ....
  public ContextWindow(final SwingGui debugGui) {
    ....
    ComponentListener clistener =
      new ComponentListener() {
          boolean t2Docked = true;

          void check(Component comp) {
              ....
              if (leftDocked && t2Docked && rightDocked && t2Docked) { // <=
                  // no change
                  return;
              }
              
              t2Docked = rightDocked;
              
              // Further t2Docked is not used
              ....
          }
          ....      
    };
    ....
  }
  ....
}
```

In the *ContextWindow* constructor, a local object of the *ComponentListener* interface type is created\. This object calls the *check* method in each of the methods that are implemented by this object\. Inside the method, there is a check whether the component is aligned left or right side of some window, and whether the location of the window dividers changes in *JSplitPane*\.

In this method, the analyzer detected an error — the *t2Docked* field of the *clistener* object \(yes, this is an object field, not a variable, since it is declared in the initialization block of the local object\) is used twice in the *if* condition:

```cpp
if (leftDocked && 
    t2Docked &&     // <=
    rightDocked && 
    t2Docked) {     // <=
  // no change
  return;
}
```

Considering that in the *check* method the aligning of components is checked for the left and right sides, I thought that initially the *clistener* object had another *t1Docked* field, which was eventually deleted\. In the case with the additional field, the condition in *if* looked more logical\. It would have had an error though, most likely due to a typo, which the analyzer would also have warned about\. After correcting this typo, the condition in *if* would become like this:

```java
leftDocked && t1Docked && rightDocked && t2Docked
```

To fix the current code fragment, I would recommend using *t2Docked* once in the condition\. For example, as follows: 

```java
t2Docked && leftDocked && rightDocked
```

## Copy paste, impossible conditions, and unreachable code

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Expression 'power < 2' is always false\. UintMap\.java\(39\)

```java
public class UintMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public UintMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}
```

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Expression 'power < 2' is always false\. ObjToIntMap\.java\(102\)

```java
public class ObjToIntMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public ObjToIntMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}
```

Code for the constructors of the *UintMap* and *ObjToIntMap* classes is identical, the only difference is the argument's name\. The check is performed at the end of the code of both constructors:

```java
if (check && power < 2) Kit.codeBug();
```

If this check is true, the *Kit\.codeBug* method is called and it simply throws an exception:

```java
public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}
```

Firstly, the check depends on the value of the final private *check* field\. Since this field is *false* by default, execution will never reach the *power < 2* expression\. A developer may have used this field to test the class, so the field can still take the *true* value when the developer will test the class again\.

Secondly, the analyzer pointed out that even if the execution reaches this expression, it will always be false, because by the time the check is performed, the value of the *power* field will be at least 2\. The thing is that *power* stores the value of the *i* local variable that is also a counter in the *for* loop\. This variable is initialized in the loop with the 2 value, and after that is incremented at each iteration of the loop\. The maximum value of *i* can grow to 31, because 1 << 31 = *Integer\.MIN\_VALUE* \(\-2147483648\)\. If there is the *minimalCapacity* variable overflow, then no iteration will be performed, which means the value of the *i* variable will remain 2\.

Because of this, the *Kit\.codeBug* method will never be called\. It means the analyzer helped detect unreachable code\. I don't know how to fix this code, as I don't really understand why this check is here\. Therefore, it is up to the programmer who wrote the code to fix this error\.

## The memory of the bug carried through the years

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Expression '\!BUG\_369394\_IS\_VALID' is always true\. XmlProcessor\.java\(288\)

```java
class XmlProcessor implements Serializable {
  ....
  private void addTextNodesToRemoveAndTrim(List<Node> toRemove, Node node) {
    if (node instanceof Text) {
      Text text = (Text) node;
      boolean BUG_369394_IS_VALID = false;
      if (!BUG_369394_IS_VALID) {                 // <=
        text.setData(text.getData().trim());
      } else {
        if (text.getData().trim().length() == 0) {
          text.setData("");
        }
      }
      if (text.getData().length() == 0) {
        toRemove.add(node);
      }
    }
    ....
  }
  ....
}
```

The analyzer warnings don't just help you find bugs and vulnerabilities in your code\. Sometimes they also help you remember to delete unnecessary code\. The developer created a branch in *addTextNodesToRemoveAndTrim* method that depends on the local *BUG\_369394\_IS\_VALID* boolean variable, which is initialized with *false* and does not change over the course of the method\. The analyzer noticed that and issued a warning\. 

This variable, as I understand it, turned out to be necessary for fixing a [bug with the 369394 number from the Bugzilla website](https://bugzilla.mozilla.org/show_bug.cgi?id=369394)\. Considering that *BUG\_369394\_IS\_VALID* is set to *false*, this bug is most likely no longer relevant\. If this is the case, it is a good idea to remove the branch that depends on this variable\. If this bug is still relevant, it is not clear why the value of this variable is *false* and not *true*\.

## When pretty isn't so pretty

[V6007](https://pvs-studio.com/en/docs/warnings/v6007/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Expression 'i < indentLevel' is always false\. XmlProcessor\.java\(454\)

```java
class XmlProcessor implements Serializable {
  ....
  private boolean prettyPrint;
  ....
  private int prettyIndent;  
  ....
  final String ecmaToXmlString(Node node) {
    // See ECMA 357 Section 10.2.1
    StringBuilder s = new StringBuilder();
    int indentLevel = 0;
    if (prettyPrint) {
      for (int i = 0; i < indentLevel; i++) { // <=
        s.append(' ');
      }
    }
    ....
  }
  ....
}
```

The analyzer detected that the *i < indentLevel* expression is always false, because the *i* and *indentLevel* variables equal 0 on the first loop\. It means, the iteration of the *for* loop will not occur\. As a result, when information is written to *StringBuilder*, space indentation will not be added if the *prettyPrint* mode is selected\. This will likely worsen the readability of the text written in *StringBuilder*\. 

According to the comment at the beginning of the method, the algorithm for this method should follow the [algorithm from the ECMA documentation described in section 10\.2\.1](https://www.ecma-international.org/wp-content/uploads/ECMA-357_2nd_edition_december_2005.pdf) \(page 41\)\. It follows from the documentation that another optional argument with the *indentLevel* name should be passed to the method\. The error may have occurred for the following reasons\. Either this argument was there originally, but then it was removed, or the developer forgot to fine\-tune the method so that the algorithm corresponded the documentation\.

## Potential dereference of a null reference

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Potential null dereference of 'generatorState' in function 'freezeGenerator'\. Interpreter\.java\(1244\), Interpreter\.java\(3188\)

```java
public final class Interpreter extends Icode implements Evaluator {
  ....
  private static Object interpretLoop(Context cx, 
                                      CallFrame frame, 
                                      Object throwable) {
  
    GeneratorState generatorState = null;
    if (throwable != null) {
      if (throwable instanceof GeneratorState) {
        generatorState = (GeneratorState) throwable;
        ....
      } else if (!(throwable instanceof ContinuationJump)) {
        Kit.codeBug();
      }
    }
    ....
    StateLoop:
    for (; ; ) {
      ....
      if (throwable != null) {
        ....
      } else {
        if (generatorState == null && frame.frozen) Kit.codeBug();
      }
    
      for (; ; ) {
        ....
        int op = iCode[frame.pc++];
        jumplessRun:
        {
          switch (op) {
            ....
            case ....:
              {
                if (!frame.frozen) {
                  return freezeGenerator(...., 
                                         generatorState, // <=
                                         ....);
                }                  
                ....
              }
            // many more cases
            ....
          }
        }
        ....
      }               
      ....
    }
    ....
  }
  ....
}
```

The analyzer reports that *generatorState* is null, it may be dereferenced inside the *freezeGenerator* method\. Given that the *interpretLoop* method stretched for almost 1500 lines, it would be difficult \(or almost impossible\) to understand this without the analyzer\. I have a feeling that the analyzer is correct\. The proof starts from the place where the *freezeGenerator* method is called\. 

**The first\. **The *frame\.frozen* field is set to *false* by default for all *CallFrame* objects when cloning happens:

```java
CallFrame cloneFrozen() {
  ....
  copy.frozen = false;        // <=
  return copy;
}
```

**Second**\. The *freezeGenerator* method will be called only if *frame\.frozen* is false:

```java
  ....
  if (!frame.frozen) {
    return freezeGenerator(
      cx,
      frame,
      stackTop,
      generatorState,                // <=
      op == Icode_YIELD_STAR);
  }
  ....
```

**The third**\. Above the call to this method, the *generatorState* variable is checked for *null*:

```java
if (generatorState == null && frame.frozen) Kit.codeBug();
```

If the expression in *if* is true, an exception will occur, because the *Kit\.codeBug* method throws an exception without any checks:

```java
public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}
```

**The fourth\.** The check will be performed only if the *throwable* argument of the *interpretLoop* method equals *null*:

```java
  if (throwable != null) { // <=
    ....
  } else {
    if (generatorState == null && frame.frozen) Kit.codeBug(); // <=
  }
```

**The fifth\.** If you look even higher up the code, you will find that the original value of the *generatorState* variable is *null*\. It changes only if it is an instance of the *GeneratorState* class and if *throwable* is not *null*:

```java
  ....
  GeneratorState generatorState = null; // <=
  if (throwable != null) {
    if (throwable instanceof GeneratorState) {
      generatorState = (GeneratorState) throwable; // <=

      // reestablish this call frame
      enterFrame(cx, frame, ScriptRuntime.emptyArgs, true);
      throwable = null;
    } else if (!(throwable instanceof ContinuationJump)) {
      // It should be continuation
      Kit.codeBug();
    }
  }
  ....
```

**Results\.** Taking all of the above into account, it turns out that if *throwable* is *null* or not *GeneratorState*, then *generatorState* will also be *null*\. And if *frame\.frozen* is false, calling the *freezeGenerator* method will throw a *NullPointerException* when the *operation* field is accessed:

```java
private static Object freezeGenerator(...., 
                                      GeneratorState generatorState, 
                                      ....) {
  if (generatorState.operation == NativeGenerator.GENERATOR_CLOSE){     // <=
      // Error: no yields when generator is closing
      throw ScriptRuntime.typeErrorById("msg.yield.closing");
  }
  ....
}
```

## Unreachable error handling

[V6019](https://pvs-studio.com/en/docs/warnings/v6019/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Unreachable code detected\. It is possible that an error is present\. Parser\.java\(3138\)

```java
public class Token {
  ....
  public static final int ERROR = -1, .... 
  ....
}

public class Parser {

  static final int CLEAR_TI_MASK = 0xFFFF, ....;    
  private AstNode primaryExpr() throws IOException {
    int ttFlagged = peekFlaggedToken();
    int tt = ttFlagged & CLEAR_TI_MASK; 

    switch (tt) {
      ....
      case Token.ERROR:
            consumeToken();                                               // <=
            // the scanner or one of its subroutines reported the error.
            break;
      ....
    }
    ....
  }
  ....
}
```

The *peekFlaggedToken* method returns the *int* value\. Then, this value is changed using the bitwise operation "&" \(and\)\. And the right operand of this expression is a static unchanging constant — 0xFFFFFF \(65535\)\. Therefore, the result of a bitwise "and" will always be the number \>= 0\. However, further down the code, the result of the bitwise "and" is used in *switch*, where one of the branches is executed when the value of the expression in *switch* is *Token\.ERROR*, namely \-1\.  The analyzer warns us that this branch will never be executed\.

## Copy paste and missing curly brackets

[V6021](https://pvs-studio.com/en/docs/warnings/v6021/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) The value is assigned to the 'localMax' variable but is not used\. NativeRegExp\.java\(568\)

```java
public class NativeRegExp extends IdScriptableObject {
  ....
  private static boolean calculateBitmapSize(....) {
    ....                             
    switch (src[index]) {
      case '\\':
        ....
        switch (c) {
          ....
          case 'c':
            if ((index < end) && isControlLetter(src[index]))
              localMax = (char) (src[index++] & 0x1F);         // <=
            else --index;
            localMax = '\\';
            break;
          ....
        }
      ....
    }
    // use localMax
    ....
  }
  ....
}
```

In the *if* branch, the result of the expression:

```java
(char) (src[index++] & 0x1F)
```

is saved to the *localMax* variable\. However, the value recorded to the variable is not used until the new '\\\\' value is written to *localMax*\. After exiting the *switch*, the *localMax* value is used in the *if* condition\. Therefore, it is possible that one of the *if\.\.\.else* branches will not be executed when needed\. The same class also has the *processCharSetImpl* method that uses a *switch* almost identical in structure\. However, there is no error in the branch for the 'c' character because the *else* body is in curly brackets:

```java
....
case 'c':
  if ((src < end) && isControlLetter(gData.regexp.source[src]))
    thisCh = (char) (gData.regexp.source[src++] & 0x1F);
  else {
    --src;
    thisCh = '\\';
  }
  break;
....
```

So, to fix this error, you need to wrap the instructions after *else* and before *break* into curly brackets in the *NativeRegExp*\.*calculateBitmapSize* method:

```java
case 'c':
  if ((index < end) && isControlLetter(src[index]))
    localMax = (char) (src[index++] & 0x1F);         
  else { // <=
    --index;
    localMax = '\\';
  }      // <=
  break;
....
```

## Forgotten unused value

[V6021](https://pvs-studio.com/en/docs/warnings/v6021/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) The value is assigned to the 'nameString' variable but is not used\. QName\.java\(293\)

```java
final class QName extends IdScriptableObject {
  ....
  QName constructQName(XMLLibImpl lib, 
                       Context cx, 
                       Object namespace, 
                       Object name) {
    String nameString = null;
    if (name instanceof QName) {
      if (namespace == Undefined.instance) {
        return (QName) name;
      } else {
        nameString = ((QName) name).localName();          // <=
      }
    }
    if (name == Undefined.instance) {
      nameString = "";
    } else {
      nameString = ScriptRuntime.toString(name);
    }
    ....
    return newQName(lib, q_uri, q_localName, q_prefix);
  }
  ....
}
```

The [V6021](https://pvs-studio.com/en/docs/warnings/v6021/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) diagnostics detected that the result of the expression is assigned to the *nameString* variable:

```java
((QName) name).localName()
```

However, further in both branches of *if\.\.\.else*, the *nameString* value is reassigned before it is used:

```java
if (name == Undefined.instance) {
  nameString = "";
} else {
  nameString = ScriptRuntime.toString(name);
}
```

Most likely, this error could have occurred as a result of refactoring and the programmer did not notice it\. I assume that between storing the value in *nameString* at the beginning of the method and changing it in *if\.\.\.else* it was supposed to be used\. Or maybe the programmer forgot to add another branch in *if\.\.\.else*, where the *nameString* value was used \(or at least was not changed\)\.

## Potential error and dependent arguments

[V6025](https://pvs-studio.com/en/docs/warnings/v6025/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Index 'sig1\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(461\)

[V6025](https://pvs-studio.com/en/docs/warnings/v6025/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Index 'sig2\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(462\)

```java
public class NativeJavaMethod extends BaseFunction {

  private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1] // <=
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1] // <=
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}
```

The analyzer warns that index access in *sig1\[sig1\.length \- 1\]* and* sig2\[sig2\.length \- 1\]* expressions may cause *ArrayIndexOutOfBoundsException* to occur\. Both of these expressions can indeed lead to the exception if the *sig1* or *sig2* arrays are empty, because in this case the access by the \-1 \(0\-1 = \-1\) index will occur\. Even checks of the *j \>= sig1\.length* and *j \>= sig2\.length* ternary operators do not protect against access by \-1 index on the first iteration of the *for* loop\. 

In fact, in the current code base this method is only called in one place and the arguments that are passed to the method when the are called are dependent on each other\. If *vararg1* is true, the *sig1* array will also have at least one element\. A similar dependency exists between *vararg2* and the *sig2* array\. This is due to Java reflection, because if calling the *isVararg* method of *java\.lang\.reflect\.Method* or *java\.lang\.reflect\.Constructor* objects returns *true*, then calling the *getParameterTypes* method will also return an array that has at least one element \(the first argument, which is declared as a vararg\)\. Objects of these classes are used in the source code as arguments for calling the *preferSignature* method\. So in the current source code, the error the analyzer warns about will never occur\. In the future, however, there is still the possibility that this method could be called incorrectly, resulting in a \-1 index access\.

To prevent incorrect calls to the *preferSignature* method in the future, I suggest that you return \-1 immediately if *vararg1* is true and the *sig1* array is empty, or if *vararg2* is true and the *sig2* array is empty:

```java
private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    if (vararg1 && sig1.length == 0 || vararg2 && sig2.length == 0){
      return -1;
    }
                                                                    
    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1]
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1]
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}
```

## Unsafe access to an array element

[V6025](https://pvs-studio.com/en/docs/warnings/v6025/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Possibly index 'last \- 1' is out of bounds\. SuperBlock\.java\(50\)

```java
final class SuperBlock {
  ....
  int[] getTrimmedLocals() {
    int last = locals.length - 1;
    while (last >= 0
        && locals[last] == TypeInfo.TOP
        && !TypeInfo.isTwoWords(locals[last - 1])) {         // <=
      last--;
    }
    ....
  }
  ....
}
```

At the beginning of the *getTrimmedLocals* method, the array elements from the *locals* field are sorted using the *while* loop\. However, a potential error has crept into the *while* loop's condition — accessing the *locals* array's element by index \-1 can generate *ArrayIndexOutOfBoundsException*\. 

This will happen if the *last* loop counter is 0 and at the same time the first element of the array \(index 0\) is equal to the value of *TypeInfo\.TOP* \(value 0\)\. In this case, the last expression from the *while* loop condition will be executed, and before the *TypeInfo\.isTwoWords* method is called, the *locals* array element is accessed by index 0 \- 1 = \-1\.

I assume that *ArrayIndexOutOfBoundsException* might never occur simply because somewhere in code, the first element in the *locals* array is always written with a value not equal to *TypeInfo\.TOP* \(value 0\)\. In this situation, it is better to replace the index in the first check with 1 instead of 0 in the *while* statement to prevent the exception in the future:

```java
while (last >= 1                          // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}
```

Or it's possible to use the \> \(greater than\) operator instead of the \>= \(greater than or equal to\) operator:

```java
while (last > 0                                  // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}
```

## Mutually annihilating operands

[V6028](https://pvs-studio.com/en/docs/warnings/v6028/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Identical expression 'end' to the left and to the right of compound assignment\. FastDtoaBuilder\.java\(74\)

```java
public class FastDtoaBuilder {
  ....
  private void toFixedFormat(int firstDigit, int decPoint) {
    if (point < end) {
      ....
    } else if (point > end) {
      // large integer, add trailing zeroes
      Arrays.fill(chars, end, point, '0');
      end += point - end;                        /// <=
    }
  }
  ....
}
```

The analyzer found the last line in the *else if* block strange\. If you describe the operations in the block in more detail and simplify the code, it becomes clearer what the analyzer is warning about:

```java
end = end + point – end 
=> end = end – end + point 
=> end = + point 
=> end = point
```

As you can see, all that will actually happen is that the value of the *point* variable will be assigned to the *end* variable\.

Most likely, there is no error in this case, and this expression remained after refactoring\. However, even in this case, I would suggest replacing the use of the *\+=* operator with the usual assignment of the *point* value to the *end* variable\.

## Commented out code

[V6032](https://pvs-studio.com/en/docs/warnings/v6032/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) It is odd that the body of method 'endCheckSwitch' is fully equivalent to the body of another method 'endCheckTry'\. Node\.java\(681\), Node\.java\(717\)

```java
public class Node implements Iterable<Node> {
  ....
  private int endCheckSwitch() {
    int rv = END_UNREACHED;

    // examine the cases
    //         for (n = first.next; n != null; n = n.next)
    //         {
    //             if (n.type == Token.CASE) {
    //                 rv |= ((Jump)n).target.endCheck();
    //             } else
    //                 break;
    //         }

    //         // we don't care how the cases drop into each other
    //         rv &= ~END_DROPS_OFF;

    //         // examine the default
    //         n = ((Jump)this).getDefault();
    //         if (n != null)
    //             rv |= n.endCheck();
    //         else
    //             rv |= END_DROPS_OFF;

    //         // remove the switch block
    //         rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);

    return rv;
  }
  ....
  private int endCheckTry() {
    int rv = END_UNREACHED;

    // a TryStatement isn't a jump - needs rewriting

    // check the finally if it exists
    //         n = ((Jump)this).getFinally();
    //         if(n != null) {
    //             rv = n.next.first.endCheck();
    //         } else {
    //             rv = END_DROPS_OFF;
    //         }

    //         // if the finally block always returns, then none of the returns
    //         // in the try or catch blocks matter
    //         if ((rv & END_DROPS_OFF) != 0) {
    //             rv &= ~END_DROPS_OFF;

    //             // examine the try block
    //             rv |= first.endCheck();

    //             // check each catch block
    //             n = ((Jump)this).target;
    //             if (n != null)
    //             {
    //                 // point to the first catch_scope
    //                 for (n = n.next.first; n != null; n = n.next.next)
    //                 {
    //                     // check the block of user code in the catch_scope
    //                     rv |= n.next.first.next.first.endCheck();
    //                 }
    //             }
    //         }

    return rv;
  }
}
```

This warning doesn't mean the code contains an error — the [V6032](https://pvs-studio.com/en/docs/warnings/v6032/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) diagnostic detects methods that perform the same thing\. Judging by the comments and commented out code pieces inside these functions, developers intended to fine\-tune both methods in the future\. Thus, the analyzer helped to detect methods with commented\-out code that need to be finalized\. The analyzer's warning helps the programmer not to forget to refine these methods in the future, as the code doesn't have a TODO comment here\.

## The hard life of Java programmers without compiler directives

[V6048](https://pvs-studio.com/en/docs/warnings/v6048/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) This expression can be simplified\. Operand 'lParenIndex' in the operation equals 0\. ClassFileWriter\.java\(2658\)

```java
public class ClassFileWriter {
  ....
  private int[] createInitialLocals() {
    int[] initialLocals = new int[itsMaxLocals];
    ....
    // No error checking should be necessary, sizeOfParameters does this
    String type = itsCurrentMethod.getType();
    int lParenIndex = type.indexOf('(');
    int rParenIndex = type.indexOf(')');
    if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
    }
    int start = lParenIndex + 1;                                  // <=
    StringBuilder paramType = new StringBuilder();
    while (start < rParenIndex) {
      switch (type.charAt(start)) {
      ....
      }
      ....
    }
    return initialLocals;  
  }
  ....
}
```

The string returned by the *itsCurrentMethod\.getType* method needs to be valid, namely, start with an opening parenthesis "\(" and have a closing parenthesis "\)"\. This follows from the comment before this method is called\. However, someone decided to play it safe and added *if* with checks of the values of the *lParenIndex* and *rParenIndex* variables — if one of these checks turns out to be true, an exception will be generated\. As a result, after *if*, the *lParenIndex* variable's value is now always 0\. This is what the analyzer discovered\.

I studied in detail all places where objects of *ClassFileMethod* type are created\. These objects are stored in *itsCurrentMethod* field\. I have come to the conclusion that the *lParenIndex \!= 0 \|\| rParenIndex < 0* condition will never be executed\. In all cases, the string stored in the *itsCurrentMethod\.type* field starts with the opening parenthesis '\(' and always contains one closing parenthesis '\)'\. 

For better code readability I would initialize the *start* variable with just 1:

```java
private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');
  if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
  }
  int start = 1; // <=
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}
```

Or, you could add a static private final *DEBUG* boolean field, and describe what it is for in the comment above the field\. Moreover, developers have already done this in some classes — the PVS\-Studio analyzer warned about unreachable code \([V6019](https://pvs-studio.com/en/docs/warnings/v6019/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990)\)\. During development, to check the value of *type*, the *DEBUG* field would have to be set to *true*, and during the release – to *false*:

```java
// set the true value if you need to check 
// currentMethodType in the createInitialLocals method
private static final boolean DEBUG = false;
....
private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');
  
  if (DEBUG &&                             // <=
     (lParenIndex != 0 || rParenIndex < 0)
  ) {
    throw new IllegalArgumentException("bad method type");
  }

  int start = 1; 
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}
```

## A caller and an argument are the same object

[V6100](https://pvs-studio.com/en/docs/warnings/v6100/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) An object is used as an argument to its own method\. Consider checking the first actual argument of the 'setParentScope' method\. Scope\.java\(123\)

```java
public class Scope extends Jump {
  ....
  public static Scope splitScope(Scope scope) {
    Scope result = new Scope(scope.getType());
    result.symbolTable = scope.symbolTable;
    scope.symbolTable = null;
    result.parent = scope.parent;
    result.setParentScope(scope.getParentScope());
    result.setParentScope(result);                   // <= 
    scope.parent = result;
    result.top = scope.top;
    return result;
  }
  ....
}
```

In the *splitScope* method, the analyzer found it strange that the same *result* object is passed to the *setParentScope* method as an argument when the *result\.setParentScope\(result\)* is called\. This is strange because inside the method's body of the class instance, the current instance is always accessed via *this* anyway\. Moreover, the strangest thing is that the same method is called on the same object before the call pointed out by the analyzer\. However, in this case the argument is the result of the *scope\.getParentScope\(\)* expression\. If the *setParentScope* method were a normal setter method for the *parentScope* field, a repeated call of the *setParentScope* method would simply overwrite the previous value, which would 100% be an error\. However, it's not that simple\. Let's take a closer look at the *setParentScope* method:

```java
public void setParentScope(Scope parentScope) {
  this.parentScope = parentScope;
  this.top = parentScope == null ? (ScriptNode) this 
                                 : parentScope.top;
}
```

This method, besides saving the passed argument to the *parentScope* field, also has a side effect\. If the argument is *null*, a reference to the current instance of the object — *this* — is stored to the *top* field\. If the argument is not *null*, the *top* field stores a reference to the object from the *top* field of the passed argument\. 

If you display the effect of calls to setParentScope methods in the splitScope method when calling *scope\.getParentScope\(\)* returns null in the form of a table, you get the result:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/106yh0netsql0xfxw2s6.png)

If the *scope call\.getParentScope\(\)* call doesn't not return null, you get the following result:

![Image description](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/a0zl0rtq7hs12u8ijp37.png)

I cannot say for sure that the repeated call of the *result\.setParentScope\(result\) method* is an error, but it looks very strange\. I think, it is up to developers to review this code and fix it, if necessary\.

## Useful false\-positive positives

[V6106](https://pvs-studio.com/en/docs/warnings/v6106/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(796\) 

```java
public class ClassFileWriter {
  ....
  public void addInvoke(int theOpCode, 
                        String className, 
                        String methodName, 
                        String methodType) {
    if (DEBUGCODE) {
        System.out.println(
                "Add "
                        + bytecodeStr(theOpCode)
                        + ", "
                        + className
                        + ", "
                        + methodName
                        + ", "
                        + methodType);
    }
    int parameterInfo = sizeOfParameters(methodType);
    int parameterCount = parameterInfo >>> 16;  // <= not commented
    int stackDiff = (short) parameterInfo;      // <= analyzer warning

    ....
  }
  ....
}
```

[V6106](https://pvs-studio.com/en/docs/warnings/v6106/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(847\)

```java
public class ClassFileWriter {
  ....
  public void addInvokeDynamic(String methodName, 
                               String methodType, 
                               MHandle bsm, 
                               Object... bsmArgs) {
    if (DEBUGCODE) {
      System.out.println("Add invokedynamic, " +
                         methodName +
                         ", " +
                         methodType);
    }
    // JDK 1.7 major class file version is required for invokedynamic
    if (MajorVersion < 51) {
        throw new RuntimeException(
          "Please build and run with JDK 1.7 for invokedynamic support");
    }

    int parameterInfo = sizeOfParameters(methodType);
    // int parameterCount = parameterInfo >>> 16;  // <= commented
    int stackDiff = (short) parameterInfo;         // <= analyzer warning
    ....
  }
  ....
}
```

Finally, the last couple of warnings of this article\. Both warnings, in my opinion, are false\-positives\. However, looking through them, I found a real error\. The code at the beginning of the *addInvoke* and *addInvokeDynamic* methods is very similar, which means that it is likely that one of these methods was created by copying the other\. However, at the beginning of the *addInvokeDynamic* method, the line with a bitwise shift of the *parameterInfo* variable is commented out, while in the *addInvoke* method an identical line is not commented out\. Most likely, a developer forgot to remove the comment from the line where the *addInvokeDynamic* method is\. And due to this, it seems that the behavior of the *addInvokeDynamic* method will be incorrect\. This can lead to even more errors when the program is running, and these errors may potentially be overlooked\.

The found error has never manifested itself yet, because the *addInvokeDynamic* method is not called anywhere in the project\.

Removing this comment is all that is needed to ensure that no one will encounter incorrect behaviour in the future when the *addInvokeDynamic* public method is called:

```java
public class ClassFileWriter {
  ....
  public void addInvokeDynamic(....) {
    ....
    
    int parameterInfo = sizeOfParameters(methodType);
    int parameterCount = parameterInfo >>> 16;    // <= comment removed
    int stackDiff = (short) parameterInfo;          
    ....
  }
  ....
}
```

## Conclusion

As you can see, even a 25\-year\-old project that is used in many other projects and utilities can still hide errors\. The longer an error lives, the more resources are required to fix it\. Therefore, it is best if the bugs are detected at the stage of code writing — it will greatly reduce the potential negative effect of bugs\.  A regular use of code analysis tools \(not just one, but several at a time, preferably\), such as for instance, PVS\-Studio, helps in this difficult matter\.

If you are interested in the PVS\-Studio analyzer and you want to try it out on your project, just [download the distribution](https://pvs-studio.com/en/pvs-studio/try-free/?utm_source=website&utm_medium=dev_to&utm_campaign=article&utm_content=0990&utm_term=link_try-free) and feel free to use the analyzer during the trial period\.

P\.S\. I'm currently continuing research on projects written in Java that process JavaScript code, so stay tuned for more articles on project analysis\. As an alternative to JavaScript engines, I am also reviewing [TypeScript Compiler API](https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API) as a tool for writing a static analyzer for JavaScript\. This is due to the fact that TypeScript is a superset of JavaScript\. I'd love to hear in the comments what diagnostics for JavaScript you'd like to see\.