Checking IronPython and IronRuby with PVS-Studio

Most recently, we released a new version of our PVS-Studio analyzer with support for checking C # projects. While at the time of release the further development of the product was suspended, I was engaged in testing the analyzer. As projects for my experiments, I took IronPython and IronRuby. And since these projects have been verified, I decided to write a small article report.

IronPython and IronRuby

IronPython and IronRuby are an implementation of the Python and Ruby programming languages ​​on the .NET platform. The source code for these projects is available on GitHub at this link . Also included is the source code DLR . Since the .NET Framework 4.0, DLR is part of it, and IronPython and IronRuby use it. Nevertheless, I still checked the old version of the DLR, since it was there.

About project validation

So, the whole code consists of three large parts: DLR, IronPython and IronRuby and contains 1630 * .cs files. For verification, I used PVS-Studio 6.00, which can be downloaded from our website . Checking the solution took a little more than a minute. As a result, the analyzer issued 34 warnings of the first, 15 warnings of the second and 280 warnings of the third level.
')
At the first level of 34 warnings, 19 turned out to be real mistakes - a pretty good result, 6 places look suspicious - they should be paid attention to. The remaining 9 messages are false positives, half of which can be eliminated by editing the analyzer itself, which we will do in the near future.

At the second and third level errors and suspicious places were found significantly less.

Bugs found

And now let's look at examples of real errors found with PVS-Studio:

Examples 1 and 2. Inattention.

private bool Enter(RangeExpression/*!*/ node, bool isCondition) { .... if (!isCondition && litBegin != null && litEnd != null && litBegin.Value is int && litBegin.Value is int) { _result = MakeNode(NodeKind.lit, new Range( (int)litBegin.Value, (int)litEnd.Value, node.IsExclusive)); } else { .... } .... } 

PVS-Studio warning : V3001 There are identical sub-expressions' litBegin.Value is the '&&' operator. IronRubyParseTreeOps.cs 277

The condition checks twice that litBegin.Value is of type int instead of also checking litEnd.Value.

Similar tests of the same expressions are present in two more places, for example, here:

 private static PythonTuple ReduceProtocol2( CodeContext/*!*/ context, object self) { .... if (self is PythonDictionary || self is PythonDictionary) { dictIterator = PythonOps.Invoke(context, self, "iteritems", ArrayUtils.EmptyObjects); } .... } 

PVS-Studio warning : V3001 There are no sub-expressions. operator. IronPython ObjectOps.cs 452

Example 3. Identical expressions.

 protected override MSAst.Expression VisitTry( MSAst.TryExpression node) { .... if (newHandlers != null || newFinally != null) { node = Ast.MakeTry(node.Type, node.Body, newFinally != null ? newFinally : node.Finally, node.Fault, newHandlers != null ? newHandlers : newHandlers ); } return node; } 

PVS-Studio warning : V3012 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: newHandlers. DebugInfoRewriter.cs 252

Here newHandlers is used in both branches of the conditional expression. It was supposed to use node.Handlers in case newHandlers is null.

Examples 4 and 5. Inattention.

 public static bool HasValue(RubyContext/*!*/ context, object/*!*/ self, object value) { var strValue = value as MutableString; if (value == null) { return false; } var clrStrValue = strValue.ConvertToString(); .... } 

PVS-Studio warning : V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'value', 'strValue'. EnvironmentSingletonOps.cs 189

This is a fairly common mistake when, due to carelessness after type casting, using the 'as' operator, the result is not checked for null, but the original object, and then use an unchecked reference.

Here is another similar case:

 private static RubyRegex/*!*/ ConstructRubyRegexp( RubyConstructor/*!*/ ctor, Node/*!*/ node) { ScalarNode scalar = node as ScalarNode; if (node == null) { throw RubyExceptions.CreateTypeError( "Can only create regex from scalar node"); } Match match = _regexPattern.Match(scalar.Value); .... } 

PVS-Studio warning : V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'scalar'. RubyConstructor.cs 230

Example 6. Copy-Paste.

 private void LoadNewObj(CodeContext/*!*/ context) { PythonTuple args = PopStack() as PythonTuple; if (args == null) { throw PythonOps.TypeError("expected second argument, got {0}", DynamicHelpers.GetPythonType(args)); } PythonType cls = PopStack() as PythonType; if (args == null) { throw PythonOps.TypeError("expected first argument, got {0}", DynamicHelpers.GetPythonType(args)); } .... } 

PVS-Studio warning : V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the statement is senseless. cPickle.cs 2194

In the above code snippet, the two conditions and the calls to the GetPythonType () function are exactly the same. Obviously, the second condition was obtained by copying the first one, but the author forgot to change the variable name. There are a couple of similar situations in the project.

Example 7. Same conditions.

 public static int Compare(SourceLocation left, SourceLocation right) { if (left < right) return -1; if (right > left) return 1; return 0; } 

PVS-Studio warning : V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the statement is senseless. SourceLocation.cs 156

This method is quite simple, and it seems that there is nowhere to be mistaken. Nevertheless, in the second condition, the parameters left and right were swapped for some reason, as a result of which both conditions checked the same thing that the analyzer found.

The correct code is:

 public static int Compare(SourceLocation left, SourceLocation right) { if (left < right) return -1; if (left > right) return 1; return 0; } 

Example 8. Excess condition.

 private void WriteSingleQuoted(string text, bool split) { .... while (ending <= text.Length) { c = '\0'; if (ending < text.Length) { c = text[ending]; } if (spaces) { if (c == 0 || c != 32) { .... } 

PVS-Studio warning : V3023 Consider inspecting the 'c == 0 || c! = 32 'expression. The expression is misprint. Emitter.cs 308

First, the variable 'c' is assigned the default value - '\ 0'. Then, if you have not yet processed the entire string, 'c' gets the value of the next character. And at the end it is checked whether the default value in the 'c' variable remains or whether something is counted there but a space. In fact, a comparison with zero here is superfluous, since zero is already different from 32 (space code). This code does not lead to errors, but makes understanding difficult, so a comparison with zero can be thrown out. The analyzer found some more similar unnecessary checks in this project.

Examples 9 and 10. Incorrect format string.

A common problem with the use of the String.Format function is that the number and numbers of the format string parameters are not checked by the compiler against the number of parameters passed to String.Format. As a result, an incorrect string may be generated, or a FormatException exception will be thrown. Consider the examples.

 public T Current { get { try { return (T)enumerable.Current; } catch (InvalidCastException iex) { throw new InvalidCastException(string.Format( "Error in IEnumeratorOfTWrapper.Current. Could not cast: {0} in {0}", typeof(T).ToString(), enumerable.Current.GetType().ToString()), iex); } } } 

PVS-Studio warning : V3025 Incorrect format. A different number of formatted items is expected while calling 'Format' function. Expected: 1. Present: 2. ConversionWrappers.cs 235

In this example, the last parameter is not used, but instead the typeof (T) .ToString () value will be printed twice.

 private static void DumpGenericParameters( MetadataTableView genericParams, MetadataRecord owner) { foreach (GenericParamDef gp in genericParams) { _output.WriteLine(" generic parameter #{0}: {1}", gp.Index, gp.Name, gp.Attributes); .... } 

PVS-Studio warning : V3025 Incorrect format. A different number of items is expected while calling the 'WriteLine' function. Expected: 2. Present: 3. Program.cs 268

And in this code snippet, the WriteLine function is passed one more parameter than the format string requires.

Example 11. Check for null after access.

 public static MutableString ChompInPlace(....) { MutableString result = InternalChomp(self, separator); if (result.Equals(self) || result == null) { self.RequireNotFrozen(); return null; } .... } 

PVS-Studio warning : V3027 The variable "result" was used. MutableStringOps.cs 1097

In this condition, you should swap the check for null and call the Equals method. In this form, the application can now fall with a NullReferenceException.

Example 12. Problems with synchronization.

 class DictThreadGlobalState { public int DoneCount; .... } private static void RunThreadTest(DictThreadGlobalState globalState) { .... globalState.DoneEvent.Reset(); globalState.Event.Set(); while (globalState.DoneCount != 0) { // wait for threads to get back to finish } .... } 

PVS-Studio warning : V3032 Waiting on this expression is unreliable, as the compiler may optimize some of the variables. Use volatile variable (s) or synchronization primitives to avoid this. EngineTest.cs 2558

This code contains an error, which may not always manifest itself, but depending on the runtime environment, the version of the .NET Framework, the number of processors in the system, or even some implementation details. Such errors can be very difficult to catch. The reason is that the DoneCount variable is not declared as volatile, and if so, the compiler believes that it is used only by one thread, and its value can, for example, be cached and returned all the time from the cache, since within the loop this variable does not change . But in our case, its value changes in another thread, so in such cases, when a variable is used to synchronize threads, it must be declared as volatile. Read more about this in MSDN .

Example 13. Dual Assignment

 private static Dictionary<string, EncodingInfoWrapper> MakeCodecsDict() { .... switch (normalizedName) { case "iso_8859_1": d["8859"] = d["latin_1"] = d["latin1"] = d["iso 8859_1"] = d["iso8859_1"] = d["cp819"] = d["819"] = d["latin"] = d["latin1"] = d["l1"] = encs[i]; break; .... } 

PVS-Studio warning : V3005 The 'd ["latin1"]' variable is assigned to itself. StringOps.cs 1905

In this code, the value is assigned twice to the variable d ["latin1"]. Most likely, this is just an extra code, not an error. But maybe they forgot about some code page. Anyway worth a look.

Example 14. Comparing unsigned variable with zero

 public static int __hash__(UInt64 x) { int total = unchecked((int) (((uint)x) + (uint)(x >> 32))); if (x < 0) { return unchecked(-total); } return total; } 

PVS-Studio warning : V3022 Expression 'x <0' is always false. Unsigned type value is always> = 0. IntOps.Generated.cs 1967

Probably, it was necessary to compare 'total' with zero, not 'x', because it is strange to always perform some actions with 'x', and then check the particular case. Yes, and 'total' has a signed type, so comparing “total <0” looks more logical.

Example 15. Identical checks.

 public void ReflectTypes(Type[]/*!*/ allTypes) { .... def.Super = null; if (cls != null && def.Extends != typeof(BasicObject) && !def.Extends.IsInterface) { if (cls != null && cls.Inherits != null) { def.Super = new TypeRef(cls.Inherits); .... } 

PVS-Studio warning : V3030 Recurring check. The 'cls! = Null' condition was already verified in line 373. LibraryDef.cs 374

Here, in both conditions, the variable 'cls' is checked for null. Most likely, the author wanted to check 'def' for null in the first condition, since there he immediately accesses his Extends property. But doing this would also be unnecessary, because null is written right in front of the condition in 'def.Super', which means that 'def' is no longer null. In general, this is just an extra check.

Example 16. Copy-paste.

I got to the third level errors, of which only 280 pieces. Of these, the overwhelming majority are warnings that the bodies of the two functions are the same, and warnings about the comparison of floating point numbers. I did not think that I could find something serious here, so I began to skim through the errors, and nevertheless I found it.

 public static bool IsPositiveOne(BigDecimal x) { return IsOne(x) && IsPositive(x); } public static bool IsNegativeOne(BigDecimal x) { return IsOne(x) && IsPositive(x); } 

PVS-Studio warning : V3013 It’s odd that the body of the body is 'IsPositiveOne' function is fully equivalent to the body of the 'IsNegativeOne' function (351, line 355). BigDecimal.cs 351

This is really a real mistake, which is the result of copying code from one function to another. The correct code should be:

 public static bool IsNegativeOne(BigDecimal x) { return IsOne(x) && IsNegative(x); } 

Example 17 Strange NaN test.

 public static bool Equals(float x, float y) { if (x == y) { return !Single.IsNaN(x); } return x == y; } 

PVS-Studio warning : V3024 An odd precise comparison: x == y. Consider using a precision with a precision: Math.Abs ​​(A - B) <Epsilon. FloatOps.cs 1048

I did not understand what is the meaning of a special test on NaN. If the condition (x == y) is met, then this means that both 'x' and 'y' are different from NaN, because NaN is not equal to any other value, including itself. That is, the first return will always return true. It seems that checking for NaN is just superfluous.

Conclusion

According to the results of the verification of the project, I was pleased with the work of the analyzer, because, first, he found a couple of dozen real errors, after correcting which, the project code will become better. And secondly, I identified several false positives that can be eliminated, thereby improving our product at the same time. Therefore, I recommend everyone to download the demo version of PVS-Studio and check its code.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Ilya Ivanov. Analyzing IronPython and IronRuby with PVS-Studio .