15.06.2015 04:00
kalessil 21 9393 Источник
Продолжая развивать тему статического анализа, который в общем случае занимается поиском любых дефектов в исходных кодах программ, давайте коснёмся проверки правильности регулярных выражений.

Тема регулярных выражений для PHP довольно щекотлива (примерно как манипулирование массивами), поэтому я вкратце напомню, с чем мы имеем дело.

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

Итак, давайте посмотрим, на примере многим знакомых ZF2 и Symfony2, какие из упомянутых проблем можно найти с помощью статического анализа.

Инструменты


В качестве инструментов были использованы PhpStorm и статический анализатор кода Php Inspections (EA Extended), который ставится как расширение (плагин).

Php Inspections (EA Extended) содержит большой набор правил, разделенных по группам. Если вы ещё не пользовались этим плагином, то первое, что стоит сделать — это проанализировать весь проект. Второе — это настроить под себя Code Style инспекции (Settings -> Editor -> Inspections -> PHP -> Php Inspections (EA Extended)), чтобы сфокусироваться на других сообщениях.

И, конечно, не стоит слепо верить анализаторам: семантика проектов — вещь нетривиальная. Обязательно убедитесь, что код покрыт тестами до внесения изменений — работа со статическими анализаторами требует определенной техники безопасности.

Примеры дефектов


В основном, были найдены мелкие недочёты и всего одна ошибка. Это неудивительно, поскольку оба фреймворка хорошо оптимизированы и протестированы: найти что-то серьёзное очень трудно.

ZF2


Предупреждение: 'str_replace("-", ...)' can be used instead

    return preg_replace('#-#', $this->separator, $value);
    /*
     * Эффективнее будет использовать следующую конструкцию, 
     * т.к. не нужно разбирать и сопоставлять регулярное выражение.
     *
     * return str_replace('-', $this->separator, $value);
     */

Предупреждение: '0 === strpos("...", «get»)' can be used instead

    if (preg_match('/^get/', $method)) {
        ...
    }
    /*
     * Случай, похожий на предыдущий, когда
     * можно использовать базовые функции для работы со строками.
     *
     * if (0 === strpos($method, 'get')) {
     *     ...
     * }
     */

Предупреждение: '[0-9]' can be replaced with '\d' (safe in non-unicode mode)

    if (!preg_match('/^([0-9]{1,3}\.){3}[0-9]{1,3}$/', $host)) {
        ...
    }
    /*
     * Замена не меняет поведения выражения до тех пор пока мы не добавим /u
     * С /u шаблон сможет работать с юникод-числами, например, с арабскими.
     *
     * if (!preg_match('/^(\d{1,3}\.){3}\d{1,3}$/', $host)) {
     *     ...
     * }
     */

Предупреждение: '[^\s]' can be replaced with '\S' (как варианты: \D, \W)

    if ($property->isPublic() && preg_match_all('/@var\s+([^\s]+)/m', $property->getDocComment(), $matches)) {
        ...
    }
    /*
     * Замена на \S не меняет поведения, и это, скорее, приведение к каноническому виду.
     */

Предупреждение: 'i' modifier is ambiguous here (no a-z in given pattern)

    if (preg_match('/([^.]{2,10})$/iu', end($domainParts), $matches)
                        || (array_key_exists(end($domainParts), $this->validIdns))) {
        ...
    }
    /*
     * /i бесполезен, т.к. в шаблоне нет буквенных символов.
     */


Symfony2


Предупреждение: 'false !== strpos("...", «file»)' can be used instead

    return preg_match('/file/', $this->getFilename());
    /*
     * Эффективнее будет использовать следующую конструкцию, 
     * т.к. не нужно разбирать и сопоставлять регулярное выражение.
     *
     * return false !== strpos($this->getFilename(), 'file');
     */

Предупреждение: '[^A-Za-z0-9_]' can be replaced with '\W' (safe in non-unicode mode)

    $gotoname = 'not_'.preg_replace('/[^A-Za-z0-9_]/', '', $name);
    /*
     * Замена на \W не меняет поведения, и это, скорее, приведение к каноническому виду.
     */

Предупреждение: '[a-zA-Z0-9_]' can be replaced with '\w' (safe in non-unicode mode)

    return '' === $name || null === $name || preg_match('/^[a-zA-Z0-9_][a-zA-Z0-9_\-:]*$/D', $name);
    /*
     * Замена на \w не меняет поведения, и это, скорее, приведение к каноническому виду.
     */

Предупреждение: Probably /s modifier is missing (nested tags are not recognized)

    $content = preg_replace('#<esi\:remove>.*?</esi\:remove>#', '', $content);
    /*
     * А вот это уже ошибка: переносы строки во вложенных тегах не учитываются.
     * Необходимо указать модификатор /s
     */


Вместо заключения


Во-первых, я хочу подчеркнуть, что статические анализаторы — это такой же инструмент, как, скажем, XDebug. Например, с Php Inspections (EA Extended) анализ возможных ошибок идёт в процессе написания кода. Стоимость такого «внедрения» нулевая, а трудоёмкость рецензирования и обучения коллег заметно снижается.

Во-вторых, хочу поблагодарить Denis Ryabov, который предложил идею и проработал спецификацию для анализа регулярных выражений в Php Inspections (EA Extended), большое ему спасибо за это.

Комментарии (21)


  1. Placido
    15.06.2015 09:11
    #8460919

    В последнем случае вместо инвертирования жадности *? эффективнее использовать инвертированный символьный класс [^<]*. Тогда и модификатор s не потребуется:

    $content = preg_replace('#<esi\:remove>[^<]*</esi\:remove>#', '', $content);


    1. kalessil Автор
      15.06.2015 09:17
      #8460923

      Этот вариант меняет поведение, по спецификации возможны вложенные теги (во всяком случае мой пулл-реквест с этими правками завернули):

      <esi:include src="http://www.example.com/ad.html"/> 
<esi:remove> 
  <a href="http://www.example.com">www.example.com</a>
</esi:remove>


      1. Placido
        15.06.2015 09:22
        #8460929
        +1

        Согласен.


  1. NorthDakota
    15.06.2015 12:21
    #8461195

    Пришёл на роботу, прочитал статью, поставил плагин.

    И к сожалению, он мне не понравился, так как зачастую «ругается» на вполне адекватные вещи.
    Например:

    protected $testArray = [
    'testKey' => 'testValue',
]


    Ругнулся на запятую, мол php до фени ваша запятая и она будет игнорироватся интерпретатором, уберите её.
    Так я знаю что она будет игнорироватся, поэтому и оставил её там. Тем более логика класа подразумевает что, возможно, я или другой разработчик дополнит масив, и вот тогда дифф в гите будет красивым и читабельным. Благодаря вот той самой запятой.

    P.S. Хотел найти в настройках плагина «отключение» некоторых фич. Не нашёл…


    1. Reposlav
      15.06.2015 12:36
      #8461227

      Перемещаете курсор на запятую, нажимаете alt+enter или появившуюся лампочку, выбираете «Inspection options->edit inspection profile setting», убираете ненужную галочку. По крайней мере так в PhpStorm 7


      1. NorthDakota
        15.06.2015 12:40
        #8461239

        В 8 сработало. Спасибо!


    1. kalessil Автор
      15.06.2015 12:48
      #8461259

      Второе — это настроить под себя Code Style инспекции, чтобы сфокусироваться на других сообщениях.


      Это как раз о таких вещах: запятые, двойные кавычки, вложенные условия.

      Как вариант, Settings -> Editor -> Inspections -> PHP -> Php Inspections (EA Extended): просмотрите всё, что Code Style и настройте под ваши реалии.


    1. kalessil Автор
      15.06.2015 13:01
      #8461311

      В Probable Bugs что-нибудь интересное нашли?


  1. t1gor
    15.06.2015 16:27
    #8461727

    Спасибо, поставил. Будем пробовать.

    А подскажите еще, пожалуйста, где найти настройки плагина?


    1. kalessil Автор
      15.06.2015 16:31
      #8461741

      Пожалуйста. Настроить можно здесь: Settings -> Editor -> Inspections -> PHP -> Php Inspections (EA Extended)

      Посмотрите что из Code Style вам подходит, остальные группы можно не трогать — по ним нареканий ещё не было.


      1. t1gor
        15.06.2015 16:39
        #8461753

        Settings -> Editor -> Inspections -> PHP -> Php Inspections (EA Extended)
        Спасибо, именно то, что искал.


  1. gro
    15.06.2015 17:06
    #8461803

    Когда внезапно начинают пролезать арабские числа, это может быть неприятно. Так что лучше уж [0-9].


    1. kalessil Автор
      15.06.2015 17:12
      #8461823

      На проверенных проектах надо вычищать либо в одну сторону, либо в другую: \d и [0-9] сосуществуют.

      Но [0-9] гораздо реже используется, поэтому, выбор — решение команды, зависящие от конкретного проекта.


      1. gro
        15.06.2015 19:01
        #8462043

        Да, но команда должна знать о таком нюансе, что \d это не только 0-9


  1. kirilloid
    20.06.2015 23:27
    #8470365
    +1

    0 === strpos($method, "get")
    и
    preg_match('/^get/', $method)

    вообще-то не эквиваленты. regex будет работать быстрей на больших строках.
    Хотя и эту оптимизацию можно провести через substr.


    1. kalessil Автор
      21.06.2015 00:33
      #8470427
      -1

      Официальная документация говорит в пользу моего варианта:

      Do not use preg_match() if you only want to check if one string is contained in another string. Use strpos() or strstr() instead as they will be faster.


      1. kirilloid
        21.06.2015 23:28
        #8471183

        Вы понимаете разницу между /get/ и /^get/?


        1. kalessil Автор
          22.06.2015 06:35
          #8471431

          Да, я прекрасно понимаю разницу и вам следует таки опираться на факты и публичные матералы, а не переходить на личности.

          Вот ещё один пруф, что strpos будет бытрее.


        1. kalessil Автор
          22.06.2015 06:42
          #8471435

          Про большие строки (например блоки XML), там же говорится что preg_match будет быстрее, про эти случаи вы правы.