Главная
Статический анализ PHP кода на примере Symfony2 (часть 2)

Статический анализ PHP кода на примере Symfony2 (часть 2) +11

05.04.2015 13:16

kalessil 20 9069 Источник

Аннотация

Второй части этой статьи не планировалось, но тема нашла отклик, так что можно продолжить.

Итак, статический анализ кода в больших проектах необходим, и проекты на PHP — не исключение. По сути, проблемы и методология внедрения средств статического анализа будут те же, что и, скажем, в С++.

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

О том, что можно найти и исправить с минимальным вложением времени (и максимальной отдачей) я расскажу под катом.

Инструменты

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

Для того чтобы автоматизировать рутину по исправлению исходного кода, можно воспользоваться PHP CS Fixer, тем более, что кое-что из Php Inspections (EA Extended) будет добавлено в CS Fixer (вот, например). Стоит присмотреться к этой утилите — она автоматически исправляет код, освобождая много времени, и уменьшает вероятность внести ошибки, исправляя код вручную.

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

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

Reference mismatch и проблемы с памятью

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

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

namespace Symfony\Component\HttpFoundation\Session\Attribute;

class NamespacedAttributeBag extends AttributeBag {
    ...
    public function get($name, $default = null) {
        /*
         * protected function &resolveAttributePath($name, $writeContext = false);
         * reference mismatch, копия возвращаемого массива будет передана в переменную
         */
         $attributes = $this->resolveAttributePath($name);
         ...
    }
    ...
}

Как должно быть:

    public function get($name, $default = null) {
        $attributes = &$this->resolveAttributePath($name);
        ...
    }

Foreach, значения по ссылке и неочевидные ошибки

Особенность foreach в том, что он не создаёт собственной области видимости, и переменные остаются в родительской области. Поэтому, объявляя значение как ссылку, можно внести очень неприятный баг, когда последнее значение массива меняется без видимой причины.

Статический анализ выявит такие места уже на первом проходе.

namespace Symfony\Component\Console\Descriptor;

class ApplicationDescription {
    ...
    private function sortCommands(array $commands) {
        ...
        foreach ($namespacedCommands as &$commands) {
            ksort($commands);
        }
        /*
         * Если тут что-то сделать с $commands,
         * то послений элемент $namespacedCommands будет повреждён
         */

        return $namespacedCommands;
    }
    ...
}

Как должно быть:

    private function sortCommands(array $commands)
    {
        ...
        foreach ($namespacedCommands as &$commands) {
            ksort($commands);
        }
        unset($commands);
        ...

        return $namespacedCommands;
    }

Микро-оптимизации

Тут я просто приведу примеры кода, которые неоптимальны, но выполняют свою задачу.
По моим наблюдениям, такой код появляется в системах при периодической смене членов команды с включением в команду начинающих разработчиков.

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

//  if (false !== strpos($lang, '-')) - оптимальный вариант
    if (strstr($lang, '-')) {
        ...
    }

//  $cast = (int) $value - оптимальный вариант
    $cast = intval($value);

//  return (float) $value - оптимальный вариант
    return floatval($value);

//  $this->ignore |= static::IGNORE_DOT_FILES - оптимальный вариант
    $this->ignore = $this->ignore | static::IGNORE_DOT_FILES;

//  if (!in_array(static::$availableOptions[$option], $this->options)) - оптимальный вариант
    if (false === array_search(static::$availableOptions[$option], $this->options)) {
        ...
    }

//  ++$calls[$id] - оптимальный вариант
    $calls[$id] += 1;

//  if ('root' === get_current_user()) - оптимальный вариант
    if (in_array(get_current_user(), array('root'))) {
        ...
    }

//  if (array_key_exists($id, $container->getAliases())) - оптимальный вариант
    if (in_array($id, array_keys($container->getAliases()))) {
        ...
    }

//  return '' === $relativePath ? './' : $relativePath - оптимальный вариант
    return (strlen($relativePath) === 0) ? './' : $relativePath;

Мёртвый код

Иногда можно найти артефакты рефакторинга, незамеченные простыми анализаторами.

namespace Symfony\Component\Security\Acl\Dbal;

class MutableAclProvider extends AclProvider implements MutableAclProviderInterface, PropertyChangedListener {
	...
    private function updateNewFieldAceProperty($name, array $changes)
    {
        ...
        // мёртвый код
        $currentIds = array();

        foreach ($changes[1] as $field => $new) {
            for ($i = 0, $c = count($new); $i < $c; $i++) {
                ...

                if (null === $ace->getId()) {
                    ...
                } else {
                    // мёртвый код		
                    $currentIds[$ace->getId()] = true;		
                }
            }
        }
    }

	...
}

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

Надеюсь, что примеры из статьи (а все они часть PRs в Symfony2) мотивируют вас проверить свои проекты еще раз и внедрить инструменты статического анализа в повседневную работу.

В больших коммерческих системах, которые в возрасте 5-10 лет требуют очень много ресурсов на наведение порядка и устранение «странных» ошибок, Php Inspections (EA Extended) и PHP CS Fixer станут для вас незаменимыми иструментами на каждый день.

Для опенсорса я напомню про Scrutinizer CI и Sensio Labs Insights — очень полезные инструменты, которые можно использовать вместе с двумя предыдущими.

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

nazarpc
05.04.2015 16:54
#8362049
+1
Во-первых релиз с указанными фичами доступен уже давненько, вышла даже более новая версия с исправлениями foreach и приоритетов внутри if.
А во-вторых, я был удивлен сколько всего нашли Sensio Labs Insignts и Scrutinizer, даже при том, что пользуюсь PhpStorm уже давно и упомянутым плагином тоже.

Так что рекомендую пересмотреть политику передачи кода третьей стороне, с вероятностью 95% (навскидку) у вас там нет ничего такого, что нельзя вообще выложить в виде Open Source, пусть даже с запретом коммерческого использования другими.

kalessil Автор
05.04.2015 17:09
#8362069
Так и есть. Только инспекция reference mismatch ещё не релизилась (в истории коммитов плагина видно).

Sensio Labs Insignts и Scrutinizer — очень продвинутые инструменты, я пробовал на приватных проектах и упоминаю при случае. По открытию исходников — не я решаю, а позиция менеджмента — закрытое ПО.

Sensio Labs Insignts очень хорошо работает с симфони проектами, а Scrutinizer еще и по уязвимостям хорошо специализируется. Вместе с Php Inspections (EA Extended) — это просто без комментов =)

nazarpc: а можете ссылки в личку кинуть с результатами от Scrutinizer и Insignts — мне любопытно?

KonstantinKuklin
05.04.2015 17:33
#8362087
плагин к IDE это хорошо, но не знаете ли вы подобный функционал, идущий как сканер, анализатор и какая-нибудь вебморда, чтобы мониторить ошибки в коде без передачи кода кому-то? (сенсио лабс, скрутинизер не подходят)
знаю, что можно экспортировать отчеты из шторма, но все же. Аля Coverity для PHP
1. kalessil Автор
  05.04.2015 17:43
  #8362095
  Подобный, к сожалению, не встречался.
  
  Посмотрите, может что-то из этого вам пригодится — я обычно отвечаю в полном объеме, но по ссылке есть детали как интегрировать с git.
  
  Ну и PHP CS Fixer, конечно, стоит использовать.
1. Corpsee
  05.04.2015 19:13
  #8362177
  PHPCI можете попробовать, вроде как раз то, что вы хотите.

kix
05.04.2015 20:17
#8362243
О, я вижу, вы продолжаете развивать плагин! Спасибо, очень полезное дело!
1. kalessil Автор
  05.04.2015 20:45
  #8362287
  Да, надо довести его до экспертного уровня — всё ещё есть пробелы. Хотя результат мне определённо нравится.
  
  Пожалуйста =) Сейчас правда остро встал вопрос пиара, но надеюсь, что это не затронет частоту релизов.

Rathil
06.04.2015 01:23
#8362491
На счёт «мертвого кода» — достаточно спорный анализ. Далеко не всегда 100% можно определить прохождения по ветке… Я бы сказал, что мало когда можно определить это…
1. kalessil Автор
  06.04.2015 08:33
  #8362589
  Анализом прохождения по всем веткам плагин и не занимается.
  
  В примере сначала нашлась локальная неиспользуемая переменная (массив, в который была только запись). Уже по факту выяснилось что это мёртвый код.
  
  Инспекция условных выражений тоже выводит на мёртвый код (if (false), if (false &&)) — в нашем проекте было несколько таких мест, по-моему в swiftmail было что-то похожее. В Symfony2, конечно же таких мест не нашлось.
  1. lany
    06.04.2015 16:49
    #8363451
    А если б массив убежал из поля видимости (например, был присвоен в переменную с более широкой областью видимости), предупреждение бы исчезло?
    
    А если бы единственное использование массива выглядело как $currentIds[$ace->getId()] = $currentIds[$ace->getId()]+1, анализатор бы понял, что массив бесполезен?
    
    kalessil Автор
    06.04.2015 19:09
    #8363613
    Если совсем просто, то любое чтение массива (передача в другую переменную, простейшие операции и передача в аргументы функции) отключит предупреждение.
    
    Поэтому:
    
    — Да
    — Нет (но анализатор ругнулся бы, т.к. можно сделать ++$currentIds[$ace->getId()] — тут ему ума хватает)

CentOS
06.04.2015 19:17
#8363637
У меня есть несколько вопросов по плагину, прошу прощения что пишу их здесь.

1) Скажите, пожалуйста, в чем практический смысл оптимизации
Problem synopsis: Safely use single quotes instead
Плагин все время просит заменить двойные кавычки на одинарные, я же в своем проекте придерживаюсь единого стиля с кавычками, только двойные в длинных строковых выражениях.
Например $this->log(Logger::INFO, «Load and calculate template»);

2) Problem synopsis: Safely use '… === ...', '… !== ...' constructions instead
Пример кода, где высвечивается эта подсказка
if ($data['type'] == 'img') {
$this->template_load_mask[$name] = 1;
}

Почему здесь лучше использовать ===, а не ==

Спасибо.
1. kalessil Автор
  06.04.2015 19:20
  #8363641
  Отвечу в личных сообщениях.
  1. DjOnline
    07.04.2015 00:57
    #8363991
    +1
    Почему же, всем интересно.
    
    kalessil Автор
    07.04.2015 12:30
    #8364437
    Хорошо:
    
    Двойные кавычки в плагине — стиль кода. В предыдущей статье обсудили что с оп-кешем двойные или одинарные кавычки — на производительность никак не повлияет. Отключите, если вы используете двойные.
    
    ===/!== — типо-зависимая операция сравнения, с ней '0' не равно 0, а вот ==/!= думает по-другому — такая же беда как и с empty.

vladkens
06.04.2015 22:44
#8363863
```
//  return '' === $relativePath ? './' : $relativePath - оптимальный вариант
```
Ещё лучше:
```
return empty($relativePath) ? './' : $relativePath;
```
1. kalessil Автор
  06.04.2015 22:56
  #8363885
  +1
  Не лучше, скорее даже наоборот (смена поведения и полная потеря контроля над типами):
  
  И, собственно, примеры, как empty портит жизнь:
  
  empty(array()); // true empty("0"); // true empty(new \ArrayObject()); // false
  1. LionAlex
    07.04.2015 11:36
    #8364333
    Ну в некоторых случаях это как раз плюс, а не минус. Также, вы рекомендуете count вместо empty, но count сильно медленней. Понятно, что это экономия на спичках, но все же.
    
    kalessil Автор
    07.04.2015 12:45
    #8364467
    Плагин в таких случаях пытается понять, что же имелось ввиду — использовать count/null сравнение и т.д. — реверсинжиниринг по сути.
    Цель — чётко описать в коде, что и как этот код делает. С учётом доступных оптимизаций, count вооще не проблема.
    
    Это один из аспектов именно этого плагина — избавиться от каши в коде.

kalessil Автор
06.04.2015 23:39
#8363933
В личных сообщениях попросили продублировать рекомендацию ещё одного плагина: PHP Advanced AutoComplete.

Дублирую: подсказывает стандартные варианты параметров, например, для date, ini_get, header и т.д.

Статический анализ PHP кода на примере Symfony2 (часть 2) +11

Аннотация

Инструменты

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

Reference mismatch и проблемы с памятью

Foreach, значения по ссылке и неочевидные ошибки

Микро-оптимизации

Мёртвый код

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

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

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор

kalessil Автор