Хабрахабр

Какие ошибки прячутся в коде Infer.NET?

Публикация корпорацией Microsoft исходников своих проектов является вполне хорошим поводом для их проверки. Этот раз исключением не стал, и сегодня мы посмотрим на подозрительные места, найденные в коде Infer.NET. Долой аннотацию – ближе к делу!

Немного о проекте и анализаторе

Infer.NET – система машинного обучения, разрабатываемая специалистами из Microsoft. Исходный код проекта недавно стал доступен на GitHub, что и послужило поводом к его проверке. Более подробно о проекте можно почитать, например, здесь.

26. Проверялся проект с помощью статического анализатора PVS-Studio версии 6. C# код пока анализируем только под Windows. Напомню, что PVS-Studio занимается поиском ошибок в коде на C\C++\C# (а скоро и на Java) под Windows, Linux, macOS. Анализатор можно скачать и попробовать на своём проекте.

Предварительно я выгрузил проект с GitHub, восстановил требуемые пакеты (зависимости) и убедился, что проект успешно собирается. Сама проверка прошла предельно просто и без проблем. После сборки в пару кликов запустил анализ solution через плагин PVS-Studio для Visual Studio. Это требуется для того, чтобы анализатору была доступна вся необходимая информация для проведения полноценного анализа.

Кстати, это не первый проект от Microsoft, проверенный с помощью PVS-Studio – были и другие: Roslyn, MSBuild, PowerShell, CoreFX и прочие.

Если вам или вашим знакомым интересен анализ Java кода — можете написать нам в поддержку, выбрав пункт «Хочу анализатор для Java». Примечание. Где-то в секретной лаборатории (через стенку) ребята активно над ней трудятся. Публичной beta-версии анализатора пока нет, но скоро должна появиться.

Но хватит отвлечённых разговоров – давайте посмотрим на проблемы в коде.

Это баг или фича?

Предлагаю попробовать найти ошибку самостоятельно – вполне решаемая задача. Никаких подколов в духе того, что было в статье "Toп 10 ошибок в C++ проектах за 2017 год", честно. Так что не спешите читать предупреждение анализатора, представленное после фрагмента кода.

private void MergeParallelTransitions()
else if (!transition1.IsEpsilon && !transition2.IsEpsilon) { .... if (double.IsInfinity(transition1.Weight.Value) && double.IsInfinity(transition1.Weight.Value)) { newElementDistribution.SetToSum( 1.0, transition1.ElementDistribution, 1.0, transition2.ElementDistribution); } else { newElementDistribution.SetToSum( transition1.Weight.Value, transition1.ElementDistribution, transition2.Weight.Value, transition2.ElementDistribution); } ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'double.IsInfinity(transition1.Weight.Value)' to the left and to the right of the '&&' operator. Runtime Automaton.Simplification.cs 479

Использование схожих имён иногда вполне оправданно, но стоит помнить, что в таком случае возрастает вероятность случайно ошибиться где-нибудь с именем. Как видно из фрагмента кода, в методе идёт работа с парой переменных – transition1 и transition2.

IsInfinity). Так и произошло при проверке чисел на бесконечность (double. Weight. Из-за ошибки 2 раза проверили значение одной и той же переменной — transition1. Проверяемым значением во втором подвыражении должна была стать переменная transition2. Value. Value. Weight.

Ещё один схожий подозрительный код.

internal MethodBase ToMethodInternal(IMethodReference imr)
{ .... bf |= BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance; ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'BindingFlags.Public' to the left and to the right of the '|' operator. Compiler CodeBuilder.cs 194

Public. При формировании значения переменной bf дважды используется элемент перечисления BindingFlags. Public должно быть другое значение перечисления. Либо этот код содержит лишнюю операцию выставления флага, либо вместо второго использования BindingFlags.

Мне кажется, что если он отформатирован в табличном стиле (как здесь), проблему обнаружить проще. Кстати, в исходниках этот код записан в одну строку.

Привожу тело метода целиком и вновь предлагаю вам найти ошибку (а может – ошибки) самостоятельно. Идём дальше.

private void ForEachPrefix(IExpression expr, Action<IExpression> action)
{ // This method must be kept consistent with GetTargets. if (expr is IArrayIndexerExpression) ForEachPrefix(((IArrayIndexerExpression)expr).Target, action); else if (expr is IAddressOutExpression) ForEachPrefix(((IAddressOutExpression)expr).Expression, action); else if (expr is IPropertyReferenceExpression) ForEachPrefix(((IPropertyReferenceExpression)expr).Target, action); else if (expr is IFieldReferenceExpression) { IExpression target = ((IFieldReferenceExpression)expr).Target; if (!(target is IThisReferenceExpression)) ForEachPrefix(target, action); } else if (expr is ICastExpression) ForEachPrefix(((ICastExpression)expr).Expression, action); else if (expr is IPropertyIndexerExpression) ForEachPrefix(((IPropertyIndexerExpression)expr).Target, action); else if (expr is IEventReferenceExpression) ForEachPrefix(((IEventReferenceExpression)expr).Target, action); else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); else if (expr is IMethodInvokeExpression) ForEachPrefix(((IMethodInvokeExpression)expr).Method, action); else if (expr is IMethodReferenceExpression) ForEachPrefix(((IMethodReferenceExpression)expr).Target, action); else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); else if (expr is IDelegateInvokeExpression) ForEachPrefix(((IDelegateInvokeExpression)expr).Target, action); action(expr);
}

Нашли? Сверяемся!

Предупреждения PVS-Studio:

  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1719, 1727. Compiler CodeRecognizer.cs 1719
  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1721, 1729. Compiler CodeRecognizer.cs 1721

Немного упростим код, чтобы проблемы стали более очевидны.

private void ForEachPrefix(IExpression expr, Action<IExpression> action)
{ if (....) .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action) ....
}

Дублируются условные выражения и then-ветви нескольких операторов if. Возможно, этот код писался методом copy-paste, из-за чего и возникла проблема. Сейчас получается, что then-ветви дубликатов никогда не выполнятся, так как:

  • если условное выражение истинно, выполняется тело первого оператора if из соответствующей пары;
  • если условное выражение ложно в первом случае, оно будет ложно и во втором.

Так как в then-ветвях содержатся одинаковые действия, сейчас это выглядит как избыточный код, который сбивает с толку. Возможно, что тут проблема иного рода – вместо дублей должны были выполняться другие проверки.

Продолжаем.

public int Compare(Pair<int, int> x, Pair<int, int> y)
{ if (x.First < y.First) { if (x.Second >= y.Second) { // y strictly contains x return 1; } else { // No containment - order by left bound return 1; } } else if (x.First > y.First) { if (x.Second <= y.Second) { // x strictly contains y return -1; } else { // No containment - order by left bound return -1; } } ....
}

Предупреждения PVS-Studio:

  • V3004 The 'then' statement is equivalent to the 'else' statement. Runtime RegexpTreeBuilder.cs 1080
  • V3004 The 'then' statement is equivalent to the 'else' statement. Runtime RegexpTreeBuilder.cs 1093

Код выглядит крайне подозрительно, так как содержит два условных оператора с одинаковыми телами then и else-ветвей. Вероятно, в обоих случаях стоит возвращать разные значения. Или же, если это задуманное поведение, будет полезно убрать избыточные условные операторы.

Пример ниже: Встречались интересные циклы.

private static Set<StochasticityPattern> IntersectPatterns(IEnumerable<StochasticityPattern> patterns)
{ Set<StochasticityPattern> result = new Set<StochasticityPattern>(); result.AddRange(patterns); bool changed; do { int count = result.Count; AddIntersections(result); changed = (result.Count != count); break; } while (changed); return result;
}

Предупреждение PVS-Studio: V3020 An unconditional 'break' within a loop. Compiler DefaultFactorManager.cs 474

В общем, код выглядит странно и подозрительно. Из-за безусловного оператора break выполняется ровно одна итерация цикла, а управляющая переменная changed даже не используется.

Соответствующее предупреждение анализатора: V3020 An unconditional 'break' within a loop. Такой же метод (точная копия) встретился и в другом классе. Windows FactorManagerView.cs 350 Visualizers.

Кстати, встретился метод с безусловным оператором continue в цикле (его нашёл анализатор этой же диагностикой), но над ним стоял комментарий, подтверждающий, что это специальное временное решение:

// TEMPORARY
continue;

Напоминаю, что около безусловного оператора break таких комментариев не было.

Идём дальше.

internal static DependencyInformation GetDependencyInfo(....)
{ .... IExpression resultIndex = null; .... if (resultIndex != null) { if (parameter.IsDefined( typeof(SkipIfMatchingIndexIsUniformAttribute), false)) { if (resultIndex == null) throw new InferCompilerException( parameter.Name + " has SkipIfMatchingIndexIsUniformAttribute but " + StringUtil.MethodNameToString(method) + " has no resultIndex parameter"); .... } .... } ....
}

Предупреждение PVS-Studio: V3022 Expression 'resultIndex == null' is always false. Compiler FactorManager.cs 382

Однако между проверками resultIndex != null и resultIndex == null значение уже поменяться не может. Сразу отмечу, что между объявлением и приведённой проверкой значение переменной resultIndex может измениться. Следовательно, результатом выражения resultIndex == null всегда будет значение false, а значит, исключение никогда сгенерировано не будет.

Код метода небольшой, приведу его целиком. Надеюсь, что интерес к самостоятельному поиску ошибок у вас есть и без моих предложений найти проблему, но на всякий случай предложу сделать это ещё раз.

public static Tuple<int, string> ComputeMovieGenre(int offset, string feature)
{ string[] genres = feature.Split('|'); if (genres.Length < 1 && genres.Length > 3) { throw new ArgumentException(string.Format( "Movies should have between 1 and 3 genres; given {0}.", genres.Length)); } double value = 1.0 / genres.Length; var result = new StringBuilder( string.Format( "{0}:{1}", offset + MovieGenreBuckets[genres[0]], value)); for (int i = 1; i < genres.Length; ++i) { result.Append( string.Format( "|{0}:{1}", offset + MovieGenreBuckets[genres[i].Trim()], value)); } return new Tuple<int, string>(MovieGenreBucketCount, result.ToString());
}

Давайте посмотрим, что здесь происходит. Входная строка парсится по символу '|'. Если длина массива не соответствует ожидаемой, нужно сгенерировать исключение. Секундочку… genres.Length < 1 && genres.Length > 3? Так как нет числа, которое попадало бы сразу в оба требуемых выражением диапазона значений ([int.MinValue..1) и (3..int.MaxValue]), результатом выражения всегда будет значение false. Следовательно, данная проверка ни от чего не защищает, и ожидаемое исключение сгенерировано не будет.

Length < 1 && genres. Именно об этом и предупреждает анализатор: V3022 Expression 'genres. Probably the '||' operator should be used here. Length > 3' is always false. Evaluator Features.cs 242

Встретилась подозрительная операция деления.

public static void CreateTrueThetaAndPhi(....)
{ .... double expectedRepeatOfTopicInDoc = averageDocLength / numUniqueTopicsPerDoc; .... int cnt = Poisson.Sample(expectedRepeatOfTopicInDoc); ....
}

Предупреждение PVS-Studio: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. LDA Utilities.cs 74

Напрашивается вопрос: а специально ли это сделано, или всё же подразумевалось деление вещественных чисел? Подозрительно здесь вот что: выполняется целочисленное деление (переменные averageDocLength и numUniqueTopicsPerDoc имеют тип int), а результат записывается в переменную типа double. Если бы переменная expectedRepeatOfTopicInDoc имела тип int, это сняло бы возможные вопросы.

Sample, аргументом которого и выступает подозрительная переменная expectedRepeatOfTopicInDoc, используется, например, так, как описано ниже. В других местах метод Poisson.

int numUniqueWordsPerTopic = Poisson.Sample((double)averageWordsPerTopic);

averageWordsPerTopic имеет тип int, который уже на месте использования приводится к double.

А вот другое место использования:

double expectedRepeatOfWordInTopic = ((double)numDocs) * averageDocLength / numUniqueWordsPerTopic;
....
int cnt = Poisson.Sample(expectedRepeatOfWordInTopic);

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

В общем, стоит посмотреть исходное место, на которое анализатор выдал предупреждение.

К следующему подозрительном делению. Но размышления над тем, стоит ли это править, и как, оставим авторам кода (им же виднее), а сами пойдём дальше.

public static NonconjugateGaussian BAverageLogarithm(....)
{ .... double v_opt = 2 / 3 * (Math.Log(mx * mz / Ex2 / 2) - m); if (v_opt != v) { .... } ....
}

Предупреждение PVS-Studio: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Runtime ProductExp.cs 137

2 и 3 – целочисленные числовые литералы, а результатом выражения 2 / 3 будет 0. Анализатор вновь обнаружил подозрительную операцию целочисленного деления, т.к. В итоге всё выражение принимает вид:

double v_opt = 0 * expr;

Согласитесь, немного странно. Несколько раз я возвращался к данному предупреждению, пытаясь найти какой-то подвох, и не стремясь добавлять его в статью. Метод наполнен математикой и разными формулами (разбирать которые, если честно, не очень-то хотелось), мало ли чего тут ожидать. К тому же, я стараюсь максимально скептически относиться к предупреждениям, которые выписываю в статью, и описываю их только предварительно получше изучив.

Так что это место, в любом случае, стоит посмотреть. Но потом меня осенило – а зачем вообще нужен множитель в виде 0, записанный как 2 / 3?

public static void WriteAttribute(TextWriter writer, string name, object defaultValue, object value, Func<object, string> converter = null)
{ if ( defaultValue == null && value == null || value.Equals(defaultValue)) { return; } string stringValue = converter == null ? value.ToString() : converter(value); writer.Write($"{name}=\"{stringValue}\" ");
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'value'. Compiler WriteHelpers.cs 78

Разыменование нулевой ссылки может произойти в выражении value. Вполне справедливое утверждение анализатора на основе условия. Так как это выражение – правый операнд оператора ||, для его вычисления левый операнд должен иметь значение false, а для этого достаточно, чтобы хотя бы одна из переменных defaultValue \ value не была равна null. Equals(defaultValue), если value == null. В итоге, если defaultValue != null, а value == null:

  • defaultValue == null -> false;
  • defaultValue == null && value == null -> false; (проверка value не произошла)
  • value.Equals(defaultValue) -> NullReferenceException, так как valuenull.

Посмотрим ещё на схожий случай:

public FeatureParameterDistribution( GaussianMatrix traitFeatureWeightDistribution, GaussianArray biasFeatureWeightDistribution)
{ Debug.Assert( (traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count), "The provided distributions should be valid and consistent in the number of features."); ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'traitFeatureWeightDistribution'. Recommender FeatureParameterDistribution.cs 65

Выкинем лишнее, оставив только логику вычисления булева значения, чтобы было проще разобраться:

(traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null)
||
traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count)

Опять же, правый операнд оператора || вычисляется только в том случае, если результат вычисления левого имеет значение false. Левый операнд может принять значение false, в том числе, когда traitFeatureWeightDistribution == null и biasFeatureWeightDistribution != null. Тогда будет вычисляться правый операнд оператора ||, а вызов traitFeatureWeightDistribution.All приведёт к возникновению ArgumentNullException.

Ещё интересный фрагмент кода:

public static double GetQuantile(double probability, double[] quantiles)
{ .... int n = quantiles.Length; if (quantiles == null) throw new ArgumentNullException(nameof(quantiles)); if (n == 0) throw new ArgumentException("quantiles array is empty", nameof(quantiles)); ....
}

Предупреждение PVS-Studio: V3095 The 'quantiles' object was used before it was verified against null. Check lines: 91, 92. Runtime OuterQuantiles.cs 91

Length, а затем quantiles проверяется на равенство null. Обратите внимание, что сначала происходит обращение к свойству quantiles. Видимо, перепутали строки местами. В итоге, если quantiles == null, метод выбросит исключение, только немного не то, и немного не там, где это было необходимо.

Чтобы было чуточку интереснее, код метода привожу целиком. Если вы успешно справились с обнаружением приведённых раньше ошибок самостоятельно, предлагаю заварить себе чашечку кофе и попробовать повторить подвиг, найдя ошибку в приведённом ниже методе.

(Ссылка на фуллсайз)

Picture 2

Давайте немного упростим задачу: Ладно, ладно, это была шутка (или вам всё же удалось?!).

if (sample.Precision < 0)
{ precisionIsBetween = true; lowerBound = -1.0 / v; upperBound = -mean.Precision;
}
else if (sample.Precision < -mean.Precision)
{ precisionIsBetween = true; lowerBound = 0; upperBound = -mean.Precision;
}
else
{ // in this case, the precision should NOT be in this interval. precisionIsBetween = false; lowerBound = -mean.Precision; lowerBound = -1.0 / v;
}

Стало лучше? Анализатор выдал на данный код следующее предупреждение: V3008 The 'lowerBound' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 324, 323. Runtime GaussianOp.cs 324

Видимо (и судя по коду выше), в одном из присвоений должна участвовать переменная upperBound. И действительно, в последней else-ветви два раза подряд присваивается значение переменной lowerBound.

Следуем дальше.

private void WriteAucMatrix(....)
{ .... for (int c = 0; c < classLabelCount; c++) { int labelWidth = labels[c].Length; columnWidths[c + 1] = labelWidth > MaxLabelWidth ? MaxLabelWidth : labelWidth; for (int r = 0; r < classLabelCount; r++) { int countWidth = MaxValueWidth; if (countWidth > columnWidths[c + 1]) { columnWidths[c + 1] = countWidth; } } ....
}

Предупреждение PVS-Studio: V3081 The 'r' counter is not used inside a nested loop. Consider inspecting usage of 'c' counter. CommandLine ClassifierEvaluationModule.cs 459

Из-за этого выходит, что на протяжении всех итераций внутреннего цикла выполняются одни и те же операции над одними и теми же элементами – ведь в индексах также используется счётчик внешнего цикла (c), а не внутреннего (r). Обратите внимание, что счётчик внутреннего цикла – r – не используется в теле этого цикла.

Посмотрим, что ещё нашлось интересного.

public RegexpFormattingSettings( bool putOptionalInSquareBrackets, bool showAnyElementAsQuestionMark, bool ignoreElementDistributionDetails, int truncationLength, bool escapeCharacters, bool useLazyQuantifier)
{ this.PutOptionalInSquareBrackets = putOptionalInSquareBrackets; this.ShowAnyElementAsQuestionMark = showAnyElementAsQuestionMark; this.IgnoreElementDistributionDetails = ignoreElementDistributionDetails; this.TruncationLength = truncationLength; this.EscapeCharacters = escapeCharacters;
}

Предупреждение PVS-Studio: V3117 Constructor parameter 'useLazyQuantifier' is not used. Runtime RegexpFormattingSettings.cs 38

Особенно подозрительным это выглядит на фоне того, что в классе определено свойство с соответствующим именем и типом – UseLazyQuantifier. В конструкторе не используется один параметр – useLazyQuantifier. Видимо, забыли провести его инициализацию через соответствующий параметр.

Пример одного из них приведён ниже: Встретилось несколько потенциально опасных обработчиков событий.

public class RecommenderRun
{ .... public event EventHandler Started; .... public void Execute() { // Report that the run has been started if (this.Started != null) { this.Started(this, EventArgs.Empty); } .... } ....
}

Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'Started', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. Evaluator RecommenderRun.cs 115

Во избежание таких проблем можно, например, сохранять ссылку на цепочку делегатов в локальную переменную или использовать оператор '?.' для вызова обработчиков. Дело в том, что между проверкой на неравенство null и вызовом обработчика может произойти отписка от события, и, если в момент между проверкой на null и вызовом обработчиков у события не окажется подписчиков, будет сгенерировано исключение NullReferenceException.

Помимо приведённого выше фрагмента кода нашлось 35 подобных мест.

Предупреждение V3024 выдаётся при сравнении вещественных чисел с использованием операторов '!=' или '=='. Кстати, ещё встретилось 785 предупреждений V3024. Не буду здесь останавливаться на том, почему такие сравнения не всегда корректны – подробнее про это написано в документации, там же есть ссылка на StackOverflow (это она же).

Учитывая, что часто встречались формулы и вычисления, эти предупреждения также могут быть важны, хоть и вынесены на 3 уровень (так как актуальны далеко не во всех проектах).

Если же есть уверенность в том, что эти предупреждения неактуальны – можно убрать их практически одним щелчком, сократив общее количество срабатываний анализатора.

Заключение

Как-то так получилось, что я уже достаточно давно не писал статей о проверке проектов, и снова прикоснуться к этому процессу было достаточно приятно. Надеюсь, что и вы вынесли для себя из этой статьи что-то новое \ полезное, или хотя бы с интересом её прочитали.

Для того и нужны дополнительные инструменты вроде статических анализаторов, чтобы находить то, что пропустил человек, верно? Разработчикам желаю скорейшего исправления проблемных мест и напоминаю, что делать ошибки – это нормально, все же мы люди. Так или иначе – удачи с проектом, и спасибо за труд!

И помните, что максимальная польза от статического анализатора достигается при его регулярном использовании.

Всех благ!

What Errors Lurk in Infer. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. NET Code?

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть