Проверяем качество кода в проектах.NET Foundation: LINQ to DB
.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.
Deja Vu
Начнём, пожалуй, с самых распространённых случаев, встречавшихся каждому разработчику хоть раз: повторяющийся код.
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 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(
Query query,
IDataContext dataContext,
Mapper 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.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 не требуется.
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 BuildRows(EvaluationContext context)
{
if (Rows != null)
return Rows;
....
var rows = new List();
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 на своём проекте.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Danila Karpov. PVS-Studio checks the code quality in the .NET Foundation projects: LINQ to DB.
