Публикация корпорацией Microsoft исходников своих проектов является вполне хорошим поводом для их проверки. Этот раз исключением не стал, и сегодня мы посмотрим на подозрительные места, найденные в коде Infer.NET. Долой аннотацию – ближе к делу!
Немного о проекте и анализаторе
Infer.NET – система машинного обучения, разрабатываемая специалистами из Microsoft. Исходный код проекта недавно стал доступен на GitHub, что и послужило поводом к его проверке. Более подробно о проекте можно почитать, например, здесь.
Проверялся проект с помощью статического анализатора PVS-Studio версии 6.26. Напомню, что PVS-Studio занимается поиском ошибок в коде на CC++C# (а скоро и на Java) под Windows, Linux, macOS. C# код пока анализируем только под Windows. Анализатор можно скачать и попробовать на своём проекте.
Сама проверка прошла предельно просто и без проблем. Предварительно я выгрузил проект с GitHub, восстановил требуемые пакеты (зависимости) и убедился, что проект успешно собирается. Это требуется для того, чтобы анализатору была доступна вся необходимая информация для проведения полноценного анализа. После сборки в пару кликов запустил анализ solution через плагин PVS-Studio для Visual Studio.
Кстати, это не первый проект от Microsoft, проверенный с помощью PVS-Studio – были и другие: Roslyn, MSBuild, PowerShell, CoreFX и прочие.
Примечание. Если вам или вашим знакомым интересен анализ Java кода — можете написать нам в поддержку, выбрав пункт «Хочу анализатор для Java». Публичной beta-версии анализатора пока нет, но скоро должна появиться. Где-то в секретной лаборатории (через стенку) ребята активно над ней трудятся.
Но хватит отвлечённых разговоров – давайте посмотрим на проблемы в коде.
Это баг или фича?
Предлагаю попробовать найти ошибку самостоятельно – вполне решаемая задача. Никаких подколов в духе того, что было в статье "Toп 10 ошибок в C++ проектах за 2017 год", честно. Так что не спешите читать предупреждение анализатора, представленное после фрагмента кода.
private void MergeParallelTransitions()
{
....
if ( transition1.DestinationStateIndex ==
transition2.DestinationStateIndex
&& transition1.Group ==
transition2.Group)
{
if (transition1.IsEpsilon && transition2.IsEpsilon)
{
....
}
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. Использование схожих имён иногда вполне оправданно, но стоит помнить, что в таком случае возрастает вероятность случайно ошибиться где-нибудь с именем.
Так и произошло при проверке чисел на бесконечность (double.IsInfinity). Из-за ошибки 2 раза проверили значение одной и той же переменной — transition1.Weight.Value. Проверяемым значением во втором подвыражении должна была стать переменная transition2.Weight.Value.
Ещё один схожий подозрительный код.
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
При формировании значения переменной bf дважды используется элемент перечисления BindingFlags.Public. Либо этот код содержит лишнюю операцию выставления флага, либо вместо второго использования BindingFlags.Public должно быть другое значение перечисления.
Кстати, в исходниках этот код записан в одну строку. Мне кажется, что если он отформатирован в табличном стиле (как здесь), проблему обнаружить проще.
Идём дальше. Привожу тело метода целиком и вновь предлагаю вам найти ошибку (а может – ошибки) самостоятельно.
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. Visualizers.Windows FactorManagerView.cs 350
Кстати, встретился метод с безусловным оператором 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 может измениться. Однако между проверками resultIndex != null и resultIndex == null значение уже поменяться не может. Следовательно, результатом выражения 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. Следовательно, данная проверка ни от чего не защищает, и ожидаемое исключение сгенерировано не будет.
Именно об этом и предупреждает анализатор: V3022 Expression 'genres.Length < 1 && genres.Length > 3' is always false. Probably the '||' operator should be used here. 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, это сняло бы возможные вопросы.
В других местах метод Poisson.Sample, аргументом которого и выступает подозрительная переменная expectedRepeatOfTopicInDoc, используется, например, так, как описано ниже.
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.Equals(defaultValue), если value == null. Так как это выражение – правый операнд оператора ||, для его вычисления левый операнд должен иметь значение false, а для этого достаточно, чтобы хотя бы одна из переменных defaultValue value не была равна null. В итоге, если defaultValue != null, а value == null:
- defaultValue == null -> false;
- defaultValue == null && value == null -> false; (проверка value не произошла)
- value.Equals(defaultValue) -> NullReferenceException, так как value – null.
Посмотрим ещё на схожий случай:
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
Обратите внимание, что сначала происходит обращение к свойству quantiles.Length, а затем quantiles проверяется на равенство null. В итоге, если quantiles == null, метод выбросит исключение, только немного не то, и немного не там, где это было необходимо. Видимо, перепутали строки местами.
Если вы успешно справились с обнаружением приведённых раньше ошибок самостоятельно, предлагаю заварить себе чашечку кофе и попробовать повторить подвиг, найдя ошибку в приведённом ниже методе. Чтобы было чуточку интереснее, код метода привожу целиком.
Ладно, ладно, это была шутка (или вам всё же удалось?!). Давайте немного упростим задачу:
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
И действительно, в последней else-ветви два раза подряд присваивается значение переменной lowerBound. Видимо (и судя по коду выше), в одном из присвоений должна участвовать переменная upperBound.
Следуем дальше.
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
Обратите внимание, что счётчик внутреннего цикла – r – не используется в теле этого цикла. Из-за этого выходит, что на протяжении всех итераций внутреннего цикла выполняются одни и те же операции над одними и теми же элементами – ведь в индексах также используется счётчик внешнего цикла (c), а не внутреннего (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 подобных мест.
Кстати, ещё встретилось 785 предупреждений V3024. Предупреждение V3024 выдаётся при сравнении вещественных чисел с использованием операторов '!=' или '=='. Не буду здесь останавливаться на том, почему такие сравнения не всегда корректны – подробнее про это написано в документации, там же есть ссылка на StackOverflow (это она же).
Учитывая, что часто встречались формулы и вычисления, эти предупреждения также могут быть важны, хоть и вынесены на 3 уровень (так как актуальны далеко не во всех проектах).
Если же есть уверенность в том, что эти предупреждения неактуальны – можно убрать их практически одним щелчком, сократив общее количество срабатываний анализатора.
Заключение
Как-то так получилось, что я уже достаточно давно не писал статей о проверке проектов, и снова прикоснуться к этому процессу было достаточно приятно. Надеюсь, что и вы вынесли для себя из этой статьи что-то новое полезное, или хотя бы с интересом её прочитали.
Разработчикам желаю скорейшего исправления проблемных мест и напоминаю, что делать ошибки – это нормально, все же мы люди. Для того и нужны дополнительные инструменты вроде статических анализаторов, чтобы находить то, что пропустил человек, верно? Так или иначе – удачи с проектом, и спасибо за труд!
И помните, что максимальная польза от статического анализатора достигается при его регулярном использовании.
Всех благ!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. What Errors Lurk in Infer.NET Code?
Автор: foto_shooter
