# PHP – компилируемый язык?\! PVS\-Studio ищет ошибки в PeachPie

PHP широко известен как интерпретируемый язык программирования, использующийся в основном для разработки сайтов\. Однако немногие знают, что для PHP есть ещё и компилятор под \.NET \- PeachPie\. Но вот насколько он качественно сделан? Сможет ли статический анализатор найти в этом компиляторе реальные ошибки? Давайте же узнаем\!

![0855_Peachpie_check_ru/image1.png](https://import.viva64.com/docx/blog/0855_Peachpie_check_ru/image1.png)

Давненько не выходили статьи о проверках C\#\-проектов с помощью PVS\-Studio\.\.\. А ведь нам ещё составлять топ ошибок за 2021 год \(топ за 2020 год, кстати, можно глянуть [тут](https://pvs-studio.ru/ru/blog/posts/csharp/0787/)\)\! Что ж, нужно срочно исправляться\. Я рад представить вам обзор результатов проверки PeachPie\.

![0855_Peachpie_check_ru/image2.png](https://import.viva64.com/docx/blog/0855_Peachpie_check_ru/image2.png)

Для начала немного расскажу о проверяемом проекте\. PeachPie — это современный компилятор языка PHP с открытым исходным кодом и среда выполнения для \.NET Framework и \.NET\. Он построен на платформе компилятора Microsoft Roslyn и основан на проекте [Phalanger](https://github.com/DEVSENSE/Phalanger) первого поколения\. С июля 2017 года данный проект также входит в [\.NET Foundation](https://dotnetfoundation.org/)\. Исходный код доступен в [репозитории на GitHub](https://github.com/peachpiecompiler/peachpie)\.

Кстати, наш C\#\-анализатор тоже широко использует возможности [Roslyn](https://github.com/dotnet/roslyn), так что можно сказать, что у PeachPie и PVS\-Studio есть что\-то общее :\)\. Вообще основам работы с Roslyn посвящена целая [статья](https://pvs-studio.ru/ru/blog/posts/csharp/0399/), написанная как раз на основе нашего опыта работы с этой платформой\. 

Для проведения же проверки PeachPie нужно было установить анализатор, открыть проект в Visual Studio или Rider и запустить анализ, используя плагин PVS\-Studio\. Подробнее об этом написано в [документации](https://pvs-studio.ru/ru/docs/)\. 

Проверять такой большой и серьёзный проект было очень интересно\. Надеюсь, что мой обзор найденных в PeachPie ошибок вам покажется не менее интересным\. Приятного чтения\!

## Проблемы с WriteLine

Начнём, пожалуй, с простого :\) Порой ошибки могут возникать в самых неожиданных и при этом самых простых местах\. Например, ошибка может оказаться даже в банальном вызове функции _WriteLine_:

```cpp
public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;
  
  ....
}
```

Предупреждение [V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/): Incorrect format\. A different number of format items is expected while calling 'WriteLine' function\. Arguments not used: 1st, 2nd, 3rd, 4th, 5th\. Mail\.cs 25

Казалось бы, а что же тут не так? Вроде бы всё выглядит нормально\. Хотя, постойте\-ка\! А каким по порядку аргументом нужно передавать формат?

Что ж, давайте взглянем на объявление _Debug\.WriteLine_:

```cpp
public static void WriteLine(string format, params object[] args);
```

Получается, что строка формата должна передаваться первым аргументом, а в коде первым аргументом является _"MAILER"_\. Очевидно, разработчик перепутал методы и передал аргументы некорректным образом\.

## Одинаковые case в switch

Очередной раздел посвящается срабатываниям, связанным с выполнением одинаковых действий в разных case\-ветках:

```cpp
private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.DisallowNullAttribute":
        result |= FlowAnalysisAnnotations.DisallowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullAttribute":
        result |= FlowAnalysisAnnotations.MaybeNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute":
        if (TryGetBoolArgument(attr, out bool maybeNullWhen))
        {
          result |= maybeNullWhen ? FlowAnalysisAnnotations.MaybeNullWhenTrue
                                  : FlowAnalysisAnnotations.MaybeNullWhenFalse;
        }
        break;
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
    }
  }
}
```

В данном фрагменте закралась если не ошибка, то как минимум странность\. Как быстро вы сможете её найти?

Впрочем, ни к чему терять время, анализатор всё нашёл за нас:

```cpp
private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      ....
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;              // <=
        break;
    }
  }
}
```

Предупреждение [V3139](https://pvs-studio.ru/ru/docs/warnings/v3139/): Two or more case\-branches perform the same actions\. ReflectionUtils\.Nullability\.cs 170

Не правда ли, странно, что два разных случая обрабатываются одинаково? На самом деле нет, в принципе такое бывает достаточно часто\. Однако есть 2 "но"\.

В первую очередь стоит отметить, что существует более элегантный способ одинаково обработать два различных случая\. Рассмотренный фрагмент можно было бы записать как\-то так:

```cpp
switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
}
```

Тем не менее, разработчики нередко пренебрегают этим удобным способом, предпочитая копипаст\. Из\-за этого наличие двух одинаковых веток само по себе не кажется таким уж страшным\. Куда более подозрительным выглядит тот факт, что перечисление _FlowAnalysisAnnotations_ имеет среди прочих значение _FlowAnalysisAnnotations\.NotNull_\. Складывается впечатление, что именно оно должно было использоваться при обработке значения _"System\.Diagnostics\.CodeAnalysis\.NotNullAttribute"_:

```cpp
switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.NotNull;              // <=
    break;
}
```

## Иммутабельный DateTime

Разработчики [частенько допускают ошибки](https://pvs-studio.ru/ru/blog/examples/v3010/), связанные с неверным пониманием особенностей работы "изменяющих" методов\. Ниже же представлена ошибка, найденная в PeachPie:

```cpp
using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}
```

Предупреждения PVS\-Studio:

* [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'AddHours' is required to be utilized\. DateTimeFunctions\.cs 1232
* [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'AddHours' is required to be utilized\. DateTimeFunctions\.cs 1239

Анализатор сообщает, что результаты вызовов должны куда\-нибудь записываться – в противном случае они попросту не имеют никакого смысла\. Дело в том, что методы вроде _AddHours_ не меняют исходный объект – вместо этого возвращается новый, отличающийся от исходного соответствующим образом\. Сложно сказать, насколько критична эта оплошность, но очевидно, что код работает некорректно\.

## Try\-методы с нюансами

Try\-методы очень часто бывают удобны при разработке приложений на C\#\. Наиболее известными их представителями являются _int\.TryParse_, _Dictionary\.TryGetValue_ и т\.д\. Привычно, что эти методы возвращают флаг, указывающий на успешность выполнения операции, а сам результат записывается в out\-параметр\. Разработчики PeachPie решили реализовать свои try\-методы, которые, казалось бы, должны были работать по той же схеме\. Что же из этого вышло? Давайте взглянем на следующий код:

```cpp
internal static bool TryParseIso8601Duration(string str,
                                             out DateInfo result,
                                             out bool negative)
{
  ....
  if (pos >= length) goto InvalidFormat;

  if (s[pos++] != 'P') goto InvalidFormat;

  if (!Core.Convert.TryParseDigits(....))
    goto Error;
  
  if (pos >= length) goto InvalidFormat;

  if (s[pos] == 'Y')
  {
    ....

    if (!Core.Convert.TryParseDigits(....)) 
      goto Error;

    if (pos >= length) goto InvalidFormat;
  }
  ....
  InvalidFormat:
  Error:

    result = default;
    negative = default;
    return false;
}
```

Данный метод сильно сокращён для удобства восприятия\. При желании вы можете посмотреть его полностью, перейдя по [ссылке](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/DateTime/DateTimeParsing.cs)\. В методе множество раз производится вызов _Core\.Convert\.TryParseDigits_\. В случаях когда такой вызов возвращает _false_, поток выполнения переходит к метке _Error_, что вполне логично\.

На метке _Error_ _out_\-параметрам присваиваются значения по умолчанию, после чего метод возвращает _false_\. Всё выглядит вполне логично – метод _TryParseIso8601Duration_ ведёт себя именно так, как и стандартные try\-методы\. Ну\.\.\. По крайней мере, это так выглядит\. Фактически же всё совсем не так :\(\.

Как уже было сказано ранее, если _Core\.Convert\.TryParseDigits _возвращает _false_, то код переходит на метку _Error_, где и производится обработка ошибки/неудачи\. Правда, вот в чём беда – анализатор\-то сообщает, что _TryParseDigits_ никогда не возвращает _false_:

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression '\!Core\.Convert\.TryParseDigits\(s, ref pos, false, out value, out numDigits\)' is always false\. DateTimeParsing\.cs 1440

Если отрицание результата вызова всегда равно _false_, то сам вызов всегда возвращает _true_\. Достаточно специфичное поведение для try\-метода\! Неужели операция и правда всегда завершается успешно? Давайте, наконец, взглянем на _TryParseDigits_:

```cpp
public static bool TryParseDigits(....)
{
  Debug.Assert(offset >= 0);

  int offsetStart = offset;
  result = 0;
  numDigits = 0;

  while (....)
  {
    var digit = s[offset] - '0';

    if (result > (int.MaxValue - digit) / 10)
    {
      if (!eatDigits)
      {
        // overflow
        //return false;
        throw new OverflowException();
      }

      ....

      return true;
    }

    result = result * 10 + digit;
    offset++;
  }

  numDigits = offset - offsetStart;
  return true;
}
```

Метод действительно всегда возвращает _true_\. Но операция может и завершиться неудачей – в этом случае будет выброшено исключение типа _OverflowException_\. Как по мне, это явно не то поведение, которого ожидаешь от try\-метода :\)\. Строка с _return false_, кстати, тут есть, но она закомментирована\.

Возможно, использование в этом месте исключения как\-то обосновано\. Но по коду складывается впечатление, что тут что\-то пошло не по плану\. И _TryParseDigits_, и использующий его _TryParseIso8601Duration,_ по идее, должны работать как привычные try\-методы – возвращать в случае неудачи _false_\. Вместо этого они выбрасывают неожиданные исключения\.

## Значение аргумента по умолчанию

Следующее срабатывание анализатора попроще, однако и оно указывает на весьма странный фрагмент кода:

```cpp
private static bool Put(Context context,
                        PhpResource ftp_stream,
                        string remote_file,
                        string local_file,
                        int mode,
                        bool append,
                        int startpos)
{ .... }

public static bool ftp_put(Context context,
                           PhpResource ftp_stream,
                           string remote_file,
                           string local_file,
                           int mode = FTP_IMAGE,
                           int startpos = 0)
{
    return Put(context,
               ftp_stream,
               remote_file,
               local_file,
               mode = FTP_IMAGE, // <=
               false,
               startpos);
}
```

Предупреждение [V3061](https://pvs-studio.ru/ru/docs/warnings/v3061/): Parameter 'mode' is always rewritten in method body before being used\. Ftp\.cs 306

Метод _ftp\_put_ принимает на вход ряд параметров, одним из которых является _mode_\. Данный параметр имеет значение по умолчанию, однако при вызове, очевидно, можно задать и другое значение\. Правда, это ни на что не повлияет – _mode_ всегда перезаписывается, и в метод _Put_ всегда передаётся значение константы _FTP\_IMAGE_\.

Сложно сказать, почему всё написано именно так – конструкция кажется бессмысленной\. Скорее всего, здесь всё же допущена ошибка\.

## Copy\-paste передаёт привет

Следующий фрагмент кода похож на жертву копирования:

```cpp
public static PhpValue filter_var(....)
{
  ....
  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_PRIV_RANGE)
  {
    throw new NotImplementedException();
  }

  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_RES_RANGE)
  {
    throw new NotImplementedException();
  }
  ....
}
```

Предупреждение [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/): Two similar code fragments were found\. Perhaps, this is a typo and 'NO\_RES\_RANGE' variable should be used instead of 'NO\_PRIV\_RANGE' Filter\.cs 771

Складывается впечатление, что второе условие нужно было записать так:

_\(flags & \(int\)FilterFlag\.**NO\_RES\_RANGE**\) \=\= \(int\)FilterFlag\.NO\_RES\_RANGE_

Во всяком случае, такой вариант выглядит логичнее и не вызывает вопросов\.

## Просто лишняя проверка в if

Думаю, можно разбавить наш сегодняшний разбор и обычным избыточным кодом:

```cpp
internal static NumberInfo IsNumber(....)
{
  ....
  int num = AlphaNumericToDigit(c);

  // unexpected character:
  if (num <= 15)
  {
    if (l == -1)
    {
      if (   longValue < long.MaxValue / 16 
          || (   longValue == long.MaxValue / 16 
              && num <= long.MaxValue % 16))         // <=
      {
        ....
      }
      ....
    }
    ....
  }
  ....
}
```

Предупреждение [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/): A part of conditional expression is always true if it is evaluated: num <\= long\.MaxValue % 16\. Conversions\.cs 994

В первую очередь хочется сказать, что код функции очень сильно урезан для удобства восприятия\. Полностью исходный код _IsNumber_ доступен по [ссылке](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Runtime/Conversions.cs) – но хочу предупредить, что изучать её будет непросто, ибо функция содержит более 300 строк кода\. Кажется, что она слегка выходит за принятые рамки "одного экрана" :\)\.

Перейдём к срабатыванию\. Во внешнем блоке производится проверка, что значение переменной _num_ меньше или равно 15\. Во внутреннем же есть проверка, что _num_ меньше или равно _long\.MaxValue % 16_\. При этом значение этого выражения_ _равно 15 – это нетрудно проверить\. Выходит, код дважды проверяет, что _num_ меньше или равен 15\.

Едва ли это срабатывание указывает на реальную ошибку – просто кто\-то написал лишнюю проверку\. Возможно, на то даже были причины – например, разработчику было проще читать именно такой код\. Хотя кажется, что использование какой\-нибудь переменной или константы для хранения результата сравнения было бы более простым вариантом\. Как бы там ни было, конструкция избыточна, и долг статического анализатора – об этом сообщить\.

## Так может ли там быть null?

Довольно часто разработчики упускают проверки на _null_\. Особенно интересна ситуация, когда в одном месте функции переменную проверили, а в другом \(где она всё так же может быть _null_\) – забыли или не посчитали нужным\. И тут остаётся лишь гадать, была ли проверка лишней или напротив – кое\-где её не хватает\. Проверки на _null_ не всегда предполагают использование операторов сравнения – например, в коде ниже разработчик использовал [null\- conditional оператор](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators):

```cpp
public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}
```

Предупреждение [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/): The 'tinfo' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. Objects\.cs 189

По мнению разработчика, вызов _Type\.GetTypeFromHandle\(caller\)_ может вернуть _null_ – оттого он и использовал "?\." для вызова _GetPhpTypeInfo_\. Судя по [документации](https://docs.microsoft.com/en-us/dotnet/api/system.type.gettypefromhandle?view=net-5.0), это действительно возможно\. 

Ура, "?\." спасает от одного исключения\. Если вызов _GetTypeFromHandle_ действительно вернёт _null_, то в переменную _tinfo_ также будет записан _null_\. Однако при попытке обращения к свойству _BaseType_ будет выброшено другое исключение\. Скорее всего, в последней строке не хватает ещё одного "?":

_return tinfo**?**\.BaseType?\.Name;_

## Fatal warning и exceptions

_Приготовьтесь, в этом разделе вас ждёт настоящее расследование\.\.\._

Ещё одно срабатывание, связанное с проверкой на _null_, оказалось куда интереснее, чем выглядело на первый взгляд\. Взгляните на код:

```cpp
static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context));
  }

  return context.HashAlgorithm;
}
```

Предупреждение [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/): The 'context' object was used after it was verified against null\. Check lines: 3138, 3133\. Hash\.cs 3138

Действительно, переменная проверяется на равенство _null_, а затем без какой\-либо проверки производится обращение к свойству\. Однако посмотрите, что произойдёт, если переменная будет равна _null_:

```cpp
PhpException.ArgumentNull(nameof(context));
```

Получается, что если _context _и правда будет равен _null_, то до обращения к свойству _HashAlgorithm_ поток выполнения не доберётся\. Следовательно, данный код безопасен\. Получается, ложное срабатывание?

Конечно, анализатор может и ошибаться\. Однако мне было известно, что PVS\-Studio подобные ситуации обрабатывать умеет – анализатор должен был знать, что в момент обращения к _HashAlgorithm_ переменная _context_ не может быть равна _null_\.

А всё\-таки что именно делает вызов _PhpException\.ArgumentNull_? Давайте взглянем:

```cpp
public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}
```

Хм, действительно, вроде что\-то выбрасывается\. Обратите внимание на первый аргумент вызова — _PhpError\.Warning_\. Хм, ну что ж, перейдём к самому методу _Throw_:

```cpp
public static void Throw(PhpError error, string formatString, string arg0)
{
  Throw(error, string.Format(formatString, arg0));
}
```

Тут в принципе ничего интересного, перейдём в другую перегрузку _Throw_:

```cpp
public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}
```

А вот и то, что мы искали\! Получается, что под капотом всей этой системы производится выбрасывание _PhpFatalErrorException_\. Вот только похоже, что выбрасывается исключение не всегда\. 

В первую очередь стоит поглядеть на места, где регистрируются обработчики события _OnError_\. Всё же они тоже могут кидать исключения – это было бы слегка неожиданно, но мало ли\. Таких оказалось немного и все они связаны с логированием соответствующих сообщений\. Один был в файле [PhpHandlerMiddleware](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.AspNetCore.Web/PhpHandlerMiddleware.cs):

```cpp
PhpException.OnError += (error, message) =>
{
  switch (error)
  {
    case PhpError.Error:
      logger.LogError(message);
      break;

    case PhpError.Warning:
      logger.LogWarning(message);
      break;

    case PhpError.Notice:
    default:
      logger.LogInformation(message);
      break;
  }
};
```

Другие 2 определялись внутри самого класса [PhpException](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Runtime/Errors.cs):

```cpp
// trace output
OnError += (error, message) =>
{
  Trace.WriteLine(message, $"PHP ({error})");
};

// LogEventSource
OnError += (error, message) =>
{
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    LogEventSource.Log.HandleFatal(message);
  }
  else
  {
    LogEventSource.Log.HandleWarning(message);
  }
};
```

Таким образом, никаких исключений обработчики события не генерируют\. Поэтому вернёмся к методу _Throw_\.

```cpp
public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}
```

Раз с _OnError_ всё ясно, то давайте подробнее разберём это условие:

```cpp
(error & (PhpError)PhpErrorSets.Fatal) != 0
```

Параметр_ error_ хранит значение перечисления _PhpError_\. Чуть ранее мы видели, что в этот параметр передаётся _PhpError\.Warning_\. Исключение же будет выброшено в случае, если результат применения "побитового и" к _error _и _PhpErrorSets\.Fatal_ не будет нулевым\.

Значение_ PhpErrorSets\.Fatal_ представляет собой "объединение" набора элементов перечисления _PhpError_ с помощью операции "побитового или":

```cpp
Fatal =   PhpError.E_ERROR | PhpError.E_COMPILE_ERROR
        | PhpError.E_CORE_ERROR | PhpError.E_USER_ERROR
```

Ниже представлены значения всех рассмотренных ранее элементов перечисления:

```cpp
E_ERROR = 1,
E_WARNING = 2,
E_CORE_ERROR = 16,
E_COMPILE_ERROR = 64,
E_USER_ERROR = 256,
Warning = E_WARNING
```

Получается, операция _error & \(PhpError\)PhpErrorSets\.Fatal_ вернёт ненулевое значение только в случае, если параметр _error_ будет иметь одно из следующих значений или их сочетаний:

```cpp
PhpError.E_ERROR,
PhpError.E_COMPILE_ERROR,
PhpError.E_CORE_ERROR,
PhpError.E_USER_ERROR
```

Если в параметр _error_ записано значение _PhpError\.Warning_, равное _PhpError\.E\_WARNING_, то результат операции "побитового и" будет нулевым\. Тогда и условие выбрасывания исключения _PhpFatalErrorException_ не будет выполнено\.

Вернёмся назад к методу _PhpException\.ArgumentNull_:

```cpp
public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}
```

Мы разобрались, что при передаче _PhpError\.Warning_ исключение выброшено не будет\. Возможно, по задумке исключение действительно не должно выбрасываться в случаях, когда передан неожиданный _null_\. Вот только\.\.\.

```cpp
static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context)); // no exceptions
  }

  return context.HashAlgorithm; // context is potential null
}
```

Если _PhpException\.ArgumentNull_ не выбрасывает исключение \(что само по себе уже очень неожиданно\), то при обращении к свойству _HashAlgorithm_ всё равно будет выбрасываться _NullReferenceException_\!

Возникает вопрос – должно ли всё\-таки выбрасываться исключение или нет? Если должно, то логичнее было бы использовать тот же _PhpFatalErrorException_\. Если же исключения в этой ситуации никто не ожидает, то нужно корректно обработать значение _null_ параметра _context_\. К примеру, использовать "?\."\. Так или иначе, анализатор всё\-таки отработал корректно и даже помог разобраться в этой странной ситуации\.

## Снова лишняя проверка? И снова exception\!

Прошлый случай показывает, как можно ожидать исключение, а нарваться на неожиданный _null_\. Фрагмент ниже – противоположность того случая:

```cpp
public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    return node.Value;
  else
    return PhpValue.Null;
}
```

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression 'node \!\= null' is always true\. Datastructures\.cs 432

Ну, как говорится, не _null_ и ладно – чего бухтеть\-то? Вот только обычно _null_ ожидается в тех случаях, когда что\-то пошло не так\. По коду видно, что здесь именно такая ситуация\. Анализатор же утверждает, что никакого _null_ тут быть не может\.

Можно подумать, что здесь дело в вызове _Debug\.Assert_\. Хорошо это или плохо, но на самом деле этот вызов не влияет на мнение анализатора\.

Если дело не в _Debug\.Assert_, то в чём же? С чего анализатор считает, что _node_ никогда не равно _null_? Давайте взглянем на метод _GetNodeAtIndex_, который и возвращает значение, записанное в _node_:

```cpp
private LinkedListNode<PhpValue> GetNodeAtIndex(PhpValue index)
{
  return GetNodeAtIndex(GetValidIndex(index));
}
```

Что же, копаем дальше\. Поглядим на вызванный здесь _GetNodeAtIndex_:

```cpp
private LinkedListNode<PhpValue> GetNodeAtIndex(long index)
{
  var node = _baseList.First;
  while (index-- > 0 && node != null)
  {
    node = node.Next;
  }

  return node ?? throw new OutOfRangeException();
}
```

Глядите – похоже, метод действительно мог бы вернуть _null_\.\.\. Но не тут\-то было – если после завершения работы цикла _node_ будет равно _null_, то производится выбрасывание исключения\. Таким образом, никакого _null_ возвращено быть не может\.

Получается, что в случае возникновения непредвиденной ситуации метод _GetNodeAtIndex_ не вернёт _null_, как ожидалось в коде метода _offsetGet_:

```cpp
public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset); // potential null expected

  Debug.Assert(node != null);

  if (node != null) // always true
    return node.Value;
  else
    return PhpValue.Null; // unreachable
}
```

При просмотре этого метода любой разработчик может легко обмануться\. Ведь по коду складывается впечатление, что либо будет возвращено корректное значение, либо _PhpValue\.Null_\. Фактически же этот метод может выбросить исключение\.

Такая вот беда — неожиданное поведение лишь одного метода в цепочке вызовов приводит к неожиданному поведению всех этих методов\. Данный пример хорошо иллюстрирует полезность статического анализа, позволяющего находить такие проблемы автоматически\.

Аналогичная проблема присутствует, кстати, и в методе _offsetSet_ из того же класса:

```cpp
public void offsetSet(PhpValue offset, PhpValue value)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    node.Value = value;
}
```

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression 'node \!\= null' is always true\. Datastructures\.cs 444

## Присваивания и переприсваивания

Предлагаю немного отдохнуть от всех этих расследований и выпить кружечку кофе\.

![0855_Peachpie_check_ru/image3.png](https://import.viva64.com/docx/blog/0855_Peachpie_check_ru/image3.png)

Пока пьём кофе, рассмотрим более простое срабатывание, которое тем не менее указывает на очень странный фрагмент кода:

```cpp
internal StatStruct(Mono.Unix.Native.Stat stat)
{
  st_dev = (uint)stat.st_dev;
  st_ctime = stat.st_ctime_nsec;
  st_mtime = stat.st_mtime_nsec;
  st_atime = stat.st_atime_nsec;
  st_ctime = stat.st_ctime;
  st_atime = stat.st_atime;
  //stat.st_blocks;
  //stat.st_blksize;
  st_mtime = stat.st_mtime;
  st_rdev = (uint)stat.st_rdev;
  st_gid = (short)stat.st_gid;
  st_uid = (short)stat.st_uid;
  st_nlink = (short)stat.st_nlink;
  st_mode = (FileModeFlags)stat.st_mode;
  st_ino = (ushort)stat.st_ino;
  st_size = stat.st_size;
}
```

Предупреждения PVS\-Studio:

* [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'st\_ctime' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 78, 75\. StatStruct\.cs 78
* [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'st\_atime' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 79, 77\. StatStruct\.cs 79

Выглядит так, будто разработчик запутался во всех этих присваиваниях и где\-то опечатался\. Это привело к тому, что значения присваиваются полям _st\_ctime_ и _st\_atime_ по 2 раза – и ведь второй раз присваивается не то же самое значение, что в первый\.

Вроде бы ошибка, верно? Но так ведь совсем не интересно\! Предлагаю вам потренировать свои навыки поиска глубинного смысла и в комментариях предложить какое\-нибудь объяснение того, зачем всё написано именно таким образом\.

Ну а пока идём дальше :\)

## Эти неизменяемые строки\.\.\. не изменить

Давным\-давно, когда вы ещё только просматривали первые срабатывания из этой статьи, мы упоминали неизменяемость экземпляров структуры _DateTime_\. Следующие же предупреждения напомнят нам об аналогичном свойстве строк:

```cpp
public TextElement Filter(IEncodingProvider enc,
                          TextElement input,
                          bool closing)
{
  string str = input.AsText(enc.StringEncoding);

  if (pending)
  {
    if (str.Length == 0) str = "\r";
    else if (str[0] != '\n') str.Insert(0, "\r"); // <=
  }

  str = str.Replace("\r\n", "\n");
  if (str.Length != 0)
  {
    pending = str[str.Length - 1] == '\r';

    if (!closing && pending) str.Remove(str.Length - 1, 1); // <=
  }

    
  return new TextElement(str);
}
```

Предупреждения PVS\-Studio:

* [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Insert' is required to be utilized\. Filters\.cs 150
* [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Remove' is required to be utilized\. Filters\.cs 161

В принципе, всё просто и понятно – хотели внести в строку изменения, но как\-то\.\.\. не срослось :\(\.

## or throw \!\= or null

Недавно мы разбирали случай, когда разработчик ожидал от функции _null_, а получил исключение\. Ниже представлено нечто похожее, но попроще:

```cpp
public static bool stream_wrapper_register(....)
{
  // check if the scheme is already registered:
  if (   string.IsNullOrEmpty(protocol)
      || StreamWrapper.GetWrapperInternal(ctx, protocol) == null)
  {
    // TODO: Warning?
    return false;
  }

  var wrapperClass = ctx.GetDeclaredTypeOrThrow(classname, true);
  if (wrapperClass == null) // <=
  {
    return false;
  }

  ....
}
```

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression 'wrapperClass \=\= null' is always false\. Streams\.cs 555

Конечно, можно провести подробный разбор, но\.\.\. Тут и по названию метода ведь всё видно\! _GetDeclaredTypeOrThrow_ как бы намекает, что если что не так, то он будет стрелять\. И ведь смотрите опять какая штука – это поведение будет передаваться и методу _stream\_wrapper\_register_, который по задумке должен был бы просто вернуть _false_\. Ага, конечно, ловите исключение\!

Вообще, мы уже сталкивались ранее с обманывающими названиями\. Помните, как вызов метода _PhpException\.ArgumentNull_ на самом деле не бросал исключение? Поэтому давайте всё\-таки проверим, действительно ли _GetDeclaredTypeOrThrow_ бросает исключение:

```cpp
PhpTypeInfo GetDeclaredTypeOrThrow(string name, bool autoload = false)
{
  return GetDeclaredType(name, autoload) ??
         throw PhpException.ClassNotFoundException(name);
}
```

Ну, здесь не обманули – и правда исключение :\)\.

## Странный while true

Бывают случаи, когда разработчики используют в качестве условия продолжения цикла _while_ значение _true_\. Кажется, что это в целом нормальная практика – для выхода из цикла может использоваться _break, return_, ну или те же исключения\. Куда более странно выглядит цикл, в качестве условия которого используется не ключевое слово _true_, а некоторое выражение, значение которого всегда имеет значение _true_:

```cpp
public static int stream_copy_to_stream(...., int offset = 0)
{
  ....
  if (offset > 0)
  {
    int haveskipped = 0;

    while (haveskipped != offset)  // <=
    {
      TextElement data;

      int toskip = offset - haveskipped;
      if (toskip > from.GetNextDataLength())
      {
        data = from.ReadMaximumData();
        if (data.IsNull) break;
      }
      else
      {
        data = from.ReadData(toskip, false);
        if (data.IsNull) break; // EOF or error.
        Debug.Assert(data.Length <= toskip);
      }

      Debug.Assert(haveskipped <= offset);
    }
  }
  ....
}
```

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression 'haveskipped \!\= offset' is always true\. Streams\.cs 769

Переменная _haveskipped_ объявлена перед запуском цикла и инициализирована значением 0\. Это значение останется с ней\.\.\. до самой смерти\. Мрачновато вышло, но как есть\. По сути, _haveskipped_ — это константа\. Значение параметра _offset_ во время выполнения цикла также не меняется\. Да и вообще не меняется ни в одном месте функции, на самом деле \(можете проверить [тут](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/Streams/Streams.cs)\)\.

Задумывал ли разработчик, что условие продолжения цикла будет всегда истинным? В теории это возможно, конечно\. Но взгляните на цикл повнимательнее\. Следующее присваивание в нём выглядит странно:

```cpp
int toskip = offset - haveskipped;
```

Какой в этом смысл, если _haveskipped_ всегда равен 0?

С этим циклом явно что\-то не то\. То ли тут действительно допущена серьёзная ошибка, то ли все эти странности с _haveskipped_ – останки каких\-то старых нереализованных идей\.

## data \=\= null && throw NullReferenceException

Довольно часто ошибки связаны с использованием некорректных операторов в условиях\. Нашлось подобное и в компиляторе PHP:

```cpp
public string ReadStringContents(int maxLength)
{
  if (!CanRead) return null;
  var result = StringBuilderUtilities.Pool.Get();

  if (maxLength >= 0)
  {
    while (maxLength > 0 && !Eof)
    {
      string data = ReadString(maxLength);
      if (data == null && data.Length > 0) break; // EOF or error.
      maxLength -= data.Length;
      result.Append(data);
    }
  }
  ....
}
```

Предупреждение [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/): Possible null dereference\. Consider inspecting 'data'\. PhpStream\.cs 1382

В цикле производится проверка значения переменной _data_\. Если она равна _null_ и при этом её свойство _Length_ имеет положительное значение, то производится выход из цикла\. Очевидно, это невозможно\. Более того, обращение к свойству _Length_ переменной, имеющей значение _null_, приведёт к выбрасыванию исключения\. Здесь же обращение подчёркнуто производится именно тогда, когда _data \= null_\.

Учитывая комментарий разработчика, я бы переписал условие как\-то так:

```cpp
data == null || data.Length == 0
```

Тем не менее, не факт, что это действительно корректный вариант обработки – для внесения исправления стоит провести более глубокое исследование кода\.

## Не то исключение

Бывают и ошибки, которые в принципе не выглядят особенно страшными, но всё же могут доставить проблем\. Например, в следующем фрагменте copy\-paste нанёс ещё один удар:

```cpp
public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}
```

Предупреждение [V3013](https://pvs-studio.ru/ru/docs/warnings/v3013/): It is odd that the body of 'addGlob' function is fully equivalent to the body of 'addPattern' function \(506, line 515\)\. ZipArchive\.cs 506

Функция _addGlob_, очевидно, не поддерживается, поэтому при её вызове выбрасывается исключение, сообщающее о том, что _addGlob_ не поддерживается\.

Поверили? А зря\! Никакого исключения тут не будет – это ж наш старый знакомый _PhpException_:

```cpp
public static class PhpException
{
  ....
  public static void FunctionNotSupported(string/*!*/function)
  {
    Debug.Assert(!string.IsNullOrEmpty(function));

    Throw(PhpError.Warning,
          ErrResources.notsupported_function_called,
          function);
  }
  ....
}
```

Как мы разбирали ранее, если в метод _Throw_ передаётся значение _PhpError\.Warning_, то исключения не будет\. Но всё же возникшая ошибка наверняка будет записана в какой\-нибудь лог или обработана как\-то ещё\.

Вернёмся к исходному фрагменту кода:

```cpp
public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}
```

_addGlob_ не поддерживается и при вызове соответствующее сообщение будет как\-то обработано – будем считать, что оно запишется в лог\. _addPattern_ тоже не поддерживается, правда, вот соответствующее сообщение будет всё равно посвящено _addGlob_\.

Очевидно, проблема возникла из\-за копирования\. Исправить легко – просто в методе _addPattern_ нужно сообщать про _addPattern_, а не про _addGlob_:

```cpp
public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addPattern));
  return false;
}
```

## String\.Join ни в чём не виноват\!

Иногда разработчики забывают об особенностях работы некоторых функций, из\-за чего проверяют совсем не те значения, которые нужно\. Из\-за этого проверка оказывается бессмысленной, а в правильном месте её и вовсе нет\. Похоже, примерно это и случилось с функцией _getallheaders_:

```cpp
public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    if (headers != null)
    {
      var result = new PhpArray(16);

      foreach (var h in headers)
      {
        result[h.Key] = string.Join(", ", h.Value) ?? string.Empty;
      }

      return result;
    }
  }

  return null;
}
```

Предупреждение [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/): Expression 'string\.Join\(", ", h\.Value\)' is always not null\. The operator '??' is excessive\. Web\.cs 932

Использование оператора "??" не имеет здесь никакого смысла, так как метод _string\.Join_ никогда не вернёт _null_\. А вот бросить _ArgumentNullException_ – это всегда пожалуйста\!

_string\.Join _выбрасывает исключение в том случае, если переданная ссылка на последовательность равна _null_\. Поэтому безопаснее будет записать эту строку как\-то так:

```cpp
result[h.Key] = h.Value != null ? string.Join(", ",h.Value) : string.Empty;
```

Вообще, я хотел узнать, может ли это _Value_ в принципе быть _null_\. А то, может, тут и вовсе не нужны никакие проверки\. Для этого нужно было понять, откуда пришла коллекция _headers_\.

```cpp
public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    ....
  }

  return null;
}
```

Значение _headers_ взято из _webctx\.RequestHeaders_, а значение _webctx_, в свою очередь, берётся из свойства _HttpPhpContext _объекта _ctx_\. Ну а свойство _HttpPhpContext_\.\.\. Что ж, глядите сами:

```cpp
partial class Context : IEncodingProvider
{
  ....
  public virtual IHttpPhpContext? HttpPhpContext => null;
  ....
}
```

Это, видимо, некий "на потом"\. Если взглянете на метод _getallheaders_ ещё разок, то увидите, что он, получается, вообще никогда не отрабатывает и просто возвращает _null_\.

Что, опять поверили? Ну что же с вами делать\! Свойство\-то виртуальное\. Следовательно, чтобы понять, что же фактически может быть им возвращено, надо изучать наследников\. Лично я решил на этом остановиться – всё же мне нужно показывать и другие срабатывания\.

## Маленькое присваивание в большом методе

Чем больше и сложнее метод, тем больше вероятность наличия в нём ошибки\. Разработчикам со временем становится сложно ориентироваться в большой куче кода, при этом всегда очень страшно что\-то в этом коде менять\. Новый код добавляется, старый остаётся прежним, кое\-как эта невероятная конструкция вроде работает, ну и слава богу\. Нет ничего удивительного в том, что в таком коде оказываются различные странности\. К примеру, давайте взглянем на метод _inflate\_fast_:

```cpp
internal int inflate_fast(....)
{
  ....
  int r;
  ....
  if (c > e)
  {
    // if source crosses,
    c -= e; // wrapped copy
    if (q - r > 0 && e > (q - r))
    {
      do
      {
        s.window[q++] = s.window[r++];
      }
      while (--e != 0);
    }
    else
    {
      Array.Copy(s.window, r, s.window, q, e);
      q += e; r += e; e = 0;                     // <=
    }
    r = 0;                                       // <=
  }
  ....
}
```

Предупреждение [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/): The 'r' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 621, 619\. InfCodes\.cs 621

Для начала вот [ссылка](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/Zlib/InfCodes.cs) на полный код\. Метод состоит более чем из двух сотен строк кода с кучей вложенных конструкций\. Кажется, что разобраться в нём явно было бы непросто\.

Срабатывание довольно однозначное – сначала в блоке переменной _r_ присваивается новое значение, а затем оно безусловно перезаписывается нулём\. Сложно сказать, что именно здесь не так\. То ли обнуление работает как\-то не так, то ли конструкция _r_ _\+\=_ _e_ здесь лишняя\.

## null dereference в логическом выражении

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

```cpp
public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}
```

Предупреждение [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/): Possible null dereference\. Consider inspecting 'langVersion'\. AnalysisFacts\.cs 20

Код проверяет, что переданный параметр _langVersion_ не равен _null_\. Стало быть, разработчик предполагал, что при вызове действительно может быть передан _null_\. Спасает ли проверка от выбрасывания исключения?

Увы, если переменная _langVersion_ будет равна _null_, то значение первой части выражения будет равно _false_\. При вычислении же второй части будет выброшено исключение\.

Как правило, фрагменты кода для статьи приходится дополнительно форматировать для улучшения читаемости\. Этот случай не исключение – выражение, рассмотренное ранее, на самом деле, было записано в одну строку:

![0855_Peachpie_check_ru/image5.png](https://import.viva64.com/docx/blog/0855_Peachpie_check_ru/image5.png)

Учитывая комментарий, можно легко понять, что здесь либо перепутали приоритеты операторов, либо попросту поставили скобку неправильно\. Скорее всего, метод должен выглядеть как\-то так:

```cpp
public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         && (   langVersion.Major > 7 
             || langVersion.Major == 7 && langVersion.Minor >= 2);
}
```

## Вот и всё\!

На самом деле нет\. Анализатор выдал примерно 5 сотен срабатываний на весь проект, и интересных среди них осталось немало\. Поэтому я всё же предлагаю вам самостоятельно [попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) и поглядеть, что новенького он ещё найдёт в этом или других проектах\. Кто знает, быть может, вы сможете обнаружить какие\-нибудь ошибки, которые будут даже интереснее, чем всё, что я тут наразбирал :\)\. Ну и, конечно же, пишите о них в комментарии\. Возможно, найденные вами баги даже попадут в Топ\-10 ошибок 2021\! 

Желаю удачи\!