09.09.2022 13:04
AndreyMoskalew 0 1800 Источник

В 2018 году Microsoft разработали ML.NET – фреймворк машинного обучения для .NET разработчиков. За прошедшее время эта библиотека претерпела существенные изменения и обзавелась новыми функциями для выявления закономерностей в данных. Посмотрим, как это отразилось на качестве её исходного кода.

Введение

Перед началом разбора ошибок предлагаю познакомиться с самим фреймворком. ML.NET — разработанная компанией Microsoft бесплатная Open Source библиотека со средствами машинного обучения для языков программирования C# и F#. Она позволяет создавать функционал для получения автоматических прогнозов на основе закономерностей в данных, доступных вашему приложению.

С помощью ML.NET можно обучить модель машинного обучения, указав соответствующий алгоритм, или импортировать предварительно обученные модели TensorFlow и ONNX.

Обученную модель можно добавить в приложение и использовать её для получения прогнозов.

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

Познакомившись с проектом, можно перейти к основной теме – разбору найденных ошибок. Но сначала хочу отдать должное разработчикам Microsoft — проект выполнен весьма качественно, найти что-то страшное и интересное (в рамках тематики этой статьи) было непросто. Тем не менее есть несколько серьёзных моментов, заслуживающих внимания.

Проверка проводилась на релизе ML.NET 1.7.1. Исходный код данной версии проекта доступен на GitHub.

Итак, приступаем к разбору!

Разбор ошибок в ML.NET

Объект использовался после проверки на null

Issue 1

private EstimatorChain(...., IEstimator<ITransformer>[] estimators, ....)
{
  ....
  _estimators = estimators ?? new IEstimator<ITransformer>[0]; // <=
  ....
  LastEstimator = estimators.LastOrDefault() as // <=
                  IEstimator<TLastTransformer>;

  _needCacheAfter = needCacheAfter ?? new bool[0];
  ....
}

Предупреждение PVS-Studio: V3125. The 'estimators' object was used after it was verified against null. EstimatorChain.cs 37, 39

Рассмотрим первое предупреждение. В данном случае PVS-Studio обнаружил потенциальную ошибку, которая может привести к разыменованию нулевой ссылки. Обратите внимание на код. С помощью оператора null-объединения выполняется проверка входного параметра estimators на наличие значения, после чего результат работы оператора записывается в поле _estimators.

Строчкой ниже разработчик снова обращается к параметру estimators, вызывая метод LastOrDefault. Т. к. estimators может иметь значение null, при обращении к этому параметру есть риск получить исключение NullReferenceException.

Решением данной проблемы является использование поля _estimators для вызова метода LastOrDefault вместо estimators:

LastEstimator = _estimators.LastOrDefault() as IEstimator<TLastTransformer>;

Имена параметра метода и поля класса очень похожи. Можно предположить, что разработчик их просто перепутал. Подобная мелочь запросто ускользнёт от взора разработчика при написании кода. Статический анализатор обращает наше внимание на такие моменты, тем самым избавляя от необходимости искать и исправлять эти ошибки в будущем.

Два оператора 'if' с одинаковыми условными выражениями

Issue 2

private protected override void CheckOptions(IChannel ch)
{
  ....
  if (FastTreeTrainerOptions.NumberOfLeaves > 2 &&
      FastTreeTrainerOptions.HistogramPoolSize >
      FastTreeTrainerOptions.NumberOfLeaves - 1)
  {
    throw ch.Except("Histogram pool size (ps) must be at least 2."); 
  }

  if (FastTreeTrainerOptions.NumberOfLeaves > 2 &&
      FastTreeTrainerOptions.HistogramPoolSize >
      FastTreeTrainerOptions.NumberOfLeaves - 1)
  {
    throw ch.Except("Histogram pool size (ps) must be at most numLeaves - 1."); 
  }
  ....
}

Предупреждение 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. BoostingFastTree.cs 44, 47

Анализатор обнаружил два похожих оператора if с идентичными условными выражениями. В блоке обоих выражений вызывается исключение, тем самым прерывая выполнение программы. Возможно, это выглядит лишь как ненужное нагромождение кода, однако я предлагаю взглянуть на проблему с другой стороны. Посмотрите на сообщения об ошибках – они не совпадают. Логично предположить, что разработчик собирался реализовать две различных проверки. Он скопировал одну из проверок, заменил сообщение, но забыл исправить само условие. Таким образом, одна из возможных ошибок осталась необработанной и всё также представляет угрозу для корректной работы программы.

Переменная использовалась после того, как она была назначена с помощью null-условного оператора

Issue 3

private static int CheckKeyLabelColumnCore<T>(....) where T : ....
{
  ....
  for (int i = 1; i < models.Length; i++)
  {
    ....
    var mdType = labelCol.Annotations
                         .Schema
                         .GetColumnOrNull(....) // <=
                         ?.Type;

    if (!mdType.Equals(keyValuesType))
      throw env.Except(....);
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3105 The 'mdType' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. PipelineEnsemble.cs 670

В этот раз анализатор PVS-Studio предупреждает о возможном возникновении NullReferenceException при разыменовании переменной, которая может иметь значение null. Анализатор сделал такой вывод из-за отсутствия соответствующей проверки между присваиванием переменной значения, потенциально равного null, и её непосредственным использованием. Обратите внимание на код. Переменной mdType присваивается значение посредством цепочки вызовов. Одним из её звеньев является метод GetColumnOrNull, который при определённом условии возвращает значение null:

public Column? GetColumnOrNull(string name)
{
  if (string.IsNullOrEmpty(name))
    throw new ArgumentNullException(nameof(name));
  if (_nameMap.TryGetValue(name, out int col))
    return _columns[col];
  return null;
}

Сразу после присваивания значения в условии выполняется обращение к переменной mdType для вызова метода Equals. Такой код может привести к возникновению исключения. Скорее всего, разработчик просто забыл обработать этот случай соответствующей проверкой.

Неправильный порядок аргументов, передаваемых методу

Issue 4

private IProgressChannel StartProgressChannel(int level)
{
  var newId = Interlocked.Increment(ref _maxSubId);
  return new SubChannel(this, level, newId);
}

Предупреждение PVS-Studio: V3066. Possible incorrect order of arguments passed to method. ProgressReporter.cs 148

Анализатор сообщает о возможном неправильном порядке аргументов, передаваемых методу. Действительно, метод StartProgressChannel возвращает экземпляр класса SubChannel, в конструктор которого были переданы параметры в неправильном порядке (id и level перепутаны местами). В этом легко убедиться, если посмотреть на реализацию конструктора класса SubChannel:

public SubChannel(ProgressChannel root, int id, int level)
{
  ....
  id = id;
  level = level;
  ....
}

Казалось бы, простая опечатка! И всё же она может стать источником серьёзных проблем. Обратите внимание на реализацию конструктора класса SubChannel. Здесь полям _id и _level присваиваются неверные данные. Эти поля, в свою очередь, могут быть использованы в остальном функционале класса, тем самым нарушая его работу.

К слову, подобные ошибки – вовсе не редкость и частенько встречаются в крупных проектах. Как вы поняли, ML.NET — не исключение. Вот ещё один пример:

Issue 5

internal sealed class LdaSingleBox : IDisposable
{
  ....
  public void AllocateModelMemory(int numTopic, int numVocab, ....)
  {
    ....
    LdaInterface.AllocateModelMemory(...., numVocab, numTopic, ....); // <=
  }
  ....
}

Предупреждение PVS-Studio: V3066. Possible incorrect order of arguments passed to 'AllocateModelMemory' method: 'numVocab' and 'numTopic'. LdaSingleBox.cs 142

Здесь при вызове метода LdaInterface.AllocateModelMemory были перепутаны местами входные параметры: numTopic и numVocab. В этом нетрудно убедиться, взглянув на их порядок в сигнатуре метода:

internal static class LdaInterface
{
  ....
  internal static extern void AllocateModelMemory(....,
                                                  int numTopic, // <=
                                                  int numVocab, // <=
                                                  ....);
  ....
}

Объект использовался после проверки на null

Issue 6

IPredictor IModelCombiner.CombineModels(....)
{
  ....
  foreach (var model in models)
  {
    ....
    var calibrated = predictor as IWeaklyTypedCalibratedModelParameters;
    ....
    if (calibrated != null)
      _host.Check(....);

    predictor = calibrated.WeaklyTypedSubModel; // <=
    paramA = -((PlattCalibrator)calibrated.WeaklyTypedCalibrator).Slope; // <=
    ....
    foreach (var t in tree.TrainedEnsemble.Trees)
    {
      ....
      if (modelCount == 1)
      {
        binaryClassifier = calibrated != null;
        ....
      }
      else
      {
        _host.Check((calibrated != null) == binaryClassifier, ....);
        ....
      }
    }
    ....
  }
}

Предупреждение PVS-Studio: V3125. The 'calibrated' object was used after it was verified against null. TreeEnsembleCombiner.cs 54, 58, 59

Представляю вам ещё один способ получения исключения типа NullReferenceException. Обратите внимание на переменную calibrated. Она инициализируется с помощью оператора as, который при неудачном приведении к типу возвращает null. Далее эта переменная используется без какой-либо проверки:

....
predictor = calibrated.WeaklyTypedSubModel; // <=
paramA = -((PlattCalibrator)calibrated.WeaklyTypedCalibrator).Slope;
....

Также хочу отметить количество проверок calibrated на null: в разных частях метода эта переменная проверяется трижды. Даже без подробного разбора кода можно сделать уверенный вывод: обсуждаемая проблема реальна и заслуживает отдельного внимания со стороны разработчика.

Замечу, что ошибки такого типа наиболее распространены в этом проекте, а потому предлагаю рассмотреть ещё парочку интересных примеров:

Issue 7

private void LoadSampledLabels(....)
{
  ....
  Contracts.Check(_instanceWeights == null || ....);
  if (....)
  {
    for (....)
    {
      ....
      weights[k] = (float)_instanceWeights[j];
    }
  }
  ....
}

Предупреждение PVS-Studio: V3125. The '_instanceWeights' object was used after it was verified against null. InternalQuantileRegressionTree.cs 81, 74

В данном случае проверка поля _instanceWeights на null реализована с помощью метода Contracts.Check:

public static void Check(bool f, string msg)
{
  if (!f)
    throw Except(msg);
}

Если _instanceWeights равен null, в методе Contracts.Check будет выполнена обработка ошибки с помощью соответствующего исключения. Скорее всего, именно так и задумывал автор, но в реальности получилось иначе. Обратите внимание на реализацию метода Contracts.Check. В случае, когда выполняется условие _instanceWeights == null, в метод Contracts.Check будет передано значение true в качестве параметра f и преобразовано в false. В результате проверка на null отработает некорректно, код продолжит выполняться, а при следующем обращении к полю _instanceWeights будет вызвано исключение NullReferenceException

Issue 8

private static List<....> Train(....)
{
  ....
  for (int i = 0; i < columns.Length; i++)
  {
    ....
    var srcColType = inputSchema[srcCol].Type as VectorDataViewType;
    if (srcColType == null || ....) // <=
      throw env.ExceptSchemaMismatch(...., srcColType.ToString()); // <=
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3125. The 'srcColType' object was used after it was verified against null. LdaTransform.cs 842, 843

Данный случай показался мне интересным, ведь, на первый взгляд здесь всё в порядке:

  • выполняется приведение типов с помощью оператора as, а затем результат записывается в переменную srcColType;

  • переменная srcColType проверяется на null, и, если проверка проходит успешно, вызывается исключение с соответствующим сообщением об ошибке.

И всё было бы отлично, только вот в качестве последнего параметра в исключении используется результат метода srcColType.ToString(), вызываемого у потенциально пустой переменной:

....
throw env.ExceptSchemaMismatch(...., srcColType.ToString());
....

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

Выражение всегда ложно

Issue 9

public static IntArray New(...., IntArrayType type, IntArrayBits bitsPerItem)
{
  ....
  Contracts.CheckParam(type == IntArrayType.Current ||
                       type == IntArrayType.Repeat ||
                       type == IntArrayType.Segmented, nameof(type));

  if (type == IntArrayType.Dense || bitsPerItem == IntArrayBits.Bits0)
  {
    ....
  }
  else if (type == IntArrayType.Sparse)
    return new DeltaSparseIntArray(length, bitsPerItem);
  ....
  return null;
}

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

  • V3022. Expression 'type == IntArrayType.Sparse' is always false. IntArray.cs 145

  • V3063. A part of conditional expression is always false if it is evaluated: type == IntArrayType.Dense. IntArray.cs 129

В данном случае было получено сразу два предупреждения, причём оба получены по одной и той же причине. Случай интересный, поэтому перейдём к делу. В приведённых условиях ожидается, что type может иметь значения IntArrayType.Sparse и IntArrayType.Dense, но наш анализатор с этим совершенно не согласен. Прав ли он? Предлагаю это выяснить. Обратите внимание на следующий код:

Contracts.CheckParam(type == IntArrayType.Current || 
                     type == IntArrayType.Repeat || 
                     type == IntArrayType.Segmented, nameof(type));

Здесь метод Contracts.CheckParam используется для проверки входного параметра type на равенство одному из следующих значений: IntArrayType.Current, IntArrayType.Repeat или IntArrayType.Segmented.

Теперь взглянем на реализацию метода Contracts.CheckParam:

public static void CheckParam(bool f, string paramName)
{
  if (!f)
    throw ExceptParam(paramName);
}

Как видно, в случае если type не равен ни одному из приведенных выше значений, вызывается исключение.

В итоге мы имеем два набора значений входного параметра type:

  • ожидаемые значения (IntArrayType.Sparse, IntArrayType.Dense);

  • допустимые значения (IntArrayType.Current, IntArrayType.Repeat, IntArrayType.Segmented).

Очевидно, что они совершенно не совпадают, а в коде, возможно, допущена серьёзная ошибка. Дело раскрыто!

Можно предположить, что при реализации приведённой выше проверки автор кода вовсе не ставил перед собой задачу перечислить все допустимые значения. Совсем наоборот, он хотел отфильтровать значения, недопустимые в данном методе. Исходя из этого, правильная реализация этой проверки могла бы выглядеть так:

Contracts.CheckParam(type != IntArrayType.Current || 
                     type != IntArrayType.Repeat || 
                     type != IntArrayType.Segmented, nameof(type));

Заключение

Серьёзных ошибок в проекте было обнаружено немного. А те, что были найдены, скорее допущены по невнимательности, нежели из-за недостатков в навыках разработчиков. Это показывает, что компания Microsoft знает свое дело, но также очередной раз доказывает, что профессионалам тоже свойственно ошибаться. Так почему бы не использовать статический анализатор в своей работе, тем самым сведя количество ошибок к минимуму и сделав свои проекты ещё качественнее и надёжнее?

Если вы согласны со мной, можете попробовать PVS-Studio в течение бесплатного периода, скачав пробную версию здесь.

Для наилучшего пользовательского опыта рекомендую уделить немного времени и ознакомиться с основами использования анализатора, прочитав интересующие вас разделы в документации.

Напоследок хочу пожелать компании Microsoft успехов в работе над фреймворком машинного обучения ML.NET, я с интересом наблюдаю за его развитием!

На этом статья завершается, надеюсь она показалась вам интересной :).

Чистого кода и успешных проектов! До встречи в следующих статьях!

Комментарии (0)