# Проверяем качество кода в проектах\.NET Foundation: LINQ to DB \.NET Foundation – независимая организация, основанная Microsoft с целью поддержки open source проектов на платформе DotNet\. Под их крылом на данный момент собралось множество библиотек, некоторые из которых уже проходили проверку анализатором PVS\-Studio\. Следующим проектом для проверки анализатором будет LINQ to DB\. ![0887_LINQ2DB_ru/image1.png](https://import.viva64.com/docx/blog/0887_LINQ2DB_ru/image1.png) ## Введение [LINQ to DB](https://dotnetfoundation.org/projects/linq2db) – фреймворк для работы с базами данных, основанный на LINQ\. Он собрал в себе лучшее от предшественников, позволяя работать с различными СУБД, тогда как LINQ to SQL в своё время позволял работать только с MS SQL\. Являясь более легковесным и простым, чем LINQ to SQL или Entity Framework, LINQ to DB предоставляет большой контроль и быстрый доступ к данным\. Фреймворк небольшой, написан на языке C\# и насчитывает более 40 000 строк кода\. LINQ to DB также входит в список проектов \.NET Foundation\. Мы уже ранее проверяли проекты этой организации: [Windows Forms](https://pvs-studio.ru/ru/blog/posts/csharp/0653/), [Xamarin\.Forms](https://pvs-studio.ru/ru/blog/posts/csharp/0400/), [Teleric UI for UWP](https://pvs-studio.ru/ru/blog/posts/csharp/0677/) и др\. Меньше слов – больше дела\. Проверим же код LINQ to DB, взятый с официального репозитория на [GitHub](https://github.com/linq2db/linq2db), с помощью нашего статического анализатора [PVS\-Studio](https://pvs-studio.ru/ru/) и посмотрим, всё ли хорошо у наследника LINQ\. ## Deja Vu Начнём, пожалуй, с самых распространённых случаев, встречавшихся каждому разработчику хоть раз: повторяющийся код\. [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'genericDefinition \=\= typeof\(Tuple<,,,,,,,\>\)' to the left and to the right of the '\|\|' operator\. TypeExtensions\.cs 230 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'Operator \=\= Operator\.NotEqual' is always false\. SqlPredicate\.cs 273\. В примере у нас повторяется условие _Operator \=\= Operator\.NotEqual_\. Эти две ветки условий выполняют немного отличные операции\. Из этого возникает вопрос – а какая из веток действительно требуется по мнению разработчика? После небольшого анализа функции _Reduce_ я предположу, что скорее всего разработчикам нужна именно первая ветка со сравнением с _Operator\.NotEqual_\. Её функционал более схож с ветками _Equal_ и _LessOrEqual_\. В отличие от двойника, вторая ветка с _NotEqual_ имеет абсолютно идентичный функционал с веткой _else_\. Вот вам для сравнения [ссылка](https://github.com/linq2db/linq2db/blob/master/Source/LinqToDB/SqlQuery/SqlPredicate.cs) на оригинальный файл, обратите внимание на строчки с 245 по 284\. [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'newElement' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1320, 1315\. ConvertVisitor\.cs 1320 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'Stop' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 25, 24\. TransformInfo\.cs 25 ```cpp public TransformInfo(Expression expression, bool stop, bool @continue) { Expression = expression; Stop = false; Stop = stop; Continue = @continue; } ``` Теперь ситуация иная\. Здесь переменной _Stop_ сначала присваивается значение _false_ и сразу следующей строкой ей присваивается значение параметра _stop_\. Логично предположить, что в этом случае требуется убрать первое присвоение, раз оно не используется и мгновенно переписывается значением аргумента\. ## Куда убежала переменная? [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'ToDictionary' is required to be utilized\. ReflectionExtensions\.cs 34 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'AppendFormat' function\. Arguments not used: 1st\. ExpressionTestGenerator\.cs 663 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3137/) The 'version' variable is assigned but is not used by the end of the function\. Query\.cs 408 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3020/) An unconditional 'return' within a loop\. QueryRunner\.cs 751 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'version \> 15' is always true\. SqlServerTools\.cs 250 ```cpp 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_\. Исправленный код может выглядеть так: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) в этом проекте\. [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'table \=\= null' is always true\. LoadWithBuilder\.cs 113 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) выдала ещё пару десятков хороших предупреждений\. Не будем приводить их все в статье, но рекомендуем авторам самостоятельно проверить проект и посмотреть все предупреждения PVS\-Studio\. [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: field\.Field\.CreateFormat \!\= null\. BasicSqlBuilder\.cs 1255 ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'rows' is always not null\. The operator '?\.' is excessive\. SQLiteSqlBuilder\.cs 214 ```cpp 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_\. Вот фрагмент этой функции: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the '\_update' object SqlUpdateStatement\.cs 60 ```cpp 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](https://pvs-studio.ru/ru/pvs-studio/try-free/) на своём проекте\.