.NET Foundation – независимая организация, основанная Microsoft с целью поддержки open source проектов на платформе DotNet. Под их крылом на данный момент собралось множество библиотек, некоторые из которых уже проходили проверку анализатором PVS-Studio. Следующим проектом для проверки анализатором будет LINQ to DB.
LINQ to DB – фреймворк для работы с базами данных, основанный на LINQ. Он собрал в себе лучшее от предшественников, позволяя работать с различными СУБД, тогда как 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 for UWP и др.
Меньше слов – больше дела. Проверим же код LINQ to DB, взятый с официального репозитория на GitHub, с помощью нашего статического анализатора PVS-Studio и посмотрим, всё ли хорошо у наследника LINQ.
Начнём, пожалуй, с самых распространённых случаев, встречавшихся каждому разработчику хоть раз: повторяющийся код.
V3001 There are identical sub-expressions 'genericDefinition == typeof(Tuple<,,,,,,,>)' to the left and to the right of the '||' operator. 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;
}
....
}Первое же сообщение анализатора привлекло мой взгляд. Кто нечасто пользуется кортежами, может подумать, что это обычное последствие copy-paste. Не задумываясь можно предположить, что в последней строке условия у Tuple<,,,,,,,> пропущена запятая. Однако даже встроенный в Visual Studio функционал показал мне мою неправоту.
Кортежи в C# разделяются на 8 видов по количеству элементов. 7 из них отличаются лишь разным количеством элементов, от 1 до 7 соответственно. В данном случае они соответствуют первым семи строчкам в условии. И последний, тот самый Tuple<,,,,,,,>, включает в себя 8 и более элементов.
В итоге при попытке написать Tuple<,,,,,,,,> Visual Studio сообщит нам, что такого кортежа нет. Выходит, в приведенном выше примере ошибка в наличии лишней проверки на соответствие переменной с типом Tuple<,,,,,,,>, а не в отсутствующей запятой, как показалось изначально.
А вот следующее сообщение анализатора, попавшееся мне на глаза, уже вызвало пару вопросов.
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 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 Expression 'Operator == Operator.NotEqual' is always false. SqlPredicate.cs 273.
В примере у нас повторяется условие Operator == Operator.NotEqual. Эти две ветки условий выполняют немного отличные операции. Из этого возникает вопрос – а какая из веток действительно требуется по мнению разработчика? После небольшого анализа функции Reduce я предположу, что скорее всего разработчикам нужна именно первая ветка со сравнением с Operator.NotEqual. Её функционал более схож с ветками Equal и LessOrEqual. В отличие от двойника, вторая ветка с NotEqual имеет абсолютно идентичный функционал с веткой else. Вот вам для сравнения ссылка на оригинальный файл, обратите внимание на строчки с 245 по 284.
V3008 The 'newElement' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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 The 'Stop' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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 The return value of function 'ToDictionary' is required to be utilized. 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 Incorrect format. A different number of format items is expected while calling 'AppendFormat' function. Arguments not used: 1st. 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 The 'version' variable is assigned but is not used by the end of the function. Query.cs 408
public void TryAdd(IDataContext dataContext, Query<T> query, QueryFlags flags)
{
QueryCacheEntry[] cache;
int version;
lock (_syncCache)
{
cache = _cache;
version = _version;
}
....
lock(_syncCaсhe)
{
....
var versionsDiff = _version - version;
....
_cache = newCache;
_indexes = newPriorities;
version = _version;
}
}В этом примере несколько запутанная ситуация. Сообщение говорит нам, что локальной переменной version присвоено значение, но оно не использовано до конца функции. Пройдемся по порядку.
В самом начале version присваивается значение из _version. В ходе выполнения значение version не изменяется, лишь раз вызываясь для подсчета разницы с _version. И в конце version снова присваивается значение _version. Наличие операторов lock подразумевает, что во время выполнения фрагмента кода за пределами блокировки с переменной _version параллельно могут происходить изменения извне функции.
В таком случае логично предположить, что в конце требовалось поменять местами version и _version. Всё же кажется странным в конце функции присваивать локальной переменной значение глобальной. Анализатор обнаружил ещё одно такое же сообщение в коде проекта: V3137 The 'leftContext' variable is assigned but is not used by the end of the function. ExpressionBuilder.SqlBuilder.cs 1989
V3020 An unconditional 'return' within a loop. 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 Expression 'version > 15' is always true. 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);
}
}
}
....
}Взглянув на этот фрагмент кода, заметили ли вы ошибку? Анализатор сообщает нам, что в данном примере условие version > 15 всегда истинно, из-за чего строка return GetDataProvider(SqlServerVersion.v2008, provider) является недостижимым кодом. Но взглянем на функцию ProviderDetector повнимательнее.
Первое, на что я предлагаю обратить внимание – на условие version <= 8. Здесь в коде отсекаются все версии SQLServer до 8 включительно. Но стоит опустить взгляд чуть ниже, как мы видим в операторе switch ветку case 8, которая выполняет идентичный код. Этот фрагмент является недостижимым кодом, т.к. 8 версия уже не может быть из-за условия выше. И раз уж она всё равно выполняет тот же код, то можно спокойно убрать эту ветку из switch.
Второй же момент связан с сообщением анализатора. Как уже сказали ранее, все версии моложе или равные 8 уже не пройдут дальше первого условия. Версии с 9 по 15 отлавливаются в ветках switch. В таком случае в ветку default попадаем при выполнении условия version > 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 Expression 'table == null' is always true. 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 A part of conditional expression is always true if it is evaluated: 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. Но в этом случае вторая проверка выполняется прямо в ветке первой проверки. Так как одна проверка уже прошла успешно, то второй раз, когда проверяемое значение не изменилось, сравнивать с null не требуется.
V3022 Expression 'rows' is always not null. The operator '?.' is excessive. 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 в этот момент. Разберёмся почему. Переменной rows присваивается результат функции BuildRows. Вот фрагмент этой функции:
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?.Count > 0, то ещё на моменте проверки условия rows.Count == 0 вылетело бы исключение NullReferenceException. В таком случае и в этом условии нужно бы тоже сделать проверку на null, чтобы избежать ошибки. А пока в текущем виде код выглядит подозрительным и проверка на null просто излишняя.
Мы добрались до сообщения, которое заставило меня пораскинуть мозгами и провести пару проверок.
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the '_update' object SqlUpdateStatement.cs 60
public override ISqlTableSource? GetTableSource(ISqlTableSource table)
{
....
if (table == _update?.Table)
return _update.Table;
....
}Маленький фрагмент, условие и выход из функции.
Итак, анализатор сообщает нам, что к _update применяются два вида обращения. С использованием null-conditional оператора и без. Можно подумать, что условие выполнится только в том случае, когда _update не равен null и обе части равенства одинаковы. Но. Большое и жирное 'но'.
В случае, когда table и _update равны null, то _update?.Table вернет null, что удовлетворит условию. Тогда при попытке вызвать _update.Table возникнет NullReferenceException. Если у нас есть возможность вернуть null, о чем сообщает нам ISqlTableSource? в объявлении функции, то стоит написать return _update?.Table, дабы избежать возникновения ошибки.
Проект LINQ to DB большой и сложный, отчего его проверка стала только интересней. У проекта очень большое сообщество, и нам повезло найти некоторое количество интересных предупреждений.
Если вас заинтересовало, нет ли подобного в вашей кодовой базе, можете попробовать PVS-Studio на своём проекте.