These days, many people take an interest in stocks, bonds, and exchanges, with some even managing their own investment portfolios (and we're not talking CVs here). Numerous technologies and tools exist to automate trading. Imagine potential losses that errors in such software source code could cause. That's why we'll review potential bugs in the popular Lean trading engine.
Introduction
What is Lean? QuantConnect Lean is an open-source algorithmic trading engine designed for easy strategy research, back testing, and live trading. It runs on Windows, Linux, and macOS, integrating with common data providers and brokerages for rapid deployment of algorithmic trading strategies.
This is a high-stakes project where any error can lead to significant financial losses. Therefore, developing such tools demands rigorous testing and code analysis means. The PVS-Studio static analyzer can serve as a valuable aid in this development.
We've previously written about errors in this project ("Talking About Errors in the QuantConnect Lean Code") quite some time ago. We regularly add new diagnostic rules to the analyzer and have enhanced existing ones. Lean has also evolved continuously, so it's interesting to see what's changed in the meantime.
Checking the project
Warnings issued by new diagnostic rules
Let's start with several warnings from new diagnostic rules that didn't exist at the time of our previous review.
Snippet 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);
}
The PVS-Studio warning: V3192 The 'Legs' property is used in the 'GetHashCode' method but is missing from the 'Equals' method. OptionStrategyDefinitionMatch.cs 176
The analyzer performed an interprocedural analysis of the Equals method, which calls an overridden Equals method, and detected that the Legs property wasn't used despite being referenced in GetHashCode.
Look at the Equals method carefully:
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;
}
Note that it iterates through other.Legs. For each element in this collection, we attempt to locate it in the positions dictionary—but this dictionary derives from other.Legs. Essentially, the code checks whether elements exist in the same collection they originate from.
The fix is straightforward: replace other.Legs with Legs at the marked location.
Snippet 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++; // <=
....
}
}
The PVS-Studio warning: 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
The analyzer reports that the lineNumber variable passed to a LINQ method changes after it is passed. Indeed, lineNumber is initialized and passed to the LINQ method, but execution is deferred. Later, during iteration over the csv collection, with each reference to this collection the lineNumber value changes. Then it changes again inside the foreach loop.
In this specific case, the printed lineNumber will be twice the expected value due to dual incrementation—in the LINQ method and in foreach.
Snippet 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}");
}
}
The PVS-Studio warning: V3220 The result of the 'Where' LINQ method with deferred execution is never used The method will not be executed. AddOptionWithOnMarketOpenOnlyFilterRegressionAlgorithm.cs 58
The message reports that the LINQ result isn't used—the Where method employs deferred execution and will never be called.
This suggests the missing logic: the developer likely forgot to implement intended functionality here.
Snippet 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; // <=
}
....
}
});
The PVS-Studio warning: V3190 Concurrent modification of the 'start' variable may lead to errors. AlgoSeekFuturesConverter.cs 115
The analyzer issued this warning because concurrent modification of the start variable inside the parallel ForEach method risks a data race.
When multiple threads modify the same variable simultaneously, results become unpredictable. This may happen when one thread is reading a variable while another one is changing it. One can mitigate this by implementing locks.
Insidious NRE
Snippet 5
if ( !ReferenceEquals(result, OptimizationResult.Initial)
&& string.IsNullOrEmpty(result?.JsonBacktestResult))
{
_runningParameterSet.Remove(result.ParameterSet);
return;
}
....
The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'result' object. EulerSearchOptimizationStrategy.cs 73
The analyzer detected a possible NullReferenceException when accessing the result parameter. The condition is flawed—the then block executes precisely when result is null (the opposite of what developers intended). Here is the code fix:
if ( !ReferenceEquals(result, OptimizationResult.Initial)
&& !string.IsNullOrEmpty(result?.JsonBacktestResult))
{
_runningParameterSet.Remove(result.ParameterSet); // <=
return;
}
....
Snippet 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);
}
}
The PVS-Studio warning: V3095 The 'security' object was used before it was verified against null. Check lines: 966, 968. AlgorithmManager.cs 966
The analyzer detected a scenario that involves using the security parameter before checking it for null via ?, risking to get the NullReferenceException.
If the parameter might be null, the authors should consider using ? for all accesses.
Snippet 7
public void ProcessDelistings(Delistings delistings)
{
foreach (var delisting in delistings?.Values
.OrderBy(x => !x.Symbol
.SecurityType
.IsOption()))
{
....
}
}
I suggest you try on the static analyzer role: see if you can find the NullReferenceException above and consider fixes.
The right answer is somewhere around here
The PVS-Studio warning: V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. BacktestingBrokerage.cs 527
The issue reappears with the ? operator—foreach doesn't handle null collections. If delistings could be null, either check it before foreach or rewrite as follows:
public void ProcessDelistings(Delistings delistings)
{
foreach (var delisting in delistings?.Values
.OrderBy(x => !x.Symbol
.SecurityType
.IsOption())
?? Enumerable.Empty<Delistings>())
{
....
}
}
In this case, even if delistings is null, no exception will follow, and the loop will iterate over an empty collection.
Copy-paste errors
Let's look at the errors likely arising from copied code where critical details went unmodified. These bugs severely impact functionality and evade manual detection due to visually similar identifiers.
Static analysis comes to aid here. As an example, the PVS-Studio tool efficiently spots such errors—let's review examples.
Snippet 8
public static SortedDictionary<....> Filter<....>(....)
{
....
var breakAfterFailure =
comparison == BinaryComparison.LessThanOrEqual ||
comparison == BinaryComparison.LessThanOrEqual;
....
}
The PVS-Studio warning: V3001 There are identical sub-expressions 'comparison == BinaryComparison.LessThanOrEqual' to the left and to the right of the '||' operator.BinaryComparisonExtensions.cs 89
The analyzer reports identical expressions on both sides of||. While unclear whether the duplication is erroneous or redundant, this warning is so time-saving—manual detection of such issues across hundreds of lines is arduous.
Identical code and analyzer warnings appear elsewhere in the project. Copy paste strikes again!
Snippet 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;
}
The PVS-Studio warning: V3001 There are identical sub-expressions 'security.Type != SecurityType.Index' to the left and to the right of the '&&' operator. ExanteBrokerageModel.cs 80
Another oversight—the analyzer highlights two identical subexpressions. However, it's difficult to say whether this is a significant error. The second subexpression may be redundant but correct. However, authors might have intended to check something else with it.
Snippet 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);
}
}
The PVS-Studio warning: V3008 The 'SessionId' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 177, 166. BacktestResultPacket.cs 177
Double assignment of the same value rarely results in an error but may signal misplaced variables.
Technical debt fights back
We all enjoy reuniting with old friends—unless that "friend" is a resurrected code bug.
As mentioned earlier, we previously analyzed this project's potential errors. We've covered new warnings, now let's revisit older ones that are still relevant today.
Snippet 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;
}
....
}
The PVS-Studio warning: V3022 Expression '_random.Next(0, 1) == 0' is always true. RandomValueGenerator.cs 101
This code intends to randomly choose between two values. But the second value passed to Next lies outside the return range. Here, the method only returns 0.
Here's a way to fix it:
return _random.Next(0, 2) == 0
? previousDayOfWeek : nextDayOfWeek;
Now the Next method will return 0 or 1.
Snippet 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)}");
}
....
}
The PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'buyingPowerModel'. BasicTemplateFuturesAlgorithm.cs 107
The PVS-Studio warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'buyingPowerModel', 'futureMarginModel'. BasicTemplateFuturesAlgorithm.cs 105
The analyzer issued two warnings here due to a bizarre condition: a variable is checked for null and is dereferenced right after that.
Most likely, the as operator result (futureMarginModel variable) should be checked for null instead. A corrected version may be as follows:
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)}");
}
....
}
Additionally, if futureMarginModel is null, buyingPowerModel might also be null, resulting in the NullReferenceException during method invocation. I suggest the following refinement:
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)}");
}
....
}
Snippet 13
public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
private readonly Minimum _medianMin;
private readonly Maximum _medianMax;
....
public override bool IsReady => _medianMax.IsReady && _medianMax.IsReady;
....
}
The PVS-Studio warning: V3001 There are identical sub-expressions '_medianMax.IsReady' to the left and to the right of the '&&' operator. FisherTransform.cs 72
Identical subexpressions wrap the && operator. The second one most likely should contain the medianMin field. A corrected version might look as follows:
public class FisherTransform : BarIndicator, IIndicatorWarmUpPeriodProvider
{
private readonly Minimum _medianMin;
private readonly Maximum _medianMax;
....
public override bool IsReady => _medianMin.IsReady && _medianMax.IsReady;
....
}
Snippet 14
if (twoMonthsPriorToContractMonth.Month == 2)
{
lastBusinessDay = FuturesExpiryUtilityFunctions
.NthLastBusinessDay(twoMonthsPriorToContractMonth,
1,
holidays);
}
else
{
lastBusinessDay = FuturesExpiryUtilityFunctions
.NthLastBusinessDay(twoMonthsPriorToContractMonth,
1,
holidays);
}
The PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. FuturesExpiryFunctions.cs 2380
The analyzer found a case with identical then and else branches—a classic copy-paste error where post-copy logic adjustments were overlooked.
To sum up
Considering its scale, the project contains relatively few bugs. Most appear to stem from oversight rather than complexity. Notably, bugs from our previous article posted 5 years ago remain unpatched today.
However, one should keep in mind, that error density typically scales with the codebase growth. The bugs from this and the previous article could have been caught and fixed at the writing code stage using static analysis.
Request a trial license and try the latest PVS-Studio version here.
Take care of your code—and of yourself!
Top comments (0)