Совсем недавно мы выпустили новую версию нашего анализатора PVS-Studio с поддержкой проверки C# проектов. Пока на время релиза дальнейшая разработка продукта была приостановлена, я занимался тестированием анализатора. В качестве проектов для своих экспериментов я взял IronPython и IronRuby. А раз эти проекты были проверены, я решил написать небольшую статью-отчёт.

IronPython и IronRuby

IronPython и IronRuby представляют собой реализацию языков программирования Python и Ruby на платформе .NET. Исходный код этих проектов доступен на GitHub по этой ссылке. Также в комплекте идёт исходный код DLR. Начиная с .NET Framework 4.0 DLR является его частью, и IronPython и IronRuby используют её. Тем не менее я всё равно проверил старую версию DLR, раз уж она там оказалась.

О проверке проектов

Итак, весь код состоит из трёх больших частей: DLR, IronPython и IronRuby и содержит 1630 *.cs файлов. Для проверки я использовал PVS-Studio 6.00, которую можно скачать с нашего сайта. Проверка решения заняла чуть больше минуты. В результате анализатор выдал 34 предупреждений первого, 15 предупреждений второго и 280 предупреждений третьего уровня.

На первом уровне из 34 предупреждений 19 оказались настоящими ошибками — довольно хороший результат, 6 мест выглядят подозрительно — на них стоит обратить внимание. Оставшиеся 9 сообщений являются ложными срабатываниями, половину из которых можно устранить правкой самого анализатора, что мы сделаем в ближайшее время.

На втором и третьем уровне ошибок и подозрительных мест нашлось значительно меньше.

Найденные ошибки

А теперь рассмотрим примеры реальных ошибок, найденных с помощью PVS-Studio:

Примеры 1 и 2. Невнимательность.

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: V3001 There are identical sub-expressions 'litBegin.Value is int' to the left and to the right of the '&&' operator. IronRubyParseTreeOps.cs 277

В условии дважды проверяется что litBegin.Value имеет тип int вместо того, чтобы также проверить litEnd.Value.

Аналогичные проверки одних и тех же выражений присутствуют ещё в двух местах, например, здесь:

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: V3001 There are identical sub-expressions 'self is PythonDictionary' to the left and to the right of the '||' operator. IronPython ObjectOps.cs 452

Пример 3. Одинаковые выражения.

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: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: newHandlers. DebugInfoRewriter.cs 252

Здесь newHandlers используется в обеих ветках условного выражения. Предполагалось использовать node.Handlers в случае если newHandlers будет null.

Примеры 4 и 5. Невнимательность.

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: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'value', 'strValue'. EnvironmentSingletonOps.cs 189

Это довольно распространённая ошибка, когда из-за невнимательности после приведения типа с помощью оператора 'as' проверяют на null не результат приведения, а исходный объект, а потом используют непроверенную ссылку.

Вот ещё один похожий случай:

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: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'scalar'. RubyConstructor.cs 230

Пример 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: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless. cPickle.cs 2194

В приведённом фрагменте кода два условия и вызовы функции GetPythonType() абсолютно одинаковые. Очевидно, что второе условие было получено копированием первого, но изменить имя переменной автор забыл. В проекте встречается ещё пара похожих ситуаций.

Пример 7. Одинаковые условия.

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

Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless. SourceLocation.cs 156

Этот метод довольно простой, и кажется, что ошибиться здесь негде. Тем не менее во втором условии параметры left и right зачем-то поменяли местами, в результате чего оба условия проверяют одно и то же, что и обнаружил анализатор.

Корректный вариант кода:

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

Пример 8. Избыточное условие.

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: V3023 Consider inspecting the 'c == 0 || c != 32' expression. The expression is excessive or contains a misprint. Emitter.cs 308

Сначала переменной 'c' присваивается значение по умолчанию — '\0'. Затем, если ещё не обработали всю строку, 'c' получает значение очередного символа. И в конце проверяется, осталось ли в переменной 'c' значение по умолчанию или туда считали что-то кроме пробела. На самом деле сравнение с нулём здесь лишнее, так как ноль и так отличен от 32 (код пробела). Этот код не приводит к ошибкам, но затрудняет понимание, поэтому сравнение с нулём можно выкинуть. Анализатор нашёл ещё несколько аналогичных лишних проверок в этом проекте.

Примеры 9 и 10. Некорректная строка форматирования.

Общая проблема при использовании функции String.Format заключается в том, что количество и номера параметров строки форматирования не проверяется компилятором на соответствие количеству параметров, переданных в String.Format. В результате может быть либо сформирована неправильная строка, либо будет выброшено исключение FormatException. Рассмотрим примеры.

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: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 1. Present: 2. ConversionWrappers.cs 235

В этом примере последний параметр не используется, а вместо этого два раза будет выведено значение typeof(T).ToString().

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: V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Expected: 2. Present: 3. Program.cs 268

А в этом фрагменте кода в функцию WriteLine передаётся на один параметр больше, чем требует строка форматирования.

Пример 11. Проверка на null после доступа.

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

Предупреждение PVS-Studio: V3027 The variable 'result' was utilized in the logical expression before it was verified against null in the same logical expression. MutableStringOps.cs 1097

В этом условии следует поменять местами проверку на null и вызов метода Equals. В таком виде как сейчас приложение может упасть с NullReferenceException.

Пример 12. Проблемы с синхронизацией.

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: V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. EngineTest.cs 2558

Этот код содержит ошибку, которая может проявляться не всегда, а в зависимости от среды выполнения, версии .NET Framework, количества процессоров в системе или ещё от каких-то деталей реализации. Такие ошибки бывает очень сложно отловить. Причина в том, что переменная DoneCount не объявлена как volatile, а раз так, то компилятор считает, что она используется только одним потоком, и её значение можно, например, закешировать и отдавать всё время из кеша, так как внутри цикла эта переменная не меняется. Но в нашем случае её значение изменяется в другом потоке, поэтому в таких случаях, когда переменная используется для синхронизации потоков, её необходимо объявлять, как volatile. Подробнее об этом можно прочитать в MSDN.

Пример 13. Двойное присваивание

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: V3005 The 'd[«latin1»]' variable is assigned to itself. StringOps.cs 1905

В этом коде дважды присваивается значение в переменную d[«latin1»]. Скорее всего, это просто лишний код, а не ошибка. Но возможно тут забыли про какую-то кодовую страницу. В любом случае стоит взглянуть.

Пример 14. Сравнение беззнаковой переменной с нулём

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: V3022 Expression 'x < 0' is always false. Unsigned type value is always >= 0. IntOps.Generated.cs 1967

Вероятно, с нулём надо было сравнить 'total', а не 'x', потому что странно выполнять какие-то действия с 'x' всегда, а потом проверять частный случай. Да и 'total' имеет знаковый тип, так что сравнение «total < 0» выглядит логичней.

Пример 15. Одинаковые проверки.

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: V3030 Recurring check. The 'cls != null' condition was already verified in line 373. LibraryDef.cs 374

Тут в обоих условиях переменная 'cls' проверяется на null. Скорее всего автор хотел проверить 'def' на null в первом условии, так как там он сразу обращается к его свойству Extends. Но это делать тоже было бы необязательно, потому что прямо перед условием в 'def.Super' записывается null, а это значит, что 'def' уже не null. В общем, это просто лишняя проверка.

Пример 16. Copy-paste.

Я дошёл до ошибок третьего уровня, которых всего 280 штук. Из них подавляющее большинство — это предупреждения о том, что тела двух функций совпадают, и предупреждения о сравнении чисел с плавающей точкой. Я не думал, что смогу найти здесь что-то серьёзное, поэтому начал бегло просматривать ошибки, и тем не менее нашёл.

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

Предупреждение PVS-Studio: V3013 It is odd that the body of 'IsPositiveOne' function is fully equivalent to the body of 'IsNegativeOne' function (351, line 355). BigDecimal.cs 351

Это действительно реальная ошибка, которая является результатом копирования кода из одной функции в другую. Правильный вариант кода должен быть таким:

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

Пример 17 Странная проверка на NaN.

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

Предупреждение PVS-Studio: V3024 An odd precise comparison: x == y. Consider using a comparison with defined precision: Math.Abs(A — B) < Epsilon. FloatOps.cs 1048

Я не понял в чём тут смысл специальной проверки на NaN. Если условие (x == y) выполняется, то это значит, что и 'x', и 'y' отличны от NaN, потому что NaN не равен никакому другому значению, в том числе и самому себе. То есть первый return всегда будет возвращать true. Похоже, что проверка на NaN просто лишняя.

Заключение

По результатам проверки проекта я остался доволен работой анализатора, так как, во-первых, он нашёл пару десятков реальных ошибок, после исправления которых код проекта станет лучше. А во-вторых, я выявил несколько ложных срабатываний, которые можно устранить, улучшив тем самым заодно и наш продукт. Поэтому рекомендую всем скачать демонстрационную версию PVS-Studio и проверить свой код.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ilya Ivanov. Analyzing IronPython and IronRuby with PVS-Studio.