.NET Foundation — это независимая организация, созданная Microsoft для поддержки проектов с открытым исходным кодом на платформе DotNet. В настоящее время организация собрала под своим крылом множество библиотек. Некоторые из этих библиотек мы уже протестировали с помощью PVS-Studio. Следующий проект для проверки анализатором — LINQ to DB.
Введение
LINQ to DB — это инфраструктура доступа к базе данных, основанная на LINQ. LINQ to DB собрал лучшее из своих предшественников. Он позволяет работать с различными СУБД, тогда как LINQ to SQL раньше позволял работать только с MS SQL. Это не так тяжело и сложно, как LINQ to SQL или Entity Framework. LINQ to DB обеспечивает больший контроль и быстрый доступ к данным. Фреймворк не такой уж большой: он написан на C# и содержит более 40 000 строк кода.
LINQ to DB также является одним из проектов .NET Foundation. Ранее мы проверили проекты этой организации: Windows Forms, Xamarin.Forms, Teleric UI для UWP и др.
Чуть меньше разговоров, чуть больше действий! Проверим код LINQ to DB, взятый из официального репозитория на GitHub. С помощью нашего статического анализатора PVS-Studio мы посмотрим, все ли в порядке с преемником LINQ.
Дежавю
Начну, пожалуй, с самых частых случаев, с которыми хотя бы раз сталкивался каждый разработчик: дублирование кода.
V3001 Слева и справа от оператора || одинаковые подвыражения genericDefinition == typeof(Tuple‹,,,,,,,›). TypeExtensions.cs 230
public static bool IsTupleType(this Type type)
{
....
if (genericDefinition == typeof(Tuple<>)
|| genericDefinition == typeof(Tuple<,>)
|| genericDefinition == typeof(Tuple<,,>)
|| genericDefinition == typeof(Tuple<,,,>)
|| genericDefinition == typeof(Tuple<,,,,>)
|| genericDefinition == typeof(Tuple<,,,,,>)
|| genericDefinition == typeof(Tuple<,,,,,,>)
|| genericDefinition == typeof(Tuple<,,,,,,,>)
|| genericDefinition == typeof(Tuple<,,,,,,,>))
{
return true;
}
....
}Первое сообщение анализатора бросилось в глаза. Те, кто редко использует кортежи, могут подумать, что это обычное последствие копирования-вставки. Без колебаний можно предположить, что разработчик пропустил запятую в последней строке условия Tuple‹,,,,,,›. Однако даже функциональность Visual Studio показала мне, что я ошибался.
Кортежи в C# делятся на 8 типов по количеству элементов. 7 из них отличаются только разным количеством элементов, от 1 до 7 соответственно. В данном случае они соответствуют первым семи строкам условия. И последний, Tuple‹,,,,,,,›, включает 8 или более элементов.
В результате при попытке написать Tuple‹,,,,,,,,› Visual Studio сообщает, что такого кортежа нет. Оказывается, в примере выше идет дополнительная проверка на соответствие переменной типу Tuple‹,,,,,,,›, а не отсутствующая запятая, как казалось изначально.
А вот очередное предупреждение анализатора, попавшееся на глаза, уже вызвало пару вопросов.
V3003 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 256, 273. SqlPredicate.cs 256
public ISqlPredicate Reduce(EvaluationContext context)
{
....
if (Operator == Operator.Equal)
{
....
}
else
if (Operator == Operator.NotEqual)
{
search.Conditions.Add(
new SqlCondition(false, predicate, true));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr1, false), false));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr2, true), true));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr1, true), false));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr2, false), false));
}
else
if (Operator == Operator.LessOrEqual ||
Operator == Operator.GreaterOrEqual)
{
....
}
else if (Operator == Operator.NotEqual)
{
search.Conditions.Add(
new SqlCondition(false, predicate, true));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr1, false), false));
search.Conditions.Add(
new SqlCondition(false, new IsNull(Expr2, false), false));
}
else
{
....
}
....
}По анализатору во фрагменте две ветки с одинаковыми условиями. Вот почему второе условие всегда ложно. Кстати, на это косвенно указывает и другое сообщение анализатора: V3022 Выражение Оператор == Оператор.Неравно всегда ложно. SqlPredicate.cs 273.
В примере мы видим повторение условия Operator == Operator.NotEqual. Эти две ветви условий выполняют немного разные операции. Итак, вопрос — какая из веток действительно нужна разработчикам? После небольшого анализа функции Reduce предположил, что скорее всего разработчикам нужна именно первая ветка. Тот, у которого есть сравнение с Operator.NotEqual. Его функциональность больше похожа на Equal и LessOrEqual. В отличие от своего близнеца, вторая ветка с NotEqual имеет абсолютно идентичный функционал с веткой else. Вот ссылка на исходный файл для сравнения, обратите внимание на строки 245–284.
V3008 Переменной newElement дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 1320, 1315. ConvertVisitor.cs 1320
internal IQueryElement? ConvertInternal(IQueryElement? element) { .... switch (element.ElementType) { .... case QueryElementType.WithClause: { var with = (SqlWithClause)element;var clauses = ConvertSafe(with.Clauses);if (clauses != null && !ReferenceEquals(with.Clauses, clauses)) { newElement = new SqlWithClause() { Clauses = clauses };newElement = new SqlWithClause() { Clauses = clauses }; } break; } .... } .... }
В этом фрагменте кода автор, видимо, не смог определиться со стилем. Они не смогли выбрать один и оставили оба варианта. Это именно то, что определил анализатор. Я бы порекомендовал выбрать один и удалить ненужное назначение. Анализатор выдал такое же сообщение еще раз:
V3008 Переменной Стоп дважды подряд присваиваются значения. Возможно, это ошибка. Отметьте строки: 25, 24. TransformInfo.cs 25
public TransformInfo(Expression expression, bool stop, bool @continue)
{
Expression = expression;
Stop = false;
Stop = stop;
Continue = @continue;
}Теперь это другая история. Здесь переменной Stop сначала присваивается значение false, а сразу после этого в следующей строке — значение параметра stop. Логично, что в этом случае нужно убрать первое присваивание, так как оно не используется и моментально перезаписывается значением аргумента.
Куда пропала переменная?
V3010 Необходимо использовать возвращаемое значение функции ToDictionary. ReflectionExtensions.cs 34
public static MemberInfo[] GetPublicInstanceValueMembers(this Type type)
{
if (type.IsAnonymous())
{
type.GetConstructors().Single()
.GetParameters()
.Select((p, i) => new { p.Name, i })
.ToDictionary(_ => _.Name, _ => _.i);
}
....
}Каков был замысел разработчика с этим фрагментом? Кажется, не хватает переменной, которой нужно присвоить результат выполнения этого выражения. В противном случае логика действий непонятна. При дальнейшем выполнении функции GetPublicInstanceValueMembers такого выражения не происходит. Замысел разработчика неизвестен. Возможно, этот фрагмент кода находится в разработке, поэтому нам нужно дождаться его дальнейшего развития.
V3025 Неверный формат. При вызове функции AppendFormat ожидается другое количество элементов формата. Аргументы не используются: 1-й. ExpressionTestGenerator.cs 663
void BuildType(Type type, MappingSchema mappingSchema)
{
....
_typeBuilder.AppendFormat(
type.IsGenericType ?
@"
{8} {6}{7}{1} {2}<{3}>{5}
{{{4}{9}
}}
"
:
@"
{8} {6}{7}{1} {2}{5}
{{{4}{9}
}}
",
MangleName(isUserName, type.Namespace, "T"),
type.IsInterface ? "interface"
: type.IsClass ? "class"
: "struct",
name,
type.IsGenericType ? GetTypeNames(type.GetGenericArguments(), ",")
: null,
string.Join("\r\n", ctors),
baseClasses.Length == 0 ? ""
: " : " + GetTypeNames(baseClasses),
type.IsPublic ? "public "
: "",
type.IsAbstract && !type.IsInterface ? "abstract "
: "",
attr,
members.Length > 0 ? (ctors.Count != 0 ? "\r\n" : "") +
string.Join("\r\n", members)
: string.Empty);
}В этом фрагменте мы видим форматирование строки. Вопрос в том, куда делся первый вызов аргумента? В первой форматированной строке разработчик использовал индексы от 1 до 9. Но аргумент с индексом 0 либо разработчику не понадобился, либо про него забыли.
V3137 Переменная версия присваивается, но не используется к концу функции. Query.cs 408
public void TryAdd(IDataContext dataContext, Query<T> query, QueryFlags flags)
{
QueryCacheEntry[] cache;
int version;
lock (_syncCache)
{
cache = _cache;
version = _version;
}
....
lock(_syncCashe)
{
....
var versionsDiff = _version - version;
....
_cache = newCache;
_indexes = newPriorities;
version = _version;
}
}У нас тут сложная ситуация. Согласно диагностическому сообщению, локальной переменной version присваивается значение без использования этого значения к концу функции. Ну, по одной штуке.
В самом начале значение из _version присваивается переменной version. Во время выполнения кода значение version не изменяется. Он вызывается только один раз для вычисления разницы с _version. И в конце _version снова присваивается version. Наличие операторов lock подразумевает, что во время выполнения фрагмента кода вне блока с переменной _version изменения могут происходить параллельно извне функции.
В данном случае логично предположить, что в конце нужно было поменять местами версию на _version. Тем не менее, кажется странным присваивать глобальное значение локальной переменной в конце функции. Анализатор еще раз выдал аналогичное сообщение: V3137 Переменная leftcontext назначена, но к концу функции не используется. ExpressionBuilder.SqlBuilder.cs 1 989
Одна итерация цикла.
V3020 Безусловный возврат внутри цикла. QueryRunner.cs 751
static T ExecuteElement<T>( Query query, IDataContext dataContext, Mapper<T> mapper, Expression expression, object?[]? ps, object?[]? preambles) { using (var runner = dataContext.GetQueryRunner(query, 0, expression, ps, preambles)) { using (var dr = runner.ExecuteReader()) { while (dr.Read()) { var value = mapper.Map(dataContext, runner, dr); runner.RowsCount++; return value; } }return Array<T>.Empty.First(); } }
Естественно использовать конструкцию while (reader.Read()), если вам нужно выбрать несколько строк из базы данных. Но здесь в цикле мы видим return без каких-либо условий, а это значит, что нужна только одна строка. Тогда вопрос — зачем использовать цикл? В нашем случае цикл while не нужен. Если вам нужен только первый элемент из базы данных, вы можете использовать простое if.
Повторяющиеся действия делают совершенство
Случаи с повторными проверками по-прежнему присутствуют.
V3022 Выражение версия › 15 всегда верно. SqlServerTools.cs 250
internal static IDataProvider? ProviderDetector(IConnectionStringSettings css, string connectionString) { .... if (int.TryParse(conn.ServerVersion.Split('.')[0], out var version)) { if (version <= 8) return GetDataProvider(SqlServerVersion.v2000, provider);using (var cmd = conn.CreateCommand()) { .... switch (version) { case 8 : return GetDataProvider(SqlServerVersion.v2000, provider); case 9 : return GetDataProvider(SqlServerVersion.v2005, provider); case 10 : return GetDataProvider(SqlServerVersion.v2008, provider); case 11 : case 12 : return GetDataProvider(SqlServerVersion.v2012, provider); case 13 : return GetDataProvider(SqlServerVersion.v2016, provider); case 14 : case 15 : return GetDataProvider(SqlServerVersion.v2017, provider); default : if (version > 15) return GetDataProvider(SqlServerVersion.v2017, provider); return GetDataProvider(SqlServerVersion.v2008, provider); } } } .... }
Вы видели фрагмент кода. Вы заметили ошибку? Анализатор говорит, что в этом примере условие версия › 15 всегда истинно, поэтому строка return GetDataProvider(SqlServerVersion.v2008, provider) является недостижимым кодом. Но давайте подробнее рассмотрим функцию ProviderDetector.
Во-первых, предлагаю обратить внимание на условие версия ‹= 8. Это означает, что дальнейший код не может быть выполнен, если версия SQLServer 8 или более ранняя. Но если мы посмотрим вниз, мы увидим ветвь case 8 в операторе switch. Эта ветвь выполняет идентичный код. Фрагмент является недостижимым кодом, потому что 8-я версия больше не может быть использована из-за вышеуказанного условия. А так как он по-прежнему выполняет тот же код, то вы можете смело удалить эту ветку из switch.
Во-вторых, поговорим о сообщении анализатора. Как мы уже говорили, все версии до или равные 8-й не выйдут за пределы первого условия. Версии с 9-й по 15-ю ловятся в ветках switch. В этом случае мы попадаем в ветку default при выполнении условия версия › 15. Это делает бессмысленной проверку того же условия внутри ветки default.
Но остается вопрос: что нам нужно прописать в GetDataProvider — v2017 или v2008? Если посмотреть на остальные ветки switch, то можно предположить следующее: чем старее версия, тем выше и год выпуска SQLServer. В этом случае воспользуемся SQLServerVersion.V2017. Правильная версия этого кода должна выглядеть так:
internal static IDataProvider? ProviderDetector(IConnectionStringSettings css, string connectionString) { .... if (int.TryParse(conn.ServerVersion.Split('.')[0], out var version)) { if (version <= 8) return GetDataProvider(SqlServerVersion.v2000, provider);using (var cmd = conn.CreateCommand()) { .... switch (version) { case 9 : return GetDataProvider(SqlServerVersion.v2005, provider); case 10 : return GetDataProvider(SqlServerVersion.v2008, provider); case 11 : case 12 : return GetDataProvider(SqlServerVersion.v2012, provider); case 13 : return GetDataProvider(SqlServerVersion.v2016, provider); case 14 : case 15 : return GetDataProvider(SqlServerVersion.v2017, provider); default : return GetDataProvider(SqlServerVersion.v2017, provider); } } } .... }
Теперь рассмотрим более простой пример срабатывания диагностики V3022 в этом проекте.
V3022 Выражение table == null всегда верно. LoadWithBuilder.cs 113
TableBuilder.TableContext GetTableContext(IBuildContext ctx, Expression path, out Expression? stopExpression) { stopExpression = null;var table = ctx as TableBuilder.TableContext;if (table != null) return table;if (ctx is LoadWithContext lwCtx) return lwCtx.TableContext;if (table == null) { .... } .... }
Что же мы имеем здесь? Переменная table дважды сравнивается с null. В первый раз условие проверяет переменную на неравенство с null. При выполнении условия происходит выход из функции. Это означает, что код ниже ветви условия выполняется только тогда, когда table = null. Никакие действия над переменной не выполняются до следующей проверки. В результате, когда код достигает условия table == null, эта проверка всегда возвращает true.
Диагностика V3022 выдала еще несколько полезных предупреждений. Мы не будем рассматривать их все в статье, но рекомендуем авторам самим проверить проект и увидеть все предупреждения анализатора PVS-Studio.
V3063 Часть условного выражения всегда истинна, если она оценивается: field.Field.CreateFormat != null. BasicSqlBuilder.cs 1255
protected virtual void BuildCreateTableStatement(....)
{
....
if (field.Field.CreateFormat != null)
{
if (field.Field.CreateFormat != null && field.Identity.Length == 0)
{
....
}
}
....
}В приведенном выше фрагменте кода видно, что field.Field.CreateFormat дважды проверяется на null. Но в этом случае вторая проверка выполняется непосредственно в ветке первой проверки. Поскольку первая проверка прошла успешно, поэтому, когда проверяемое значение не изменилось, нет необходимости сравнивать значение field.Field.CreateFormat с null во второй раз. .
ноль как то, за что можно умереть
V3022 Выражение строки всегда не равно нулю. Оператор ‘?.’ является излишним. SQLiteSqlBuilder.cs 214
protected override void BuildSqlValuesTable( SqlValuesTable valuesTable, string alias, out bool aliasBuilt) { valuesTable = ConvertElement(valuesTable); var rows = valuesTable.BuildRows(OptimizationContext.Context);if (rows.Count == 0) { .... } else { ....if (rows?.Count > 0) { .... }.... } aliasBuilt = false; }
По мнению анализатора, в строке этого фрагмента кода if (rows?.Count › 0)проверка на null не нужна, так как rows не может быть null в этот момент. Разберемся, почему. Результат функции BuildRows присваивается переменной rows. Вот фрагмент кода функции:
internal IReadOnlyList<ISqlExpression[]> BuildRows(EvaluationContext context) { if (Rows != null) return Rows; .... var rows = new List<ISqlExpression[]>(); if (ValueBuilders != null) { foreach (var record in source) { ....var row = new ISqlExpression[ValueBuilders!.Count]; var idx = 0; rows.Add(row);.... } } return rows; }
Поскольку BuildRows не может вернуть null, то, по мнению анализатора, проверка на null избыточна. Но если BuildRows вернул null — что подразумевается под условием rows rows?.Count › 0 — тогда в момент rows.Count == 0 условие, было бы выдано NullReferenceException. В таком случае вам также потребуется выполнить проверку null, чтобы избежать ошибки. До тех пор текущий код выглядит подозрительно, и проверка null является избыточной.
Мы добрались до сообщения, которое заставило меня крепко задуматься и сделать пару проверок.
V3042 Возможное исключение NullReferenceException. Операторы ?. и . используются для доступа к членам объекта _update SqlUpdateStatement.cs 60
public override ISqlTableSource? GetTableSource(ISqlTableSource table)
{
....
if (table == _update?.Table)
return _update.Table;
....
}Небольшой фрагмент, условие и выход из функции.
Итак, анализатор обнаружил, что к update обращаются двумя способами — с нулевым условным оператором и без него. Вы можете подумать, что условие выполняется, только если _update не равно null и обе части равенства одинаковы. Но. Большое жирное НО.
В случае, когда table и _update равны null, тогда _update?.Table возвращает null. Это соответствует условию. Затем при попытке вызвать _update.Table вы получите NullReferenceException. Если мы можем вернуть null, как сообщает нам ISqlTableSource? в объявлении функции, то мы должны написать return _update?.Table, чтобы избежать ошибки .
Заключение
Проект LINQ to DB большой и сложный, что делает его более интересным для проверки. У проекта очень большое сообщество, и нам посчастливилось получить несколько интересных предупреждений.
Если вы хотите узнать, есть ли в вашей кодовой базе подобные ошибки, вы можете попробовать PVS-Studio на своем проекте.
