Проверяем качество кода в проектах .NET Foundation: LINQ to DB

.NET Foundation – независимая организация, основанная Microsoft с целью поддержки open source проектов на платформе DotNet. Под их крылом на данный момент собралось множество библиотек, некоторые из которых уже проходили проверку анализатором PVS-Studio. Следующим проектом для проверки анализатором будет LINQ to DB.

Введение

LINQ to DB ^[1] – фреймворк для работы с базами данных, основанный на LINQ. Он собрал в себе лучшее от предшественников, позволяя работать с различными СУБД, тогда как LINQ to SQL в своё время позволял работать только с MS SQL. Являясь более легковесным и простым, чем LINQ to SQL или Entity Framework, LINQ to DB предоставляет большой контроль и быстрый доступ к данным. Фреймворк небольшой, написан на языке C# и насчитывает более 40 000 строк кода.

LINQ to DB также входит в список проектов .NET Foundation. Мы уже ранее проверяли проекты этой организации: Windows Forms ^[2], Xamarin.Forms ^[3], Teleric UI for UWP ^[4] и др.

Меньше слов – больше дела. Проверим же код LINQ to DB, взятый с официального репозитория на GitHub ^[5], с помощью нашего статического анализатора PVS-Studio ^[6] и посмотрим, всё ли хорошо у наследника LINQ.

Deja Vu

Начнём, пожалуй, с самых распространённых случаев, встречавшихся каждому разработчику хоть раз: повторяющийся код.

V3001 ^[7] 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 ^[8] 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 ^[9] Expression 'Operator == Operator.NotEqual' is always false. SqlPredicate.cs 273.

В примере у нас повторяется условие Operator == Operator.NotEqual. Эти две ветки условий выполняют немного отличные операции. Из этого возникает вопрос – а какая из веток действительно требуется по мнению разработчика? После небольшого анализа функции Reduce я предположу, что скорее всего разработчикам нужна именно первая ветка со сравнением с Operator.NotEqual. Её функционал более схож с ветками Equal и LessOrEqual. В отличие от двойника, вторая ветка с NotEqual имеет абсолютно идентичный функционал с веткой else. Вот вам для сравнения ссылка ^[10] на оригинальный файл, обратите внимание на строчки с 245 по 284.

V3008 ^[11] 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 ^[11] 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 ^[12] 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 ^[13] 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("rn", ctors),
    baseClasses.Length == 0 ? "" 
                            : " : " + GetTypeNames(baseClasses),
    type.IsPublic ? "public " 
                  : "",
    type.IsAbstract && !type.IsInterface ? "abstract " 
                                         : "",
    attr,
    members.Length > 0 ? (ctors.Count != 0 ? "rn" : "") + 
                         string.Join("rn", members) 
                       : string.Empty);
}

В данном фрагменте мы видим форматирование строки. Встает вопрос – а куда подевалось упоминание первого аргумента? В первой форматируемой строке у нас использованы индексы от 1 до 9. Но либо разработчику не потребовался аргумент с индексом 0, либо он про него забыл.

V3137 ^[14] 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 ^[15] 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 ^[9] 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 ^[9] в этом проекте.

V3022 ^[9] 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 ^[9] выдала ещё пару десятков хороших предупреждений. Не будем приводить их все в статье, но рекомендуем авторам самостоятельно проверить проект и посмотреть все предупреждения PVS-Studio.

V3063 ^[16] 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 ^[9] 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 ^[17] 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 ^[18] на своём проекте.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Danila Karpov. PVS-Studio checks the code quality in the .NET Foundation projects: LINQ to DB ^[19].