# The check of the Rhino JavaScript engine or how the unicorn met the rhino 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\. ![0990_RhinoEngine/image1.png](https://import.viva64.com/docx/blog/0990_RhinoEngine/image1.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/) — 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/)\. Here are the most striking ones: * In my opinion, all [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) 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/) 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/) 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/) 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: ```cpp leftDocked && t1Docked && rightDocked && t2Docked ``` To fix the current code fragment, I would recommend using _t2Docked_ once in the condition\. For example, as follows: ```cpp t2Docked && leftDocked && rightDocked ``` ## Copy paste, impossible conditions, and unreachable code [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'power < 2' is always false\. UintMap\.java\(39\) ```cpp 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/) Expression 'power < 2' is always false\. ObjToIntMap\.java\(102\) ```cpp 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: ```cpp if (check && power < 2) Kit.codeBug(); ``` If this check is true, the _Kit\.codeBug_ method is called and it simply throws an exception: ```cpp 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/) Expression '\!BUG\_369394\_IS\_VALID' is always true\. XmlProcessor\.java\(288\) ```cpp class XmlProcessor implements Serializable { .... private void addTextNodesToRemoveAndTrim(List 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/) Expression 'i < indentLevel' is always false\. XmlProcessor\.java\(454\) ```cpp 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/) Potential null dereference of 'generatorState' in function 'freezeGenerator'\. Interpreter\.java\(1244\), Interpreter\.java\(3188\) ```cpp 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: ```cpp CallFrame cloneFrozen() { .... copy.frozen = false; // <= return copy; } ``` **Second**\. The _freezeGenerator_ method will be called only if _frame\.frozen_ is false: ```cpp .... 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_: ```cpp 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: ```cpp 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_: ```cpp 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_: ```cpp .... 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: ```cpp 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/) Unreachable code detected\. It is possible that an error is present\. Parser\.java\(3138\) ```cpp 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/) The value is assigned to the 'localMax' variable but is not used\. NativeRegExp\.java\(568\) ```cpp 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: ```cpp (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: ```cpp .... 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: ```cpp 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/) The value is assigned to the 'nameString' variable but is not used\. QName\.java\(293\) ```cpp 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/) diagnostics detected that the result of the expression is assigned to the _nameString_ variable: ```cpp ((QName) name).localName() ``` However, further in both branches of _if\.\.\.else_, the _nameString_ value is reassigned before it is used: ```cpp 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/) Index 'sig1\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(461\) [V6025](https://pvs-studio.com/en/docs/warnings/v6025/) Index 'sig2\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(462\) ```cpp 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: ```cpp 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/) Possibly index 'last \- 1' is out of bounds\. SuperBlock\.java\(50\) ```cpp 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: ```cpp 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: ```cpp 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/) Identical expression 'end' to the left and to the right of compound assignment\. FastDtoaBuilder\.java\(74\) ```cpp 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: ```cpp 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/) 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\) ```cpp public class Node implements Iterable { .... 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/) 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/) This expression can be simplified\. Operand 'lParenIndex' in the operation equals 0\. ClassFileWriter\.java\(2658\) ```cpp 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: ```cpp 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/)\)\. During development, to check the value of _type_, the _DEBUG_ field would have to be set to _true_, and during the release – to _false_: ```cpp // 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/) An object is used as an argument to its own method\. Consider checking the first actual argument of the 'setParentScope' method\. Scope\.java\(123\) ```cpp 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: ```cpp 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: ![0990_RhinoEngine/image2.png](https://import.viva64.com/docx/blog/0990_RhinoEngine/image2.png) If the _scope call\.getParentScope\(\)_ call doesn't not return null, you get the following result: ![0990_RhinoEngine/image3.png](https://import.viva64.com/docx/blog/0990_RhinoEngine/image3.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/) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(796\) ```cpp 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/) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(847\) ```cpp 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: ```cpp 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/) 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\.