Время от времени мы возвращаемся к проектам, которые уже проверяли ранее с помощью PVS-Studio и писали про это статьи. Делать это интересно по двум причинам. Во-первых, чтобы понять, насколько лучше стал наш анализатор. Во-вторых, чтобы отследить, обратили ли авторы проекта внимание на нашу статью, а также на отчет об ошибках, который мы им обычно предоставляем. Конечно, ошибки могут быть исправлены и без нашего участия. Но всегда приятно, когда именно наши усилия помогают сделать какой-то проект лучше. Не стал исключением и Roslyn. Предыдущая статья о проверке этого проекта датируется 23 декабря 2015 года. Это довольно давно, учитывая путь, который за это время проделал в своём развитии наш анализатор. Дополнительный интерес лично для нас Roslyn представляет ещё и тем, что на нём базируется ядро C# анализатора PVS-Studio. Таким образом, мы очень заинтересованы в качестве кода этого проекта. Устроим ему повторную проверку и выясним, что же нового и интересного (но будем надеяться, что ничего существенного) там сможет найти PVS-Studio.
Roslyn (или .NET Compiler Platform) наверняка знаком многим из наших читателей. Если кратко, то это набор компиляторов с открытым исходным кодом и API для анализа кода для языков C# и Visual Basic .NET от Microsoft. Исходный код проекта доступен на GitHub.
Не буду приводить подробного описания этой платформы, а всем интересующимся рекомендую статью моего коллеги Сергея Васильева "Введение в Roslyn. Использование для разработки инструментов статического анализа". Из этой статьи можно узнать не только об особенностях архитектуры Roslyn, но и как именно мы используем эту платформу.
Как я уже упоминал ранее, с момента написания последней статьи моего коллеги Андрея Карпова о проверке Roslyn "Новогодний релиз PVS-Studio 6.00: проверяем Roslyn" прошло более трёх лет. За это время C# анализатор PVS-Studio обзавёлся многими новыми возможностями. Вообще, статья Андрея была неким «пробным шаром», ведь C# анализатор тогда только был добавлен в PVS-Studio. Несмотря на это, уже тогда в безусловно качественном проекте Roslyn удалось найти интересные ошибки. Что же изменилось в анализаторе для C# кода к настоящему времени, что потенциально позволит провести более глубокий анализ?
За прошедшее время развивались как ядро анализатора, так и инфраструктура. Была добавлена поддержка Visual Studio 2017 и Roslyn 2.0, а также осуществлена глубокая интеграция с MSBuild. Подробнее про наш подход к интеграции с MSBuild и про причины, которые побудили нас к его принятию, можно прочитать в статье моего коллеги Павла Еремеева "Поддержка Visual Studio 2017 и Roslyn 2.0 в PVS-Studio: иногда использовать готовые решения не так просто, как кажется на первый взгляд".
Сейчас мы активно работаем над переходом на Roslyn 3.0 по той же схеме, по которой изначально поддержали Visual Studio 2017, то есть через свой собственный toolset, идущий в дистрибутиве PVS-Studio c «заглушкой» в виде пустого файла MSBuild.exe. Несмотря на то, что это выглядит как «костыль» (API MSBuild не очень дружелюбен для переиспользования в third-patry проектах из-за невысокой портабельности библиотек), такой подход уже помог нам относительно безболезненно пережить несколько обновлений Roslyn в течение жизни Visual Studio 2017, и сейчас, хотя и с большим количеством накладок, пережить обновление до Visual Studio 2019, а также сохранять полную обратную совместимость и работоспособность на системах с более старыми версиями MSBuild.
Ядро анализатора также претерпело ряд улучшений. Одно из главных нововведений — полноценный межпроцедурный анализ с учётом входных и выходных значений методов, с расчётом, в зависимости от этих параметров, достижимости веток выполнения и точек возврата.
Уже близка к завершению задача отслеживания параметров внутри методов, с сохранением авто-аннотаций для того, что с этими параметрами там происходит (например, потенциально опасное разыменование). Это позволит для любой диагностики, использующей механизм data-flow, учитывать опасные ситуации, происходящие при передаче параметра в метод. Раньше при анализе таких опасных мест предупреждение не генерировалось, так как мы не могли знать всех возможных входных значений в такой метод. Сейчас же мы можем обнаружить опасность, так как во всех местах вызова этого метода эти входные параметры будут учтены.
Замечание: вы можете ознакомиться с основными механизмами анализатора, такими как data-flow и другими, из статьи "Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей".
Межпроцедурный анализ в PVS-Studio C# не ограничен ни входными параметрами, ни глубиной. Единственное ограничение — виртуальные методы в незакрытых для наследования классах и попадание в рекурсию (остановимся, когда увидели в стеке повторный вызов уже рассчитываемого метода). При этом сам рекурсивный метод в итоге будет посчитан с допущением, что возвращаемое значение его саморекурсии неизвестно.
Другим большим нововведением в C# анализаторе стал учёт возможности разыменования потенциально нулевого указателя. Раньше анализатор ругался на возможный null reference exception, если был уверен, что во всех ветках выполнения значение переменной будет принимать null. Конечно, он иногда ошибался, поэтому диагностика V3080 уже раньше называлась potential null reference.
Теперь анализатор запоминает, что переменная могла быть null в одной из веток выполнения (например, при определённом условии в if). Если он увидит доступ к такой переменной без проверки, то выдаст сообщение V3080, но на более низком уровне важности, чем если он видит null во всех ветках. В сочетании с улучшенным межпроцедурным анализом, такой механизм позволяет находить очень сложно обнаруживаемые ошибки. Пример — длинная цепочка вызовов методов, последний из которых вам не знаком, и который, например, при определённых обстоятельствах в catch возвращает null, но вы не защитились от этого, так как этого просто не знали. В данном случае анализатор ругается только тогда, когда точно видит назначение null. На наш взгляд, это качественно отличает наш подход от такого нововведения C# 8.0, как nullable reference тип, которое, по факту, сводится к установке проверок на null в каждом методе. Мы же предлагаем альтернативу — делать проверки только там, где null может действительно прийти, и наш анализатор теперь умеет искать такие ситуации.
Итак, не откладывая в долгий ящик, перейдём к «разбору полётов» — анализу результатов проверки Roslyn. Сначала рассмотрим ошибки, найденные благодаря описанным выше нововведениям. Вообще, для кода Roslyn в этот раз было выдано довольно много предупреждений. Я думаю, это связано с тем, что платформа очень активно развивается (объем кодовой базы на данный момент составляет около 2 770 000 строк кода без учета пустых), а анализ этого проекта мы давно не делали. Тем не менее, критичных ошибок не так много, а именно они представляют интерес для статьи. И да, в Roslyn довольно много тестов, которые я, как обычно, исключил из проверки.
Начну с ошибок V3080, с уровнем критичности Medium, в которых анализатор обнаружил возможный доступ по нулевой ссылке, но не во всех возможных случаях (ветках кода).
Possible null dereference — Medium
V3080 Possible null dereference. Consider inspecting 'current'. CSharpSyntaxTreeFactoryService.PositionalSyntaxReference.cs 70
private SyntaxNode GetNode(SyntaxNode root)
{
var current = root;
....
while (current.FullSpan.Contains(....)) // <=
{
....
var nodeOrToken = current.ChildThatContainsPosition(....);
....
current = nodeOrToken.AsNode(); // <=
}
....
}
public SyntaxNode AsNode()
{
if (_token != null)
{
return null;
}
return _nodeOrParent;
}Рассмотрим метод GetNode. Анализатор считает, что возможен доступ по нулевой ссылке в условии блока while. В теле блока while переменной current будет присвоено значение — результат выполнения метода AsNode. И это значение в ряде случаев будет null. Хороший пример межпроцедурного анализа в действии.
Теперь рассмотрим похожий случай, в котором межпроцедурный анализ был произведен через два вызова метода.
V3080 Possible null dereference. Consider inspecting 'directory'. CommonCommandLineParser.cs 911
private IEnumerable<CommandLineSourceFile>
ExpandFileNamePattern(string path, string baseDirectory, ....)
{
string directory = PathUtilities.GetDirectoryName(path);
....
var resolvedDirectoryPath = (directory.Length == 0) ? // <=
baseDirectory :
FileUtilities.ResolveRelativePath(directory, baseDirectory);
....
}
public static string GetDirectoryName(string path)
{
return GetDirectoryName(path, IsUnixLikePlatform);
}
internal static string GetDirectoryName(string path, bool isUnixLike)
{
if (path != null)
{
....
}
return null;
}Переменная directory в теле метода ExpandFileNamePattern получает значение из метода GetDirectoryName(string). Тот, в свою очередь, вернет результат работы перегруженного метода GetDirectoryName(string, bool), значением которого может быть null. Так как далее в теле метода ExpandFileNamePattern переменная directory используется без предварительной проверки на равенство null — можно говорить о правомерности выдачи предупреждения анализатором. Это потенциально небезопасная конструкция.
Ещё один фрагмент кода с ошибкой V3080, точнее, сразу с двумя ошибками, выданными для одной строки кода. Здесь межпроцедурный анализ не понадобился.
V3080 Possible null dereference. Consider inspecting 'spanStartLocation'. TestWorkspace.cs 574
V3080 Possible null dereference. Consider inspecting 'spanEndLocationExclusive'. TestWorkspace.cs 574
private void MapMarkupSpans(....)
{
....
foreach (....)
{
....
foreach (....)
{
....
int? spanStartLocation = null;
int? spanEndLocationExclusive = null;
foreach (....)
{
if (....)
{
if (spanStartLocation == null &&
positionInMarkup <= markupSpanStart && ....)
{
....
spanStartLocation = ....;
}
if (spanEndLocationExclusive == null &&
positionInMarkup <= markupSpanEndExclusive && ....)
{
....
spanEndLocationExclusive = ....;
break;
}
....
}
....
}
tempMappedMarkupSpans[key].
Add(new TextSpan(
spanStartLocation.Value, // <=
spanEndLocationExclusive.Value - // <=
spanStartLocation.Value));
}
}
....
}Переменные spanStartLocation и spanEndLocationExclusive имеют тип nullable int и инициализируются значением null. Далее в коде им могут быть присвоены значения, но только при выполнении определенных условий. В ряде случаев их значение останется равным null. Далее в коде происходит доступ к этим переменным по ссылке без предварительной проверки на равенство null, на что и указывает анализатор.
В коде Roslyn содержится довольно много подобных ошибок, более 100. Зачастую паттерн этих ошибок одинаков. Есть некий общий метод, который потенциально возвращает null. Результат работы данного метода используется в большом числе мест, иногда через десятки промежуточных вызовов методов или дополнительных проверок. Важно понимать, что эти ошибки не фатальны, но они потенциально могут приводить к доступу по нулевой ссылке. А обнаружить такие ошибки очень сложно. Поэтому в ряде случаев следует подумать о рефакторинге кода, при котором в случае возврата null методом выбрасывалось бы исключение. В противном случае вы сможете обезопасить свой код только тотальными проверками, что достаточно утомительно и ненадёжно. Конечно, в каждом конкретном случае решение должно приниматься исходя из особенностей проекта.
Примечание. Бывает, что на данный момент не существует ситуаций (входных данных), при которых метод вернёт null и никакой настоящей ошибки нет. Однако такой код всё равно не надёжен, так как всё может измениться при внесении изменений в код.
Чтобы закрыть тему с V3080, давайте взглянем на очевидные ошибки с уровнем достоверности High, когда доступ по нулевой ссылке более вероятен или даже неизбежен.
Possible null dereference — High
V3080 Possible null dereference. Consider inspecting 'collectionType.Type'. AbstractConvertForToForEachCodeRefactoringProvider.cs 137
public override async Task
ComputeRefactoringsAsync(CodeRefactoringContext context)
{
....
var collectionType = semanticModel.GetTypeInfo(....);
if (collectionType.Type == null &&
collectionType.Type.TypeKind == TypeKind.Error)
{
return;
}
....
}Вследствие опечатки в условии (вместо оператора || использовали &&), код работает совсем не так, как задумано, и доступ к переменной collectionType.Type будет выполнен при её равенстве null. Условие необходимо исправить следующим образом:
if (collectionType.Type == null ||
collectionType.Type.TypeKind == TypeKind.Error)
....Кстати, возможен и второй вариант развития событий: в первой части условия перепутали операторы == и !=. Тогда исправленный код имел бы вид:
if (collectionType.Type != null &&
collectionType.Type.TypeKind == TypeKind.Error)
....Этот вариант кода менее логичен, но также исправляет допущенную ошибку. Окончательное решение за авторами проекта.
Ещё одна подобная ошибка.
V3080 Possible null dereference. Consider inspecting 'action'. TextViewWindow_InProc.cs 372
private Func<IWpfTextView, Task> GetLightBulbApplicationAction(....)
{
....
if (action == null)
{
throw new
InvalidOperationException(
$"Unable to find FixAll in {fixAllScope.ToString()}
code fix for suggested action '{action.DisplayText}'.");
}
....
}Ошибка допущена при формировании сообщения для исключения. При этом производится попытка доступа к свойству action.DisplayText через переменную action, которая заведомо равна null.
И последняя ошибка V3080 уровня High.
V3080 Possible null dereference. Consider inspecting 'type'. ObjectFormatterHelpers.cs 91
private static bool IsApplicableAttribute(
TypeInfo type,
TypeInfo targetType,
string targetTypeName)
{
return type != null && AreEquivalent(targetType, type)
|| targetTypeName != null && type.FullName == targetTypeName;
}Метод небольшой, поэтому я привожу его код целиком. Условие в блоке return некорректно. В ряде случаев возможен выброс исключения NullReferenceException при доступе type.FullName. Использую скобки (здесь они не будут менять поведения) для прояснения ситуации:
return (type != null && AreEquivalent(targetType, type))
|| (targetTypeName != null && type.FullName == targetTypeName);Именно так, в соответствии с приоритетом операций, будет работать данный код. В случае, если переменная type будет равна null, мы попадём в else-проверку, где, убедившись в неравенстве null переменной targetTypeName, используем нулевую ссылку type. Исправить код можно, например, так:
return type != null &&
(AreEquivalent(targetType, type) ||
targetTypeName != null && type.FullName == targetTypeName);Думаю, на этом можно завершить изучение ошибок V3080 и посмотреть, что еще интересного удалось найти анализатору PVS-Studio в коде Roslyn.
Опечатка
V3005 The 'SourceCodeKind' variable is assigned to itself. DynamicFileInfo.cs 17
internal sealed class DynamicFileInfo
{
....
public DynamicFileInfo(
string filePath,
SourceCodeKind sourceCodeKind,
TextLoader textLoader,
IDocumentServiceProvider documentServiceProvider)
{
FilePath = filePath;
SourceCodeKind = SourceCodeKind; // <=
TextLoader = textLoader;
DocumentServiceProvider = documentServiceProvider;
}
....
}Из-за неудачного названия переменных, в конструкторе класса DynamicFileInfo допустили опечатку. Полю SourceCodeKind присваивают его же собственное значение, вместо использования параметра sourceCodeKind. Для минимизации вероятности возникновения таких ошибок рекомендуется использовать префикс подчеркивания для имен параметров в таких случаях. Приведу пример исправленного варианта кода:
public DynamicFileInfo(
string _filePath,
SourceCodeKind _sourceCodeKind,
TextLoader _textLoader,
IDocumentServiceProvider _documentServiceProvider)
{
FilePath = _filePath;
SourceCodeKind = _sourceCodeKind;
TextLoader = _textLoader;
DocumentServiceProvider = _documentServiceProvider;
}Невнимательность
V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new InvalidOperationException(FOO). ProjectBuildManager.cs 61
~ProjectBuildManager()
{
if (_batchBuildStarted)
{
new InvalidOperationException("ProjectBuilderManager.Stop()
not called.");
}
}При определенном условии деструктор должен выбросить исключение, но этого не происходит, а объект исключения просто создается. Было пропущено ключевое слово throw. Исправленный вариант кода:
~ProjectBuildManager()
{
if (_batchBuildStarted)
{
throw new InvalidOperationException("ProjectBuilderManager.Stop()
not called.");
}
}Вопрос работы с деструкторами в C# и выбросу из них исключений — тема отдельной дискуссии, которая выходит за рамки данной статьи.
Когда результат не важен
Некоторое количество предупреждений V3009 было получено для методов, которые во всех случаях возвращают одинаковое значение. Иногда это не критично, или в вызывающем коде возвращаемое значение просто не проверяют. Такие предупреждения я пропустил. Но несколько фрагментов кода показались мне подозрительными. Приведу один из них:
V3009 It's odd that this method always returns one and the same value of 'true'. GoToDefinitionCommandHandler.cs 62
internal bool TryExecuteCommand(....)
{
....
using (context.OperationContext.AddScope(....))
{
if (....)
{
return true;
}
}
....
return true;
}Метод TryExecuteCommand возвращает только true, и ничего кроме true. При этом в вызывающем коде возвращаемое значение участвует в каких-то проверках:
public bool ExecuteCommand(....)
{
....
if (caretPos.HasValue && TryExecuteCommand(....))
{
....
}
....
}Трудно сказать, насколько такое поведение опасно. Но если результат не нужен, возможно, стоит заменить тип возвращаемого значения на void и внести минимальные правки в вызывающий метод. Это сделает код более понятным и безопасным.
Другие подобные предупреждения:
- V3009 It's odd that this method always returns one and the same value of 'true'. CommentUncommentSelectionCommandHandler.cs 86
- V3009 It's odd that this method always returns one and the same value of 'true'. RenameTrackingTaggerProvider.RenameTrackingCommitter.cs 99
- V3009 It's odd that this method always returns one and the same value of 'true'. JsonRpcClient.cs 138
- V3009 It's odd that this method always returns one and the same value of 'true'. AbstractFormatEngine.OperationApplier.cs 164
- V3009 It's odd that this method always returns one and the same value of 'false'. TriviaDataFactory.CodeShapeAnalyzer.cs 254
- V3009 It's odd that this method always returns one and the same value of 'true'. ObjectList.cs 173
- V3009 It's odd that this method always returns one and the same value of 'true'. ObjectList.cs 249
Не то проверили
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'value', 'valueToSerialize'. RoamingVisualStudioProfileOptionPersister.cs 277
public bool TryPersist(OptionKey optionKey, object value)
{
....
var valueToSerialize = value as NamingStylePreferences;
if (value != null)
{
value = valueToSerialize.CreateXElement().ToString();
}
....
}Переменную value приводят к типу NamingStylePreferences. Проблема в следующей за этим проверке. Даже если переменная value не нулевая, это не гарантирует, что приведение типа было успешным и valueToSerialize содержит не null. Возможен выброс исключения NullReferenceException. Код необходимо исправить следующим образом:
var valueToSerialize = value as NamingStylePreferences;
if (valueToSerialize != null)
{
value = valueToSerialize.CreateXElement().ToString();
}И ещё одна похожая ошибка.
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'columnState', 'columnState2'. StreamingFindUsagesPresenter.cs 181
private void SetDefinitionGroupingPriority(....)
{
....
foreach (var columnState in ....)
{
var columnState2 = columnState as ColumnState2;
if (columnState?.Name == // <=
StandardTableColumnDefinitions2.Definition)
{
newColumns.Add(new ColumnState2(
columnState2.Name, // <=
....));
}
....
}
....
}Переменную columnState приводят к типу ColumnState2. Однако результат операции, переменную columnState2, далее не проверяют на равенство null. Вместо этого проверяется переменная columnState при помощи оператора условного null. Чем опасен этот код? Как и в предыдущем примере, приведение типа при помощи оператора as может закончиться неудачей, и переменная columnState2 будет равна null, что приведёт к дальнейшему выбросу исключения. Кстати, виновата здесь может быть и опечатка. Обратите внимание на условие в блоке if. Возможно, вместо columnState?.Name хотели написать columnState2?.Name. Это весьма вероятно, если учесть довольно неудачные имена переменных columnState и columnState2.
Избыточные проверки
Довольно большое количество предупреждений (более 100) было выдано на некритичные, но потенциально небезопасные конструкции, связанные с избыточными проверками. Для примера приведу одну из них.
V3022 Expression 'navInfo == null' is always false. AbstractSyncClassViewCommandHandler.cs 101
public bool ExecuteCommand(....)
{
....
IVsNavInfo navInfo = null;
if (symbol != null)
{
navInfo = libraryService.NavInfoFactory.CreateForSymbol(....);
}
if (navInfo == null)
{
navInfo = libraryService.NavInfoFactory.CreateForProject(....);
}
if (navInfo == null) // <=
{
return true;
}
....
}
public IVsNavInfo CreateForSymbol(....)
{
....
return null;
}
public IVsNavInfo CreateForProject(....)
{
return new NavInfo(....);
}Пожалуй, настоящей ошибки здесь нет. Просто хороший повод продемонстрировать связку технологий «межпроцедурный анализ + анализ потока данных» в действии. Анализатор считает, что вторая проверка navInfo == null избыточна. Действительно, перед этим значение для присвоения navInfo будет получено из метода libraryService.NavInfoFactory.CreateForProject, который сконструирует и вернет новый объект класса NavInfo. Но никак не null. Возникает вопрос, почему же анализатор не выдал предупреждение для первой проверки navInfo == null? Этому есть объяснение. Во-первых, в случае, если переменная symbol окажется равной null, то и значение navInfo останется нулевой ссылкой. Во-вторых, даже если navInfo получит значение из метода libraryService.NavInfoFactory.CreateForSymbol, это значение может быть равно null. Таким образом, первая проверка navInfo == null действительно необходима.
Недостаточно проверок
А теперь ситуация, обратная рассмотренной выше. Несколько предупреждений V3042 было получено для кода, в котором возможен доступ по нулевой ссылке. При этом всего одна-две небольшие проверки могли бы всё исправить.
Рассмотрим один интересный фрагмент кода, в котором содержатся две подобные ошибки.
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'receiver' object Binder_Expressions.cs 7770
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'receiver' object Binder_Expressions.cs 7776
private BoundExpression GetReceiverForConditionalBinding(
ExpressionSyntax binding,
DiagnosticBag diagnostics)
{
....
BoundExpression receiver = this.ConditionalReceiverExpression;
if (receiver?.Syntax != // <=
GetConditionalReceiverSyntax(conditionalAccessNode))
{
receiver = BindConditionalAccessReceiver(conditionalAccessNode,
diagnostics);
}
var receiverType = receiver.Type; // <=
if (receiverType?.IsNullableType() == true)
{
....
}
receiver = new BoundConditionalReceiver(receiver.Syntax, 0, // <=
receiverType ?? CreateErrorType(),
hasErrors: receiver.HasErrors) // <=
{ WasCompilerGenerated = true };
return receiver;
}Переменная receiver может быть равна null. Автор кода это знает, так как использует оператор условного null в условии блока if для доступа к receiver?.Syntax. Далее в коде переменная receiver используется без всяких проверок для доступа к receiver.Type, receiver.Syntax и receiver.HasErrors. Эти ошибки необходимо исправить:
private BoundExpression GetReceiverForConditionalBinding(
ExpressionSyntax binding,
DiagnosticBag diagnostics)
{
....
BoundExpression receiver = this.ConditionalReceiverExpression;
if (receiver?.Syntax !=
GetConditionalReceiverSyntax(conditionalAccessNode))
{
receiver = BindConditionalAccessReceiver(conditionalAccessNode,
diagnostics);
}
var receiverType = receiver?.Type;
if (receiverType?.IsNullableType() == true)
{
....
}
receiver = new BoundConditionalReceiver(receiver?.Syntax, 0,
receiverType ?? CreateErrorType(),
hasErrors: receiver?.HasErrors)
{ WasCompilerGenerated = true };
return receiver;
}Также необходимо убедиться, что конструктор BoundConditionalReceiver поддерживает получение значений null для своих параметров, или провести дополнительный рефакторинг.
Другие подобные ошибки:
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'containingType' object SyntaxGeneratorExtensions_Negate.cs 240
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions.cs 349
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions.cs 349
Ошибка в условии
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommonCommandLineParser.cs 109
internal static bool TryParseOption(....)
{
....
if (colon >= 0)
{
name = arg.Substring(1, colon - 1);
value = arg.Substring(colon + 1);
}
....
}В случае, если переменная colon будет равна 0, что допускает условие в коде, метод Substring выбросит исключение ArgumentOutOfRangeException. Необходимо исправление:
if (colon > 0)Возможна опечатка
V3065 Parameter 't2' is not utilized inside method's body. CSharpCodeGenerationHelpers.cs 84
private static TypeDeclarationSyntax
ReplaceUnterminatedConstructs(....)
{
....
var updatedToken = lastToken.ReplaceTrivia(lastToken.TrailingTrivia,
(t1, t2) =>
{
if (t1.Kind() == SyntaxKind.MultiLineCommentTrivia)
{
var text = t1.ToString();
....
}
else if (t1.Kind() == SyntaxKind.SkippedTokensTrivia)
{
return ReplaceUnterminatedConstructs(t1);
}
return t1;
});
....
}В лямбда-выражение передают два параметра: t1 и t2. Однако используют только t1. Выглядит подозрительно, учитывая, как легко ошибиться при использовании переменных с такими именами.
Невнимательность
V3083 Unsafe invocation of event 'TagsChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PreviewUpdater.Tagger.cs 37
public void OnTextBufferChanged()
{
if (PreviewUpdater.SpanToShow != default)
{
if (TagsChanged != null)
{
var span = _textBuffer.CurrentSnapshot.GetFullSpan();
TagsChanged(this, new SnapshotSpanEventArgs(span)); // <=
}
}
}Событие TagsChanged вызывают небезопасно. Между проверкой на равенство null и вызовом события, от него могут успеть отписаться, тогда будет выброшено исключение. Более того, в теле блока if непосредственно перед вызовом события делаются ещё какие-то операции. Я назвал эту ошибку «Невнимательность», потому что в других местах в коде с этим событием работают более аккуратно, например, так:
private void OnTrackingSpansChanged(bool leafChanged)
{
var handler = TagsChanged;
if (handler != null)
{
var snapshot = _buffer.CurrentSnapshot;
handler(this,
new SnapshotSpanEventArgs(snapshot.GetFullSpan()));
}
}Использование дополнительной переменной handler исключает появление проблемы. В методе OnTextBufferChanged необходимо внести правки для такой же безопасной работы с событием.
Пересекающиеся диапазоны
V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. ILBuilderEmit.cs 677
internal void EmitLongConstant(long value)
{
if (value >= int.MinValue && value <= int.MaxValue)
{
....
}
else if (value >= uint.MinValue && value <= uint.MaxValue)
{
....
}
else
{
....
}
}Для лучшего понимания, перепишу данный фрагмент кода, заменив имена констант на их фактические значения:
internal void EmitLongConstant(long value)
{
if (value >= -2147483648 && value <= 2147483648)
{
....
}
else if (value >= 0 && value <= 4294967295)
{
....
}
else
{
....
}
}Наверное, здесь нет настоящей ошибки, но условие выглядит странно. Вторая его часть (else if) будет выполняться только для диапазона значений от 2147483648 + 1 до 4294967295.
Ещё пара подобных предупреждений:
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. LocalRewriter_Literal.cs 109
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. LocalRewriter_Literal.cs 66
Ещё о проверках на равенство null (или их отсутствии)
Несколько ошибок V3095 о проверке переменной на равенство null уже после использования. Первая неоднозначна, рассмотрим код.
V3095 The 'displayName' object was used before it was verified against null. Check lines: 498, 503. FusionAssemblyIdentity.cs 498
internal static IAssemblyName ToAssemblyNameObject(string displayName)
{
if (displayName.IndexOf('\0') >= 0)
{
return null;
}
Debug.Assert(displayName != null);
....
}Предполагается, что ссылка displayName может быть нулевой. Для этого сделали проверку Debug.Assert. Непонятно только, почему она идёт уже после использования строки. Также следует учитывать, что для других, отличных от Debug конфигураций, компилятор уберёт Debug.Assert из кода вовсе. Значит ли это, что только для Debug возможно получение нулевой ссылки? И если это не так, то почему не была сделана проверка string.IsNullOrEmpty(string), например. Это вопросы к авторам кода.
Следующая ошибка более очевидна.
V3095 The 'scriptArgsOpt' object was used before it was verified against null. Check lines: 321, 325. CommonCommandLineParser.cs 321
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
....
while (args.Count > 0)
{
....
if (parsingScriptArgs)
{
scriptArgsOpt.Add(arg); // <=
continue;
}
if (scriptArgsOpt != null)
{
....
}
....
}
}Думаю, этот фрагмент кода не требует пояснений. Приведу исправленный вариант:
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
....
while (args.Count > 0)
{
....
if (parsingScriptArgs)
{
scriptArgsOpt?.Add(arg);
continue;
}
if (scriptArgsOpt != null)
{
....
}
....
}
}В коде Roslyn нашлось еще 15 подобных ошибок:
- V3095 The 'LocalFunctions' object was used before it was verified against null. Check lines: 289, 317. ControlFlowGraphBuilder.RegionBuilder.cs 289
- V3095 The 'resolution.OverloadResolutionResult' object was used before it was verified against null. Check lines: 579, 588. Binder_Invocation.cs 579
- V3095 The 'resolution.MethodGroup' object was used before it was verified against null. Check lines: 592, 621. Binder_Invocation.cs 592
- V3095 The 'touchedFilesLogger' object was used before it was verified against null. Check lines: 111, 126. CSharpCompiler.cs 111
- V3095 The 'newExceptionRegionsOpt' object was used before it was verified against null. Check lines: 736, 743. AbstractEditAndContinueAnalyzer.cs 736
- V3095 The 'symbol' object was used before it was verified against null. Check lines: 422, 427. AbstractGenerateConstructorService.Editor.cs 422
- V3095 The '_state.BaseTypeOrInterfaceOpt' object was used before it was verified against null. Check lines: 132, 140. AbstractGenerateTypeService.GenerateNamedType.cs 132
- V3095 The 'element' object was used before it was verified against null. Check lines: 232, 233. ProjectUtil.cs 232
- V3095 The 'languages' object was used before it was verified against null. Check lines: 22, 28. ExportCodeCleanupProvider.cs 22
- V3095 The 'memberType' object was used before it was verified against null. Check lines: 183, 184. SyntaxGeneratorExtensions_CreateGetHashCodeMethod.cs 183
- V3095 The 'validTypeDeclarations' object was used before it was verified against null. Check lines: 223, 228. SyntaxTreeExtensions.cs 223
- V3095 The 'text' object was used before it was verified against null. Check lines: 376, 385. MSBuildWorkspace.cs 376
- V3095 The 'nameOrMemberAccessExpression' object was used before it was verified against null. Check lines: 206, 223. CSharpGenerateTypeService.cs 206
- V3095 The 'simpleName' object was used before it was verified against null. Check lines: 83, 85. CSharpGenerateMethodService.cs 83
- V3095 The 'option' object was used before it was verified against null. Check lines: 23, 28. OptionKey.cs 23
Рассмотрим ошибки V3105. Здесь используется оператор условного null при инициализации переменной, а вот далее в коде переменная используется без проверок на равенство null.
О следующей ошибке сигнализируют сразу два предупреждения.
V3105 The 'documentId' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeLensReferencesService.cs 138
V3105 The 'documentId' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeLensReferencesService.cs 139
private static async Task<ReferenceLocationDescriptor>
GetDescriptorOfEnclosingSymbolAsync(....)
{
....
var documentId = solution.GetDocument(location.SourceTree)?.Id;
return new ReferenceLocationDescriptor(
....
documentId.ProjectId.Id,
documentId.Id,
....);
}Переменная documentId может быть инициализирована значением null. В результате, создание объекта ReferenceLocationDescriptor закончится выбросом исключения. Код необходимо исправить:
return new ReferenceLocationDescriptor(
....
documentId?.ProjectId.Id,
documentId?.Id,
....);Также далее в коде необходимо предусмотреть возможность равенства null переменных, передаваемых в конструктор.
Другие похожие ошибки в коде:
- V3105 The 'symbol' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. SymbolFinder_Hierarchy.cs 44
- V3105 The 'symbol' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. SymbolFinder_Hierarchy.cs 51
Приоритеты и скобки
V3123 Perhaps the '?:' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its condition. Edit.cs 70
public bool Equals(Edit<TNode> other)
{
return _kind == other._kind
&& (_oldNode == null) ? other._oldNode == null :
_oldNode.Equals(other._oldNode)
&& (_newNode == null) ? other._newNode == null :
_newNode.Equals(other._newNode);
}Условие в блоке return вычисляется совсем не так, как думал разработчик. Предполагалось, что первым условием будет _kind == other._kind (поэтому после этого условия сделан перенос строки), а затем последовательно будут вычислены блоки условий с оператором "?". На самом деле, первым условием будет _kind == other._kind && (_oldNode == null). Это связано с тем, что оператор && имеет более высокий приоритет, чем оператор "?". Для исправления ошибки, необходимо взять в скобки все выражения оператора "?":
return _kind == other._kind
&& ((_oldNode == null) ? other._oldNode == null :
_oldNode.Equals(other._oldNode))
&& ((_newNode == null) ? other._newNode == null :
_newNode.Equals(other._newNode));На этом я завершаю описание найденных ошибок.
Выводы
Несмотря на значительное число ошибок, которые мне удалось обнаружить, в пересчете на размер кода проекта Roslyn (2 770 000 строк) это составит довольно незначительную величину. Как и Андрей в предыдущей статье, я также готов признать высокое качество этого проекта.
Хочу отметить, что подобные эпизодические проверки кода не имеют ничего общего с методологией статического анализа и практически не приносят пользы. Статический анализ должен применяться регулярно, а не от случая к случаю. Тогда многие ошибки будут исправлены на самых ранних этапах, а, следовательно, стоимость их исправления будет в десятки раз ниже. Более подробно эта мысль изложена в этой маленькой заметке, с которой я очень прошу ознакомиться.
Вы можете самостоятельно поискать ещё какие-то ошибки как в рассмотренном проекте, так и в любом другом. Для этого просто необходимо скачать и попробовать наш анализатор.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Checking the Roslyn Source Code.
