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

Введение

Что же такое Lean? QuantConnect Lean — это алгоритмический торговый движок с открытым исходным кодом, созданный для легкого исследования стратегий, бэктестинга и живой торговли. Совместим с Windows, Linux и macOS. Интегрируется с распространёнными поставщиками данных и брокерскими компаниями для быстрого развёртывания алгоритмических торговых стратегий.

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

Когда-то давно мы уже писали об ошибках в этом проекте ("Об ошибках в коде QuantConnect Lean"), с тех пор много воды утекло. Мы регулярно добавляем новые диагностики в анализатор, а также улучшаем уже имеющиеся. Lean также постоянно развивается, поэтому интересно посмотреть, что изменилось за это время.

Проверяем проект

Предупреждения новых диагностик

Для начала рассмотрим некоторые предупреждения новых диагностик, которых не существовало на момент предыдущей статьи.

Фрагмент кода 1

public override int GetHashCode()
{
  unchecked
  {
    var hashCode = Definition.GetHashCode();
    var arr = new int[Legs.Count];
    for (int i = 0; i < Legs.Count; i++)
    {
      arr[i] = Legs[i].GetHashCode();
    }

    Array.Sort(arr);

    for (int i = 0; i < arr.Length; i++)
    {
      hashCode = (hashCode * 397) ^ arr[i];
    }

    return hashCode;
  }
}

public override bool Equals(object obj)
{
    ....

    return Equals((OptionStrategyDefinitionMatch) obj);
}

Предупреждение PVS-Studio: V3192 The 'Legs' property is used in the 'GetHashCode' method but is missing from the 'Equals' method. OptionStrategyDefinitionMatch.cs 176

Анализатор межпроцедурно проанализировал метод Equals, который вызывает переопределённый метод Equals, и обнаружил отсутствие использования свойства Legs, хотя это свойство используется в методе GetHashCode.

Рассмотрим внимательно метод Equals:

public bool Equals(OptionStrategyDefinitionMatch other)
{
  ....

  var positions = other.Legs
                        .ToDictionary(leg => leg.Position, 
                                       leg => leg.Multiplier);
  foreach (var leg in other.Legs)                                   // <=
  {
    int multiplier;
    if (!positions.TryGetValue(leg.Position, out multiplier))
    {
      return false;
    }

    if (leg.Multiplier != multiplier)
    {
      return false;
    }
  }

  return true;
}

Заметим, что в нём есть итерация по other.Legs. Каждый элемент этой коллекции мы пытаемся найти в словаре positions, но этот словарь получается из other.Legs. Таким образом, проверяется наличие элементов коллекции в этой же коллекции.

Исправить код можно, заменив в отмеченном месте other.Legs на Legs.

Фрагмент кода 2

public void FutureMarginModel_MarginEntriesValid(string market)
{
  ....
  var lineNumber = 0;
  var errorMessageTemplate = $"Error encountered in file 
                               {marginFile.Name} on line ";
  var csv = File.ReadLines(marginFile.FullName)
                .Where(x =>    !x.StartsWithInvariant("#") 
                            && !string.IsNullOrWhiteSpace(x))
                .Skip(1)
                .Select(x =>
  {
    lineNumber++;                                                  // <=

    ....
  });

  lineNumber = 0;                                                  // <=
  foreach (var line in csv)
  {
    lineNumber++;                                                  // <=
    ....
  }
}

Предупреждение PVS-Studio: V3219 The 'lineNumber' variable was changed after it was captured in a LINQ method with deferred execution. The original value will not be used when the method is executed. FutureMarginBuyingPowerModelTests.cs 720

Анализатор сообщает нам, что переданная в LINQ метод переменная lineNumber изменяется после непосредственной передачи. Действительно, сначала инициализируется переменная lineNumber, передаётся в LINQ метод, но он не сразу выполняется. Затем происходит итерация по коллекции csv, и при каждом обращении к этой коллекции значение lineNumber меняется, а затем меняется еще внутри цикла foreach.

Конкретно в этом случае при выводе lineNumber значение будет в 2 раза больше предполагаемого, потому что переменная инкрементируется в LINQ методе, а затем ещё раз в foreach.

Фрагмент кода 3

 public override void OnSecuritiesChanged(SecurityChanges changes)
{
  if (changes.AddedSecurities.All(s => s.Type != SecurityType.Option))
  {
    return;
  }

  var changeOptions = changes.AddedSecurities
                             .Concat(changes.RemovedSecurities
                             .Where(s => s.Type == SecurityType.Option);

  if (Time != Time.Date)
  {
    throw new RegressionTestException($"Expected options filter to be run 
                                        only at midnight. Actual was {Time}");
  }
}

Предупреждение PVS-Studio: V3220 The result of the 'Where' LINQ method with deferred execution is never used. The method will not be executed. AddOptionWithOnMarketOpenOnlyFilterRegressionAlgorithm.cs 58

Данное срабатывание сообщает нам, что результат LINQ метода не используется. Метод Where с отложенным выполнением, а значит он не будет вызван.

Возможно, здесь должна быть какая-то логика, но её забыли реализовать.

Фрагмент кода 4

var start = DateTime.MinValue;

var symbolMultipliers = LoadSymbolMultipliers();

Parallel.ForEach(files, file =>
{
  try
  {
    Log.Trace("Remote File :" + file);

    var csvFile = Path.Combine(_source.FullName,
                               Path.GetFileNameWithoutExtension(file.Name));

    Log.Trace("Source File :" + csvFile);

    if (!File.Exists(csvFile))
    {
      var csvFileInfo = new FileInfo(csvFile);
      Directory.CreateDirectory(csvFileInfo.DirectoryName);

      Log.Trace("AlgoSeekFuturesConverter.Convert(): Extracting " + file);

      Compression.Extract7ZipArchive(file.FullName, _source.FullName, -1);
      }

    // setting up local processors
    var processors = new Processors();

    var reader = new AlgoSeekFuturesReader(csvFile, 
                                           symbolMultipliers, 
                                           _symbolFilter);
    if (start == DateTime.MinValue)
    {
      start = DateTime.Now;                                     // <=
    }

    ....

  }
});

Предупреждение PVS-Studio: V3190 Concurrent modification of the 'start' variable may lead to errors. AlgoSeekFuturesConverter.cs 115

Анализатор выдал предупреждение, потому что параллельное изменение переменной может привести к гонке данных. В данном случае переменная start изменяется внутри параллельного метода ForEach.

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

Коварный NRE

Фрагмент кода 5

if (   !ReferenceEquals(result, OptimizationResult.Initial) 
    && string.IsNullOrEmpty(result?.JsonBacktestResult))
{
  _runningParameterSet.Remove(result.ParameterSet); 
  return;
}
....

Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'result' object. EulerSearchOptimizationStrategy.cs 73

Анализатор обнаружил возможный NullReferenceException при обращении к параметру result. Дело в том, что условие некорректное — блок then будет выполняться в том случае, когда result равен null (а разработчики как раз хотели обратного). Исправить код можно так:

if (   !ReferenceEquals(result, OptimizationResult.Initial) 
    && !string.IsNullOrEmpty(result?.JsonBacktestResult))
{
  _runningParameterSet.Remove(result.ParameterSet);      // <=
  return;
}
....

Фрагмент кода 6

private static void ApplySplitOrDividendToVolatilityModel(IAlgorithm algorithm,
                                                          Security security,
                                                          bool liveMode,
                                   DataNormalizationMode dataNormalizationMode)
{
  if (   security.Type == SecurityType.Equity                             // <=
      && (liveMode || dataNormalizationMode == DataNormalizationMode.Raw))
  {
    security?.VolatilityModel.WarmUp(algorithm.HistoryProvider,           // <=
                                     algorithm.SubscriptionManager, 
                                     security, algorithm.UtcTime,
                                     algorithm.TimeZone, 
                                     liveMode, 
                                     dataNormalizationMode);
  }
}

Предупреждение PVS-Studio: V3095 The 'security' object was used before it was verified against null. Check lines: 966, 968. AlgorithmManager.cs 966

Анализатор обнаружил ситуацию, когда параметр security используется, а потом проверяется на null с помощью оператора ?. Потенциально такой код может привести к NullReferenceException.

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

Фрагмент кода 7

public void ProcessDelistings(Delistings delistings)
{
  foreach (var delisting in delistings?.Values
                                       .OrderBy(x => !x.Symbol
                                                       .SecurityType
                                                       .IsOption()))
  {    
     ....   
  }
}

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

public void ProcessDelistings(Delistings delistings)
{
  foreach (var delisting in delistings?.Values
                                       .OrderBy(x => !x.Symbol
                                                       .SecurityType
                                                       .IsOption())
                            ?? Enumerable.Empty<Delistings>())
  {    
     ....   
  }
}

Copy-paste ошибки

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

На помощь разработчикам приходит статический анализ. Например, наш анализатор PVS-Studio отлично справляется с поиском таких ошибок. Рассмотрим же их.

Фрагмент кода 8

public static SortedDictionary<....> Filter<....>(....)
{
  ....
  var breakAfterFailure =
      comparison == BinaryComparison.LessThanOrEqual ||
      comparison == BinaryComparison.LessThanOrEqual;
  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'comparison == BinaryComparison.LessThanOrEqual' to the left and to the right of the '||' operator.BinaryComparisonExtensions.cs 89

Анализатор сообщает нам, что по обе стороны от оператора || находятся идентичные выражения. Это действительно так, но сложно сказать, является ли второе выражение лишним или ошибочным. Однако срабатывание полезное, ведь вручную найти такую ошибку среди сотен строк кода очень тяжело.

Также упомяну, что такой же код и срабатывание анализатора встречаются в другом файле этого же проекта. Вот такой вредный бывает copy-paste.

Фрагмент кода 9

public override bool CanSubmitOrder(Security security, 
                                    Order order, 
                                    out BrokerageMessageEvent message)
{
  ....

  if (security.Type != SecurityType.Forex &&
      security.Type != SecurityType.Equity &&
      security.Type != SecurityType.Index &&          // <=
      security.Type != SecurityType.Option &&
      security.Type != SecurityType.Future &&
      security.Type != SecurityType.Cfd &&
      security.Type != SecurityType.Crypto &&
      security.Type != SecurityType.Index)            // <=
  {
    message = new BrokerageMessageEvent(BrokerageMessageType.Warning, 
                                        "NotSupported",
    Messages.DefaultBrokerageModel
            .UnsupportedSecurityType(this, security));
        return false;
    }

    return true;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'security.Type != SecurityType.Index' to the left and to the right of the '&&' operator. ExanteBrokerageModel.cs 80

Очередная потенциальная ошибка из-за невнимательности. В данном случае статический анализатор указывает на два одинаковых подвыражения. Однако сложно сказать, существенная ли это ошибка. Может быть, что второе подвыражение просто лишнее, а не ошибочное, а может и наоборот — вместо повторения должно проверяться что-то другое.

Фрагмент кода 10

public BacktestResultPacket(BacktestNodePacket job, 
                            BacktestResult results, 
                            DateTime endDate, 
                            DateTime startDate, 
                            decimal progress = 1m) : this()
{
  try
  {
    Progress = Math.Round(progress, 3);
    SessionId = job.SessionId;                   // <=
    PeriodFinish = endDate;
    PeriodStart = startDate;
    CompileId = job.CompileId;       
    Channel = job.Channel;
    BacktestId = job.BacktestId;
    OptimizationId = job.OptimizationId;
    Results = results;
    Name = job.Name;
    UserId = job.UserId;
    ProjectId = job.ProjectId;
    SessionId = job.SessionId;                   // <=
    TradeableDates = job.TradeableDates;
  }
  catch (Exception err) {
    Log.Error(err);
 }
}

Предупреждение PVS-Studio: V3008 The 'SessionId' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 177, 166. BacktestResultPacket.cs 177

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

Технический долг наносит ответный удар

Наверняка каждому из вас знакомо приятное чувство от встречи со старым знакомым. Но если "знакомый" — это баг в коде, то вряд ли такую встречу можно назвать приятной.

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

Фрагмент кода 11

public virtual DateTime NextDate(DateTime minDateTime, 
                                 DateTime maxDateTime, 
                                 DayOfWeek? dayOfWeek)
{
    
  ....

  // both are valid dates, so chose one randomly
  if (IsWithinRange(nextDayOfWeek, minDateTime, maxDateTime) &&
        IsWithinRange(previousDayOfWeek, minDateTime, maxDateTime))
  {
    return _random.Next(0, 1) == 0
        ? previousDayOfWeek
        : nextDayOfWeek;
  }

  ....  
}

Предупреждение PVS-Studio: V3022 Expression '_random.Next(0, 1) == 0' is always true. RandomValueGenerator.cs 101

Смысл подобного кода в том, чтобы случайно выбирать из двух значений. Но второе значение, передаваемое в метод Next НЕ входит в диапазон возвращаемых значений. В данном случае метод может вернуть только 0.

Исправить это можно следующим образом:

return _random.Next(0, 2) == 0
        ? previousDayOfWeek : nextDayOfWeek;

Теперь метод Next будет возвращать 0 или 1.

Фрагмент кода 12

public override void OnEndOfAlgorithm()
{
  var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;
  var futureMarginModel = buyingPowerModel as FutureMarginModel;
  if (buyingPowerModel == null)
  {
    throw new Exception($"Invalid buying power model. " +
                        $"Found: {buyingPowerModel.GetType().Name}. " +
                        $"Expected: {nameof(FutureMarginModel)}");  
  }
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'buyingPowerModel'. BasicTemplateFuturesAlgorithm.cs 107

Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'buyingPowerModel', 'futureMarginModel'. BasicTemplateFuturesAlgorithm.cs 105

На этот фрагмент кода анализатор выдал аж два предупреждения. Дело тут в неправильном условии, поскольку происходит очень странная ситуация, когда переменная проверяется на null и сразу разыменовывается.

Вероятнее всего, на null должен проверяться результат оператора as — переменная futureMarginModel. Исправленный код может выглядеть так:

public override void OnEndOfAlgorithm()
{
  var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;
  var futureMarginModel = buyingPowerModel as FutureMarginModel;
  if (futureMarginModel == null)
  {
    throw new Exception($"Invalid buying power model. " +
                        $"Found: {buyingPowerModel.GetType().Name}. " +
                        $"Expected: {nameof(FutureMarginModel)}");  
  }
  ....
}

Также нужно учесть, что если futureMarginModel будет null, то buyingPowerModel тоже может быть null. В таком случае будет выброшено NullReferenceException при попытке вызвать метод. Поэтому предлагаю следующее исправление:

public override void OnEndOfAlgorithm()
{
  var buyingPowerModel = Securities[_contractSymbol].BuyingPowerModel;
  var futureMarginModel = buyingPowerModel as FutureMarginModel;
  if (futureMarginModel == null)
  {
    string foundType =    buyingPowerModel?.GetType().Name 
                       ?? "the type was not found because the variable is null";
    throw new Exception($"Invalid buying power model. " +
                        $"Found: {foundType}. " +
                        $"Expected: {nameof(FutureMarginModel)}");   
  }
  ....
}

Фрагмент кода 13

public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
  private readonly Minimum _medianMin;
  private readonly Maximum _medianMax;
  ....
  public override bool IsReady => _medianMax.IsReady && _medianMax.IsReady;
  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions '_medianMax.IsReady' to the left and to the right of the '&&' operator. FisherTransform.cs 72

По обе стороны логического оператора && стоят одинаковые подвыражения. Скорее всего, во втором выражении должно быть поле medianMin. Исправленный вариант может выглядеть следующим образом:

public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
  private readonly Minimum _medianMin;
  private readonly Maximum _medianMax;
  ....
  public override bool IsReady => _medianMin.IsReady && _medianMax.IsReady;
  ....
}

Фрагмент кода 14

if (twoMonthsPriorToContractMonth.Month == 2)
{
  lastBusinessDay = FuturesExpiryUtilityFunctions
                    .NthLastBusinessDay(twoMonthsPriorToContractMonth, 
                                        1, 
                                        holidays);
}
else
{
  lastBusinessDay = FuturesExpiryUtilityFunctions
                    .NthLastBusinessDay(twoMonthsPriorToContractMonth, 
                                        1, 
                                        holidays);
}

Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. FuturesExpiryFunctions.cs 2380

Анализатор нашёл ситуацию, когда then ветка эквивалентна else ветке. Скорее всего, эта ошибка вызвана тем, что разработчик скопировал часть кода, но забыл изменить логику.

Подводим итоги

Можно сказать, что проект для своего большого размера содержит не так уж и много ошибок. Также замечу, что их большая часть допущена скорее по невнимательности. Но те, что были описанны в предыдущей статье, так и не исправили спустя почти 5 лет.

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

Запросить пробный ключ и попробовать последнюю версию анализатора PVS-Studio можно на этой странице.

Берегите себя и свой код!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Georgii Tormozov. Stonks or not stonks. Checking Lean trading engine source code.