Проект как найти ошибку

Проектная и исследовательская деятельность учащихся на базе областной предметной лаборатории «Биология. Экология»

Анализ типичных ошибок в проектных и исследовательских работах учащихся

• Предметная эколого-биологическая лаборатория школы создана как инновационная структура, педагогов, осваивающих новые методы и технологии обучения в 2012 году

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

Организуй свою жизнь как ПРОЕКТ

перспективный

универсальный метод

легкомысленное прожектерство

Ведущий

≠

Доклад

≠

Проект

?

Реферат

Доклад — устное или письменное сообщение с целью познакомить слушателей (читателей) с определённой темой (проблемой), дать общую информацию, возможно, представить соображения автора доклада, которые в данном случае не требуют научной проверки или доказательств.

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

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

Этапы работы методом проектов

1

Погружение

в проект

Мероприятия,

методы, способы

«Как?»

Результат

«Что получиться?»

(мы это

(как решится

можем делать)

проблема)

Проблема проекта

«Почему?»

(это важно

для меня лично)

Цель проекта

«Зачем?»

(мы делаем проект)

2

организация

деятельности

Планирование

деятельности по

решению

задач проекта

Планирование

Распределение

возможной формы

заданий

презентации

результатов

в группах

Организация

рабочих групп

3

Изучение

осуществление

и анализ

деятельности

информации

Исследование

наблюдения

эксперимент

Поиск

информации

4

презентация

проекта

Проектирование

Исследование

1. Разработка и создание планируемого объекта или его определенного состояния

2. Решение практической проблемы

1. Не предполагает создание заранее планируемого объекта

3. Подготовка конкретного варианта изменения элементов среды

2. Создание нового интеллектуального продукта

3. Процесс поиска неизвестного, получение нового знания

Научно-исследовательская деятельность учащихся – это перспективная форма организации детей. Это и учеба, и работа одновременно

Экологическая тропа

Этапы исследовательской работы

• введение (где отражены цель и задачи, актуальность проблемы)
• основная часть (в которой раскрывается содержание работы: теоретическая и практическая часть)
• заключение (в котором содержатся итоги работы, выводы и рекомендации)
• список литературы (представляет собой перечень использованных книг и статей)
• приложение (наглядное представление опытных данных в виде таблиц, схем, диаграмм, и т.д.)

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

Неудачные названия

Адекватные правильные названия

Метеориты – гости из космоса (журналистское, очень широкое)

Изучение метеоритов в современной науке и их классификация

Как компьютер появился на свет (примитивное, ненаучное, не связывает с предметом исследования)

История преодоления проблем при создании компьютеров

Внимание, Акулы! (журналистское, очень широкое, не связывает с предметом исследования)

Исследование причин и факторов нападения акул на человека

Радуга в доме (ненаучное, примитивное, журналистское, не связывает с предметом исследования)

Исследования условий возникновения радуги

ВВЕДЕНИЕ

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

Объект

Предмет

Глаз

Свойства и структура глаза как оптического инструмента

Магнитное поле

Магнитное поле в школьных учебных кабинетах

  Два тюлененка, привезенные в зоопарк с побережья Балтийского моря.

Адаптация тюленей к условиям зоопарка.

Гипотеза должна удовлетворять ряду требований:

быть проверяемой;

содержать предположение;

быть логически непротиворечивой;

соответствовать фактам.

При формулировке гипотезы обычно используются словесные конструкции типа:

«если…, то…»;

«так…, как …»;

«при условии, что…»

Пример

Тема: «Русская кухня: вчера, сегодня, завтра…»

Положительная гипотеза: рецептура блюд русской кухни изменилась с течением времени.

Отрицательная гипотеза: рецептура блюд русской кухни не изменилась с течением времени.

Цель и задачи

Формулировку цели рекомендуется обычно начинать существительным.

Задачи – это этапы работы, они не могут быть крупнее цели или повторять ее. Часто конкретная задача связана с определенным методом исследования. Не следует формулировать задачи, которые непосильны для ученика

Пример из пособия «Школа исследователей»

Тема: « Символический образ моря в искусстве »

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

Предмет исследования : символический образ моря в искусстве.

Объекты исследования: стихотворения, музыкальные произведения, художественные полотна.

Задачи:

— проанализировать тексты поэтов: В. А. Жуковского, А. С. Пушкина, Н. А. Языкова, Д. Г. Байрона с точки зрения символической составляющей.

-проанализировать музыкальные композиции Н. А. Римского- Корсакова, К. Дебюсси с точки зрения символической составляющей.

-проанализировать художественные композиции И. К. Айвазовского, К. Моне с точки зрения символической составляющей.

Типичные ошибки при написании Введения

• Слишком большой объем раздела (во введении присутствует часть обзора литературы). • Отсутствие четкой формулировки проблемы, актуальности работы.

• Некорректная формулировка цели и задач исследования, несоответствие цели работы теме; несоответствие пунктов плана задачам и цели исследования.

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

Наиболее типичные ошибки:

содержание представляет собой реферат – копирование текстов из научных трудов, без их анализа и часто без соответствующих сносок; глава представляет собой «Словарь» терминов и понятий или классификацию без логического начала и завершения; или носит характер школьного сочинения на заданную тему, а не исследование.

Выводы и заключение

• Любая работа требует выводов и заключения(1-2 страницы)
• Это логическое завершение всей работы. При написании заключения необходимо держать перед глазами введение работы.
• В заключение указывается: была ли достигнута цель исследования и были ли решены все задачи, указанные в введении.

Ошибки при написании выводов:

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

• Каждый вывод – это обычно одна достаточно короткая фраза, в которой цитируются конкретные результаты, формальные фразы или формальные выводы никак не связанные с основной частью работы.
• Не должно быть перечислений выводов, без соответствующих пояснений.. Если выводы не соответствуют исходным задачам, надо изменить формулировку задач в окончательном тексте работы в соответствии с полученными результатами и сформулированными выводами.

Защита работ юных исследователей

Опыт менеджеров Redmadrobot.

Мы, электрические, запускаем проекты с 2009 года, и за 11 лет сформировали сильную команду робоменеджеров. Прокачивать железных помогают боевые задачи и одна из самых сложных — управлять проектом.

Ситуации, при которых появляется необходимость взять на себя обязанности PM (project manager), бывают разные: в маркетинге — при создании сайта, в HR — при организации мероприятий. Мы вспомнили десятки подобных случаев и подготовили список ошибок, которые допускают новоиспеченные руководители проектов.

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

На самом старте

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

Ошибка № 1: отсутствие фиксации договорённостей со стейкхолдерами

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

Ожидания от продукта задают требования к результату и существенно влияют на дальнейшую работу всей команды. Согласование помогает избежать возможных недопониманий и разногласий по развитию проекта.

Простое решение: составьте текстовый документ или таблицу, где будут собраны ожидания стейкхолдеров. Чтобы упростить себе жизнь, вы можете воспользоваться нашей подсказкой по структурированию содержания.

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

После формирования документа синхронизируйте его с командой (наверняка вам подкинут ещё пару-тройку идей) и смело отправляйте стейкхолдерам, чтобы финально согласовать все моменты.

Вариант посложнее: железная команда аналитиков и дизайнеров Redmadrobot на этапе старта проекта всегда собирает паспорт продукта — документ с описанием характеристик и основных целей.

Он содержит в себе ключевые показатели и выступает в роли ориентира для дальнейшего развития проекта или группы проектов. Если хотите избежать проблем, то его нужно проработать, синхронизировать с командой (собрать всевозможные идеи) и согласовать со стейкхолдерами.

После этого итоги согласования нужно зафиксировать для всех — разослать письма, выложить документ в облако или создать борд в Miro.

Ошибка № 2: бездумное формулирование целей и задач в команде

Одна из причин, почему идеи не выдерживают проверку временем — отсутствие «моста реализуемости». Менеджер может понимать цели стейкхолдеров и то, как их достичь, но не представлять вижн результата. Или, всё же видеть желаемый результат, но не учитывать всех трудностей, которые, вероятно, возникнут в будущем.

На выходе мы имеем старое доброе «ничего», а прикрываемся «внешними факторами», «незапланированными активностями» и «кто-то виноват, а мы не подозревали».

Простое решение: вариант для самостоятельного использования — воспользуйтесь одной из проверенных временем методологий постановки задач, например, SMART. Мы часто используем в работе подходы DoD и DoR .

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

Вариант посложнее: изучите вместе с коллегами одну из методологий постановки задач и грамотно её внедрите. Для этого разделите планирование результатов на несколько итераций, после чего сделайте deep dive (погружение в проект) всей команды по методологии постановки задач.

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

Ошибка № 3: перекладывание ответственности

Здесь всё просто: ответственность и распределение ролей в команде должны быть понятны всем участникам. В какой-то момент (а он точно может наступить, поверьте) придётся разбираться, почему что-то пошло не так и кто-то не выполнил важную задачу.

А в том случае, если в проекте что-то пойдёт не по плану, менеджер должен чётко понимать, в какой его части произошёл сбой. Иначе PM рискует поставить под угрозу результаты работы и деморализовать всю команду.

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

Простое решение, если не хотите заморачиваться и у вас небольшой проект:

Главное — договоритесь о том, как планируете взаимодействовать в команде и зафиксируйте это любым удобным способом: текстом, в Miro или, например, набросайте mind map — как вам удобно.

Вариант посложнее: для крупных проектов можно сделать ролевую модель проекта в виде карты и добавить описание функций каждой роли (это можно сделать, например, в Miro).

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

По ходу реализации: про реализацию

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

Ошибка № 4: отсутствие инструментов для командной работы

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

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

Простое решение: не гонитесь за модой — используйте доступные и универсальные инструменты, такие как Trello или Google-таблицы.

Благодаря своей системе карточек, Trello отлично подойдёт для распределения задач. В них вы можете фиксировать важные части проекта: процесс выполнения работ, ведение сделок и заявок, организацию документооборота и другие вещи. Ну а Google-таблицы хороши своей многозадачностью. В Redmadrobot команды часто используют их в самых разных делах:

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

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

Ошибка № 5: отсутствие чётких приоритетов

В ситуации, когда приоритетно всё — на самом деле никаких приоритетов не существует. Если работа кипит и проект развивается, то это ещё не значит, что команда идёт к нужному результату.

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

Простое решение: если у вас небольшой проект, то используйте методологию MоSCоW — она хорошо подходит для определения приоритезации. Также обратите внимание на методологии RICE и ICE.

Вариант посложнее: в крупном проекте поможет формирование собственной системы приоритезации входящих задач. Её можно построить на базе Google-таблиц или Trello.

С помощью Google-таблиц или Trello можно собрать систему с упором на потребности команды. Например, Google-таблицы позволяют настроить фильтры и формулы, которые автоматизируют учёт. Исходя из баллов, выставленных по конкретному параметру, фильтры и формулы могут автоматически выстраивать итоговый список с приоритетами, что значительно экономит время.

Ошибка № 6: отсутствие контрольных точек и оценки результата

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

Решение:

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

По ходу реализации: про взаимодействие

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

Ошибка № 7: неинформирование команды

Чтобы чётко определить вектор развития проекта, эскалацию, решение проблем и запросов, важно рассказывать команде обо всех рабочих изменениях. Это касается не только коллег, но и стейкхолдеров.

Решение:

Ошибка № 8: отсутствие реакции на изменения или критичные ситуации

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

Поэтому, каждой команде нужен понятный механизм эскалации вопросов и конфликтов. Менеджер проектов должен уметь принимать верные решения в критических и незапланированных ситуациях, и находить пути выхода из «застоя» (в ситуациях, которые требуют пересмотра).

Простое решение: расскажите команде про базовые правила эскалации. Для этого сформулируйте, как вы видите процесс эскалации в идеале, зафиксируйте его в виде простых тезисов. Разместите информацию в «общем пространстве», чтобы в нужный момент у всех был доступ к документу.

Вариант посложнее: сформируйте собственную систему эскалации. Для этого составьте список коллег, которые будут помогать вам в решении проблемных ситуаций и конфликтов, а также в определении стратегических действий. О возможностях инструмента эскалации и способах его организации написано много статей, поэтому, если вам интересна эта тема, просто «загуглите».

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

И в конце

Проект всегда имеет конечные точки и финал. Соберитесь силами и не допустите в них последних двух ошибок.

Ошибки № 9 и № 10: отсутствие рефлексии и подведения итогов

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

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

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

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

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

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

Вопросы рефлексии корректируются в зависимости от ситуации, но стандарт при их составлении один. Постарайтесь найти ответ на каждый из них:

У нас получился не самый полный список ошибок, но если вы сможете не совершать хотя бы их, то сделаете работу над проектом проще и эффективнее. Мы, роботы, убеждены, что совершение ошибок — это путь к совершенству, ведь мы постоянно на них учимся. Поэтому, советуем вам не бояться рисковать. А ещё, не забывайте прислушиваться не только к коллегам, но и к самому себе. Согласны?

Крупные проекты проверять интересно. Как правило, в них удаётся найти различные интересные ошибки и рассказать о них людям. Также это хороший способ тестирования нашего анализатора и улучшения различных его аспектов. Я давно хотел проверить ‘Mono’, и наконец-то такая возможность появилась. И, стоит сказать, проверка себя оправдала, так как удалось найти много занимательного. О том, что же нашлось, какие нюансы возникли при проверке, и будет эта статья.

О проекте

Mono — проект по созданию полноценного воплощения системы .NET Framework на базе свободного программного обеспечения. Основной разработчик проекта Mono — корпорация Xamarin, ранее Novell.

Mono включает компилятор языка C#, среду исполнения .NET — mono (с поддержкой JIT) и mint (без поддержки JIT), отладчик, а также ряд библиотек, включая реализацию WinForms, ADO.NET и ASP.NET, а также компиляторы smcs (для создания приложений для Moonlight) и vbc (для приложений, написанных на VB.NET).

В рамках проекта также разрабатываются привязки для графической библиотеки GTK+ на платформу .NET.

Исходный код доступен для загрузки в репозитории на GitHub. Количество строк кода при проверке загруженного с GitHub репозитория составило около 6.3 миллионов (исключая пустые строки). Такой объём кодовой базы выглядит крайне привлекательно — наверняка где-то в коде закрались ошибки. С другой стороны, проверка такого большого проекта будет полезна и анализатору, так как послужит ему хорошим стресс-тестом.

Инструмент анализа, особенности проверки

В качестве инструмента анализа выступал статический анализатор кода PVS-Studio. На данный момент C#-анализатор включает в себя свыше 100 диагностических правил, каждое из которых сопровождается документацией, содержащей информацию об ошибке, возможных последствиях и способах исправления. С момента релиза удалось проверить много различных проектов, написанных на C#, таких как Roslyn, Xamarin.Forms, Space Engineers, CoreFX, Code Contracts и пр. (с полным списком можно ознакомиться по ссылке).

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

Также хочу отметить, что в статье не приводились ошибки из файлов, содержащих в начале файла какое-то упоминание корпорации Microsoft. Это сделано в основном для того, чтобы избежать возможных пересечений с ошибками, упоминавшимися в других статьях. В любом случае, материала и так набралось достаточно.

Как и всегда, в статье приведены не все ошибки, так как это сильно бы увеличило её объем. Я постарался выбрать наиболее интересные места, но многие осталась всё же за рамками статьи. Не подумайте, что я хочу упрекнуть в чем-то авторов ‘Mono’. Количество ошибок обусловлено большим размером проекта, и это понятно. Тем не менее, хорошо бы исправить имеющиеся ошибки и не допускать появления новых. Помочь с этим поможет внедрение статического анализа в процесс разработки. Подробнее об этом написано ниже в соответствующем разделе.

Пара слов о том, почему проверка проектов — занятие нетривиальное

В идеальном мире проверка проектов и написание статьи осуществляется по следующему сценарию: нашёл проект -> собрал -> проверил анализатором -> нашёл достаточное количество ошибок -> написал статью. Все рады, все довольны: мы поставили себе галочку о проверенном проекте, люди прочитали новую статью, разработчики узнали об ошибках в коде, автор — молодец.

Увы, наш мир не идеален. На тех или иных этапах часто возникают проблемы, в этот раз — со сборкой. Если есть подробная инструкция по сборке, или же она легко проводится своими силами — это замечательно! Тогда можно смело приступать к проверке проекта и написанию статьи. Иначе начинается головная боль. Так вышло и с ‘Mono’. Решение net_4_x.sln, объединяющее в себе C#-проекты, не собирается ‘из коробки’ (т.е. сразу после загрузки из репозитория). Один из скриптов сборки оказался нерабочим (неправильно был прописан путь (возможно из-за того, что иерархия директорий со временем менялась)), но его исправление также не принесло плодов.

Конечно, отступать не хотелось, поэтому я экспериментировал со сборкой даже во внерабочее время, но результата оказалось немного. В итоге, провозившись с этим порядочное количество времени, было принято решение написать статью «как есть».

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

Конечно, проверять несобранный проект нехорошо по нескольким причинам:

• анализ проекта получается не таким качественным, как мог бы. Это факт. В чём именно снижается качество — зависит от реализации диагностического правила. Возможно, появится ложное срабатывание, или же наоборот, полезное срабатывание не будет выдано;
• при просмотре лога нужно быть на порядок внимательнее, так как возникает (пусть и небольшая) вероятность появления ложных срабатываний, которых не было бы при корректной сборке проекта;
• так как некоторые полезные срабатывания пропадают, возможен пропуск интересных ошибок, которые могли бы попасть в статью и о которых узнали бы разработчики (однако они могут узнать об этих ошибках, самостоятельно проверив проект);
• приходится писать разделы «Пара слов о том, почему проверка проектов…».

Тем не менее, нашлось много подозрительных мест, некоторые из которых и будут рассмотрены ниже.

Результаты проверки

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

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

Во-вторых, на полностью собранном проекте эта статистика может поменяться в любую из сторон: уменьшения или увеличения количества предупреждений. В собранном проекте анализатору доступно больше семантической информации, следовательно, он может провести более глубокий анализ (исчезнут ложные срабатывания, появятся новые предупреждения). Для интересующихся, как семантическая информация влияет на анализ и по каким принципам всё это работает, предлагаю ознакомиться со статьёй «Введение в Roslyn. Использование для разработки инструментов статического анализа».

Но вам наверняка не терпится посмотреть, что же интересного удалось найти в коде проекта? Что ж, давайте рассмотрим некоторые фрагменты кода.

Одинаковые подвыражения в пределах одного выражения

Одна из самых распространённых ошибок. Причин для возникновения — множество. К ним можно отнести copy-paste, схожие названия переменных, злоупотребление IntelliSense, банальную невнимательность. Отвлеклись на минутку — сделали ошибку.

public int ExactInference (TypeSpec u, TypeSpec v)
{
  ....
  var ac_u = (ArrayContainer) u;
  var ac_v = (ArrayContainer) v;
  ....
  var ga_u = u.TypeArguments;
  var ga_v = v.TypeArguments;
  ....
  if (u.TypeArguments.Length != u.TypeArguments.Length) // <=
    return 0;

  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘u.TypeArguments.Length’ to the left and to the right of the ‘!=’ operator. generic.cs 3135

Теперь, когда код метода упрощён донельзя, заметить ошибку в условии оператора if не составит труда — в качество экземпляра типа TypeSpec во втором подвыражении должен был выступать параметр v, а не u. Возможно, ошибка была допущена из-за того, что внешне символы u и v схожи, и, не акцентируя внимания на этом выражении, можно легко не заметить подвоха.

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

if (u.TypeArguments.Length != v.TypeArguments.Length)
    return 0;

Не менее интересный случай.

bool BetterFunction (....)
{
  ....
  int j = 0;
  ....
  if (!candidate_params && 
      !candidate_pd.FixedParameters [j - j].HasDefaultValue) { // <=
    return true;
  }
  ....
  if (!candidate_pd.FixedParameters [j - 1].HasDefaultValue &&   
       best_pd.FixedParameters [j - 1].HasDefaultValue)
    return true;

  if (candidate_pd.FixedParameters [j - 1].HasDefaultValue &&     
      best_pd.HasParams)
    return true;
  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘j’ to the left and to the right of the ‘-‘ operator. ecore.cs 4832

В одном из выражений вычисления индекса программист ошибся, написав выражение j — j. Таким образом, всегда будет выполняться доступ к первому элементу массива. Если бы это и требовалось, логичнее было использовать целочисленный литерал, равный 0. То, что это действительно ошибка, подтверждают другие обращения по индексу к этому же массиву: j — 1. Предположу, что, опять же, ошибка не была замечена из-за некоторой схожести в написании символов j и 1, так что при беглом просмотре кода этого можно было не заметить.

Примечание коллеги Андрея Карпова. Когда я читал черновик статьи, то уже собирался сделать Сергею пометку, что он видимо не тот участок кода выписал. Я смотрел в код и не видел ошибку. Только начав читать описание, я понял, в чем дело. Подтверждаю, эту опечатку очень сложно заметить. Наш PVS-Studio шикарен!

Продолжим «ломать глаза»:

internal void SetRequestLine (string req)
{
  ....
  if ((ic >= 'A' && ic <= 'Z') ||
      (ic > 32 && c < 127 && c != '(' && c != ')' && c != '<'    &&
       c != '<' && c != '>'  && c != '@' && c != ',' && c != ';' &&
       c != ':' && c != '\' && c != '"' && c != '/' && c != '[' &&
       c != ']' && c != '?'  && c != '=' && c != '{' && c != '}'))
    continue;
  ....
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘c != ‘<» to the left and to the right of the ‘&&’ operator. HttpListenerRequest.cs 99

В пределах выражения дважды встречается подвыражение c != ‘<‘. Наверное, просто лишнее сравнение.

protected virtual bool ShouldSerializeLinkHoverColor ()
{
  return grid_style.LinkHoverColor != grid_style.LinkHoverColor;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘grid_style.LinkHoverColor’ to the left and to the right of the ‘!=’ operator. DataGrid.cs 2225

Упрощать метод, чтобы выделить ошибку, не понадобилось. В сравнении участвуют одинаковые подвыражения — grid_style.LinkHoverColor.

Корректный код должен выглядеть так:

protected virtual bool ShouldSerializeLinkHoverColor ()
{
  return grid_style.LinkHoverColor != default_style.LinkHoverColor;
}

Почему именно так? Выше есть ряд методов, в которых различные свойства объекта grid_style сравниваются со свойствами объекта default_style. Но в последнем случае расслабились и допустили ошибку. Хм, «эффект последней строки»?

Ну а это классика опечаток:

static bool AreEqual (VisualStyleElement value1, 
                      VisualStyleElement value2)
{
  return
    value1.ClassName == value1.ClassName && // <=
    value1.Part == value2.Part &&
    value1.State == value2.State;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions ‘value1.ClassName’ to the left and to the right of the ‘==’ operator. ThemeVisualStyles.cs 2141

Случайно сравнили подвыражение value1.ClassName само с собой. Конечно же, во втором случае должен был использоваться объект value2.

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

static bool AreEqual (VisualStyleElement value1, 
                      VisualStyleElement value2)
{
  return
       value1.ClassName == value1.ClassName
    && value1.Part      == value2.Part
    && value1.State     == value2.State;
}

Отформатированный таким образом код намного проще читать, и легче заметить, что с одним столбцом что-то не так. Подробнее про выравнивание кода смотрите главу 13 из книги «Главный вопрос программирования, рефакторинга и всего такого».

Остальные подозрительные места, обнаруженные диагностическим правилом V3001, приведены в файле.

Одинаковые условия в конструкции ‘else if’

enum TitleStyle {
  None   = 0,
  Normal = 1,
  Tool   = 2
}
internal TitleStyle title_style;
public Point MenuOrigin {
  get {
    ....
    if (this.title_style == TitleStyle.Normal)  {        // <=
      pt.Y += caption_height;
    } else if (this.title_style == TitleStyle.Normal)  { // <=
      pt.Y += tool_caption_height;
    }
    ....
}

Предупреждение PVS-Studio: V3003 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 597, 599. Hwnd.cs 597

Два раза проверяется одинаковое выражение this.title_style == TitleStyle.Normal. Очевидно, что этот код содержит ошибку. Ведь вне зависимости от значения вышеприведённого выражения, выражение pt.Y += tool_caption_height никогда не будет выполнено. Предположу, что втором случае подразумевалось сравнение поля title_style с константой TitleStyle.Tool.

Одинаковые тела ‘if-then’ и ‘if-else’

public static void DrawText (....)
{
  ....
  if (showNonPrint)
    TextRenderer.DrawTextInternal (g, text, font, 
      new Rectangle (new Point ((int)x, (int)y), max_size), color,   
      TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
  else
    TextRenderer.DrawTextInternal (g, text, font, 
      new Rectangle (new Point ((int)x, (int)y), max_size), color, 
      TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
  ....
}

Предупреждение PVS-Studio: V3004 The ‘then’ statement is equivalent to the ‘else’ statement. System.Windows.Forms-net_4_x TextBoxTextRenderer.cs 79

Вне зависимости от значения переменной showNonPrint будет вызван статический метод DrawTextInternal класса TextRenderer с одними и теми же аргументами. Возможно, что ошибку допустили из-за использования copy-paste. Вызов метода скопировали, а исправить аргументы забыли.

Возвращаемое значение метода не используется

public override object ConvertTo(.... object value, 
                                 Type destinationType) 
{
  ....
  if (destinationType == typeof(string)) {
    if (value == null) {
      return String.Empty;
    }
    else {
      value.ToString();
    }
  }
  ....
}

Предупреждение PVS-Studio: V3010 The return value of function ‘ToString’ is required to be utilized. ColumnTypeConverter.cs 91

Весьма интересная ошибка, судя по всему, с далеко идущими последствиями. Из кода видно, что выполняется проверка типа, и, если тип — string, осуществляется проверка на равенство null. А дальше — самое интересное. Если ссылка value имеет значение null, возвращается пустая строка, а иначе… Скорее всего предполагалось, что будет возвращено строковое представление объекта, но оператор return отсутствует. Следовательно, возвращаемое значение метода ToString() никак не будет использовано, а метод ConvertTo продолжит своё выполнение. Таким образом, из-за забытого оператора return изменилась логика работы программы. Предполагаю, что корректный вариант кода должен выглядеть так:

if (value == null) {
  return String.Empty;
}
else {
  return value.ToString();
}

Какая ошибка описана здесь, узнаете позже

Обычно я упрощаю методы, чтобы ошибку было легче заметить. Предлагаю сыграть в игру. Найдите ошибку в следующем фрагменте метода. Чтобы было интереснее, я не буду писать тип ошибки и упрощать весь код (и так предоставляю только часть метода).

Ну что, как успехи? Мне почему-то кажется, что большинство даже не пытались. Не буду вас мучить.

Предупреждение PVS-Studio: V3012 The ‘?:’ operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258

Вот он, злосчастный тернарный оператор:

button_pressed_highlight = use_system_colors ?
                             Color.FromArgb (150, 179, 225) : 
                             Color.FromArgb (150, 179, 225);

Независимо от значения переменной use_system_colors объекту button_pressed_highlight будет присвоено одно и то же значение. Если вы думаете, что такие ошибки сложно отследить, предлагаю вам взглянуть на файл целиком (ProfessionalColorTable.cs) и понять, что такие ошибки не просто сложно отследить самостоятельно — это просто нереально.

Подобных мест, кстати, весьма много (целых 32), что даже наводит на мысль о том, что это вовсе не ошибка, а таков замысел. Тем не менее, код выглядит странно, и я бы советовал его проверить. Даже если это не ошибка, а ожидаемая логика, проще ведь было бы использовать обыкновенное присваивание, чем писать странные тернарные операторы, сбивающие с толку. Остальные предупреждения V3012 приведены в файле.

Использование счетчика другого цикла

public override bool Equals (object obj)
{
  if (obj == null)
    return false;
  PermissionSet ps = (obj as PermissionSet);
  if (ps == null)
    return false;
  if (state != ps.state)
    return false;
  if (list.Count != ps.Count)
    return false;

  for (int i=0; i < list.Count; i++) {
    bool found = false;
    for (int j=0; i < ps.list.Count; j++) {
      if (list [i].Equals (ps.list [j])) {
        found = true;
        break;
      }
    }
    if (!found)
      return false;
  }
  return true; 
}

Предупреждение PVS-Studio: V3015 It is likely that a wrong variable is being compared inside the ‘for’ operator. Consider reviewing ‘i’ corlib-net_4_x PermissionSet.cs 607

Подозрительным выглядит условие выхода из вложенного цикла: i < ps.list.Count. В качестве счётчика цикла выступает переменная j, однако в условии выхода используется счетчик внешнего цикла — переменная i.

Намерения автора кода понятны — проверить, что в коллекциях содержатся одинаковые элементы. Но если получится, что какой-то элемент коллекции list не содержится в ps.list, выход из вложенного цикла не будет осуществлён с помощью оператора break. В то же время внутри этого же цикла не изменяется переменная i, т.е. выражение i < ps.list.Count неизменно будет иметь значение true. В итоге цикл будет выполняться до тех пор, пока не произойдет выход за границу коллекции (из-за постоянного инкремента счётчика j).

Проверка не той ссылки на null после приведения с использованием оператора as

Как оказалось, это типовая ошибка для языка C#. Она находится чуть ли не в каждом проекте, по которому написана статья. Как правило, диагностическое правило V3019 обнаруживает случаи следующего вида:

var derived = base as Derived;
if (base == null) {
  // do something
}
// work with 'derived' object

Проверка base == null спасёт только в том случае, если base действительно имеет значение null, и тогда нам неважно, удалось выполнить приведение или нет. Но очевидно, что подразумевалась проверка ссылки derived. И тогда, если base != null и не удалось выполнить преобразование, а дальше по методу идёт работа с членами объекта derived, будет сгенерировано исключение типа NullReferenceException.

Мораль: если вы используете данный паттерн, убедитесь, что проверяете на равенство null нужную сслыку.

Но это теория. Давайте посмотрим, что удалось обнаружить на практике:

public override bool Equals (object o)
{
  UrlMembershipCondition umc = (o as UrlMembershipCondition);
  if (o == null)
    return false;

  ....

  return (String.Compare (u, 0, umc.Url, ....) == 0); // <=
}

Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using ‘as’ keyword. Check variables ‘o’, ‘umc’. UrlMembershipCondition.cs 111

Паттерн — один в один описанный выше. Если тип объекта o несовместим с типом UrlMembershipCondition, и при этом объект o не равен null, то при попытке обращения к свойству umc.Url будет сгенерировано исключение типа NullReferenceException.

Соответственно, для исправления ошибки необходимо исправить проверку:

if (umc == null)
  return false;

Взглянем на ещё один ляп.

static bool QSortArrange (.... ref object v0, int hi, 
                          ref object v1, ....)
{
  IComparable cmp;
  ....
  cmp = v1 as IComparable;

  if (v1 == null || cmp.CompareTo (v0) < 0) {
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using ‘as’ keyword. Check variables ‘v1’, ‘cmp’. Array.cs 1487

Ситуация аналогична описанным выше. Разве что в случае неудачного приведения исключение NullReferenceException будет сгенерировано тут же — прямо при проверке выражения.

В общем-то, ситуация в других местах аналогична, так что просто приведу ещё 12 предупреждений в текстовом файле.

Безусловный выброс исключения

public void ReadEmptyContent(XmlReader r, string name)
{
  ....
  for (r.MoveToContent(); 
         r.NodeType != XmlNodeType.EndElement; 
           r.MoveToContent())
  {
    if (r.NamespaceURI != DbmlNamespace)
      r.Skip();
    throw UnexpectedItemError(r); // <=
  }
  ....
}

Предупреждение PVS-Studio: V3020 An unconditional ‘throw’ within a loop. System.Data.Linq-net_4_x XmlMappingSource.cs 180

На первой же итерации цикла будет сгенерировано исключение типа UnexpectedItemError. Как минимум, выглядит странно. К слову, Visual Studio подсвечивает объект r в секции изменения счётчика цикла с подсказкой о недостижимом коде. Но, вероятно, автор кода не использовал Visual Studio, или просто не заметил предупреждения, из-за чего ошибка осталась в коде.

Подозрительные операторы ‘if’

Периодически встречаются ошибки такого типа, когда в методе присутствуют 2 одинаковых оператора ‘if’, причём значения объектов, используемых в условных выражениях этих операторов, между ними не меняются. При истинности любого из этих условных выражений будет осуществлён выход из тела метода. Таким образом, второй оператор ‘if’ никогда не будет выполнен. Давайте рассмотрим фрагмент кода, в котором была допущена подобная ошибка:

public int LastIndexOfAny (char [] anyOf, int startIndex, int count)
{
  ....
  if (this.m_stringLength == 0)
    return -1;

  ....
  if (this.m_stringLength == 0)
    return -1;

  ....
}

Предупреждение PVS-Studio: V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless corlib-net_4_x String.cs 287

Выполнение метода никогда не дойдет до второго оператора if, приведённого в данном фрагменте, так как если this.m_stringLength == 0, выход будет осуществлён при выполнении первого условного оператора. Этот код был бы оправдан, если бы между операторами if изменялось бы значение поля m_stringLength, но это не так.

Следствия ошибки зависят от причины её возникновения:

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

Пример второго, более серьёзного случая ошибки, можно наблюдать в следующем фрагменте кода (нажмите на изображение для увеличения):

Заметить ошибку в этом коде не составит труда. Шучу, шучу, конечно составит. Но не для анализатора. Прибегнем к нашей стандартной методике упрощения кода, чтобы все стало понятнее:

private PaperKind GetPaperKind (int width, int height)
{
  ....
  if (width == 1100 && height == 1700)
    return PaperKind.Standard11x17;
  ....
  if (width == 1100 && height == 1700)
    return PaperKind.Tabloid;
  ....
}

Предупреждение PVS-Studio: V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless System.Drawing-net_4_x PrintingServicesUnix.cs 744

При истинности выражения width == 1100 && height == 1700 будет выполнен только первый оператор if. Однако значения, возвращаемые при истинности данного выражения, отличаются, так что нельзя просто так сказать, что второй оператор if лишний. Более того — скорее всего в его условии должно находиться другое выражение. Налицо нарушение логики работы программы.

Напоследок я хотел бы рассмотреть ещё один фрагмент кода с подобной ошибкой:

private void SerializeCore (SerializationStore store, 
                            object value, bool absolute)
{
  if (value == null)
    throw new ArgumentNullException ("value");
  if (store == null)
    throw new ArgumentNullException ("store");

  CodeDomSerializationStore codeDomStore = 
    store as CodeDomSerializationStore;
  if (store == null)
    throw new InvalidOperationException ("store type unsupported");

  codeDomStore.AddObject (value, absolute);
}

Предупреждение PVS-Studio: V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562

Данное предупреждение пересекается с предупреждением V3019, так как здесь явно наблюдается паттерн проверки на null после приведения с использованием оператора as не той ссылки. Но какое бы предупреждение выдано ни было — ошибка очевидна.

Встречались и другие подобные предупреждения:

• V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless Mono.Data.Sqlite-net_4_x SQLiteDataReader.cs 270
• V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless System.Web-net_4_x HttpUtility.cs 220
• V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562
• V3021 There are two ‘if’ statements with identical conditional expressions. The first ‘if’ statement contains method return. This means that the second ‘if’ statement is senseless Mono.Security.Providers.DotNet-net_4_x DotNetTlsProvider.cs 77

Подозрительные строки форматирования

Диагностическое правило V3025 обнаруживает ошибочно составленные строки форматирования. Это также тип ошибок, встречающийся во многих проверяемых проектах. Обнаруживаются ситуации 2-ух видов:

• строка форматирования ожидает параметров больше, чем их представлено;
• строка форматирования ожидает параметров меньше, чем их представлено.

В первом случае будет сгенерировано исключение типа FormatException, во втором случае неиспользуемые аргументы просто будут проигнорированы. Так или иначе, стоит уделить внимание таким местам и исправить код должным образом.

Конечно, я бы не стал вспоминать это диагностическое правило, если бы в коде не нашлись подобные ошибки.

static IMessageSink GetClientChannelSinkChain(string url, ....)
{
  ....
  if (url != null) 
  {
    string msg = String.Format (
      "Cannot create channel sink to connect to URL {0}. 
       An appropriate channel has probably not been registered.", 
      url); 
    throw new RemotingException (msg);
  }
  else 
  {
    string msg = String.Format (
      "Cannot create channel sink to connect to the remote object. 
       An appropriate channel has probably not been registered.", 
      url); 
    throw new RemotingException (msg);
  }
  ....
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling ‘Format’ function. Arguments not used: url. corlib-net_4_x RemotingServices.cs 700

Хочу обратить ваше внимание на вторую строку форматирования. Она представляет собой строковый литерал, в котором не предусмотрена подстановка аргументов (в отличии от строки форматирования выше). Тем не менее, метод Format принимает в качестве второго аргумента объект url. Из вышесказанного следует, что объект url просто будет проигнорирован при формировании новой строки, а информация о нем не попадет в текст исключения.

В C# 6.0 были добавлены интерполированные строки, которые в некоторых случаях помогут избежать проблем, связанных с использованием строк форматирования, в том числе — с несоответствующим количеством аргументов.

Рассмотрим ещё один фрагмент ошибочного кода.

public override string ToString ()
{
  return string.Format ("ListViewSubItem {{0}}", text);
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling ‘Format’ function. Arguments not used: text. System.Windows.Forms-net_4_x ListViewItem.cs 1287

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

"ListViewSubItem {{0}}"

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

public override string ToString ()
{
  return $"ListViewSubItem {{{text}}}",;
}

Или же, оставаясь верным методу String.Format, стоило добавить по одной фигурной скобке с каждой стороны. Тогда строка форматирования выглядела бы следующим образом:

"ListViewSubItem {{{0}}}"

И последний фрагмент кода со строками форматирования. Как всегда, самое интересное осталось на десерт:

void ReadEntropy ()
{
  if (reader.IsEmptyElement)
    throw new XmlException (
      String.Format ("WS-Trust Entropy element is empty.{2}", 
                      LineInfo ()));
  ....
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling ‘Format’ function. Format items not used: {2}. Arguments not used: 1st. System.ServiceModel-net_4_x WSTrustMessageConverters.cs 147

Не знаю, каким образом в строку форматирования затесался элемент форматирования с индексом ‘2’, но он приводит к весьма интересной ошибке. Планировалось выбросить исключение с каким-то текстом, составляемым строкой форматирования. И исключение будет сгенерировано. Исключение типа FormatException, ведь для текущей строки форматирования необходимы 3 аргумента (так как требуется именно третий), а представлен только один.

Если каким-то образом перепутали только номер запрашиваемого аргумента (например, в ходе рефакторинга), исправить ошибку не составит труда:

"WS-Trust Entropy element is empty.{0}"

Прочие подозрительные места, обнаруженные правилом V3025, приведены в файле.

Разыменование нулевой ссылки

private bool IsContractMethod (string methodName, 
                               Method m, 
                               out TypeNode genericArgument)
{
  ....
  return m.Name != null && m.Name == methodName &&
    (m.DeclaringType.Equals (this.ContractClass)     // <=
     || (m.Parameters    != null && 
         m.Parameters.Count == 3 && 
         m.DeclaringType != null &&                  // <=
         m.DeclaringType.Name != ContractClassName));
}

Предупреждение PVS-Studio: V3027 The variable ‘m.DeclaringType’ was utilized in the logical expression before it was verified against null in the same logical expression. Mono.CodeContracts-net_4_x ContractNodes.cs 211

Перед тем, как обратиться к свойству Name свойства DeclaringType, программист решил подстраховаться и проверить DeclaringType на неравенство null, чтобы случайно не разыменовать нулевую ссылку. Желание понятное и вполне законное — всё правильно. Вот только это все равно не возымеет действия, так как выше по коду у свойства DeclaringType вызывается экземплярный метод Equals, а значит, если DeclaringType == null, будут сгенерировано исключение типа NullReferenceException. Для решения проблемы можно, например, перенести проверку на неравенство null выше по коду, или использовать null-условный оператор (‘?.’), доступный в C# 6.0.

Другой случай.

Node leftSentinel;
....
IEnumerator<T> GetInternalEnumerator ()
{
  Node curr = leftSentinel;
  while ((curr = curr.Nexts [0]) != rightSentinel && curr != null) {
    ....
  }
}

Предупреждение PVS-Studio: V3027 The variable ‘curr’ was utilized in the logical expression before it was verified against null in the same logical expression. Mono.Parallel-net_4_x ConcurrentSkipList.cs 306

И опять та же ситуация. Если curr == null, будет осуществлён выход из цикла. Но вот если curr изначально имел значение null (т.е. на момент выполнения кода leftSentinel == null), опять получим исключение NullReferenceException.

Избыточная проверка

В проверяемых проектах периодически встречаются выражения следующего вида или им подобные:

!aa || (aa && bb)

Их можно упростить до выражения следующего вида:

!aa || bb

Возможно, в некоторых случаях вы получите выигрыш в производительности (пусть и незначительный), но также второй вариант читается легче при том, что он логически эквивалентен первому (в случае, если подвыражение аa не меняется между вызовами).

Возможно, что вместо aa должно было находиться другое подвыражение:

!aa || (cc && bb)

Тогда речь идёт о настоящей ошибке. Так или иначе, в анализаторе PVS-Studio присутствует диагностическое правило V3031, обнаруживающее подобные случи. Рассмотрим несколько фрагментов кода, которые удалось обнаружить с его помощью:

public void Emit(OpCode opc)
{
  Debug.Assert(opc != OpCodes.Ret || (
               opc == OpCodes.Ret && stackHeight <= 1));
  ....
}

Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The ‘||’ operator is surrounded by opposite expressions. mcs-net_4_x ILGenerator.cs 456

Избыточный код. Обращение к объекту opc не изменяет его значения, так что данное выражение можно упростить:

Debug.Assert(opc != OpCodes.Ret || stackHeight <= 1));

Другой фрагмент кода:

public bool Validate (bool checkAutoValidate)
{
  if ((checkAutoValidate && (AutoValidate != AutoValidate.Disable)) ||
      !checkAutoValidate)
    return Validate ();

  return true;
}

Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The ‘||’ operator is surrounded by opposite expressions. System.Windows.Forms-net_4_x ContainerControl.cs 506

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

!checkAutoValidate || (AutoValidate != AutoValidate.Disable)

Некоторые отобранные мной предупреждения приведены в файле.

Форматирование кода, не соответствующее логике

При разработке диагностических правил, подобных V3033, были споры о том, насколько они актуальны. Дело в том, что диагностики, завязанные на форматировании кода, весьма специфичны, так как многие редакторы/среды разработки (та же Visual Studio) сами форматируют код уже по ходу его написания. Следовательно, вероятность допустить такую ошибку весьма мала. Я редко встречал в проверяемых проектах такие ошибки, но в ‘Mono’ парочка всё же нашлась.

public bool this [ string header ] {
  set {
      ....
      if (value)
        if (!fields.Contains (header))
          fields.Add (header, true);
      else
        fields.Remove (header);
  }
}

Предупреждение PVS-Studio: V3033 It is possible that this ‘else’ branch must apply to the previous ‘if’ statement. HttpCacheVaryByHeaders.cs 159

Код отформатирован так, что может показаться, будто else относится к первому оператору if. Но компилятору совершенно без разницы, как отформатирован ваш код, поэтому он расшифрует данный фрагмент по-своему, ассоциировав else со вторым оператором if, как и положено. Налицо интересная ошибка. Код, отформатированный в соответствии с заданной логикой должен выглядеть так:

if (value)
  if (!fields.Contains (header))
    fields.Add (header, true);
  else
    fields.Remove (header);

Аналогичное предупреждение встретилось ещё раз: V3033 It is possible that this ‘else’ branch must apply to the previous ‘if’ statement. HttpCacheVaryByParams.cs 102

К этой же категории можно отнести ещё одно диагностическое правило — V3043.

Ошибочный код:

public void yyerror (string message, string[] expected) {
  ....
  for (int n = 0; n < expected.Length; ++ n)
    ErrorOutput.Write (" "+expected[n]);
    ErrorOutput.WriteLine ();
  ....
}

Предупреждение PVS-Studio: V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. cs-parser.cs 175

Если судить по форматированию кода (забыв про правила языка программирования), можно подумать, что оба вызова метода (Write и WriteLine) относятся к оператору for. На самом деле в цикле будет вызываться только метод Write. Этот код точно в чём-то ошибочен! Если подразумевалась такая логика (казалось бы, всё логично — выводятся элементы, после чего вставляется пустая строка), к чему форматирование, вводящее в заблуждение? С другой стороны — бегло просматривая код, можно сразу и не понять истинную логику оператора. Не просто так же программисты придерживаются определённых стилей форматирования.

private string BuildParameters ()
{
  ....
  if (result.Length > 0)
    result.Append (", ");
    if (p.Direction == TdsParameterDirection.InputOutput) // <=
      result.Append (String.Format("{0}={0} output",     
                                   p.ParameterName));
    else
  result.Append (FormatParameter (p));
  ....
}

Предупреждение PVS-Studio: V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Tds50.cs 379

Второй оператор if никак не относится к первому. Зачем тогда вводить в заблуждение людей, работающих с этим кодом?

public void Restore ()
{
  while (saved_count < objects.Count)
    objects.Remove (objects.Last ().Key);
    referenced.Remove (objects.Last ().Key);
  saved_count = 0;
  referenced.RemoveRange (saved_referenced_count, 
                          referenced.Count - saved_referenced_count);
  saved_referenced_count = 0;
}

Предупреждение PVS-Studio: V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. XamlNameResolver.cs 81

Судя по всему, планировалось удалить из коллекций objects и referenced значения, соответствующие определённому ключу. Но забыли про фигурные скобки, в итоге из коллекции referenced будет удалено только одно значение. Что ещё интереснее — одними фигурными скобками тут не отделаться, так как в таком случае на каждой итерации цикла из коллекции referenced будет удаляться объект не по тому ключу, по которому происходило удаление из коллекции objects. Это связано с тем, что на момент вызова метода Remove для коллекции referenced коллекция objects уже будет изменена, а значит метод Last вернёт другой элемент.

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

• V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ExpressionParser.cs 92
• V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. EcmaUrlParser.cs 80
• V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ILParser.cs 167

Приведение объекта к собственному типу / проверка объекта на совместимость с собственным типом

За поиск подобных ситуаций отвечает диагностическое правило V3051. Как правило, оно находит избыточный код вида

String str;
String str2 = str as String;

или

String str;
if (str is String)

Но встречаются (хотя и редко) куда более интересные случаи.

Рассмотрим фрагмент кода:

public string GenerateHttpGetMessage (Port port, 
                                      OperationBinding obin, 
                                      Operation oper, 
                                      OperationMessage msg)
{
  ....
  MimeXmlBinding mxb = 
    (MimeXmlBinding) obin.Output
                         .Extensions
                         .Find (typeof(MimeXmlBinding)) 
      as MimeXmlBinding;
  if (mxb == null) return req;
  ....
}

Предупреждение PVS-Studio: V3051 An excessive type cast. The object is already of the ‘MimeXmlBinding’ type. SampleGenerator.cs 232

Казалось бы — лишнее приведение, ну и что? Ниже проверяем mxb на равенство null, так что, если тип несовместим — ничего страшного. Не тут-то было. Метод Find возвращает экземпляр типа Object, после чего его явно приводят к типу MimeXmlBinding и уже после этого выполняют приведение к этому же типу с использованием оператора as. Однако оператор явного приведения, в случае, если аргумент имеет несовместимый тип, не возвращает null (в отличии от оператора as), а генерирует исключение типа InvalidCastException. В итоге проверка mxb == null никак не поможет при неудачном приведении типов.

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

Параметр перезаписывается в теле метода до того, как используется

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

internal static int SetErrorInfo (int dwReserved, 
                                  IErrorInfo errorInfo)
{
  int retVal = 0;
  errorInfo = null;

  ....
  retVal = _SetErrorInfo (dwReserved, errorInfo);
  ....
}

Предупреждение PVS-Studio: V3061 Parameter ‘errorInfo’ is always rewritten in method body before being used. corlib-net_4_x Marshal.cs 1552

Значение, пришедшее в качестве параметра errorInfo, никак не используется. Параметр сразу обнуляется, а затем передаётся в метод. В таком случае логично было бы сделать errorInfo локальной переменной (если возможно изменение сигнатуры метода).

Остальные места схожи, поэтому, как и раньше, привожу перечень предупреждений списком в файле.

Неверная инициализация статических членов

class ResXResourceWriter : IResourceWriter, IDisposable
{
  ....
  public static readonly string ResourceSchema = schema;
  ....
  static string schema = ....;
  ....
}

Предупреждение PVS-Studio: V3070 Uninitialized variable ‘schema’ is used when initializing the ‘ResourceSchema’ variable. ResXResourceWriter.cs 59
Программист хотел задать значение публичного статического поля ResourceSchema, доступного только для чтения, равным другому статическому полю — schema. Без ошибки не обошлось. На момент инициализации ResourceSchema поле schema будет проинициализировано значением по умолчанию (null в данном случае). Маловероятно, что именно этого разработчик и добивался.

Ошибочная инициализация статического поля, декорированного атрибутом [ThreadStatic]

Редкая и интересная ошибка. Рассмотрим фрагмент кода:

static class Profiler
{
  [ThreadStatic]
  private static Stopwatch timer = new Stopwatch();
  ....
}

Предупреждение PVS-Studio: V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16

Декорирование поля атрибутом [ThreadStatic] означает, что в каждом потоке значение этого поля будет уникальным. Вроде бы ничего сложного. Но дело в том, что такие поля нельзя инициализировать ни при объявлении, ни в статическом конструкторе. Это отличный способ выстрелить себе в ногу (или даже обе) и получить трудноуловимую ошибку.

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

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

Пересекающиеся диапазоны

public void EmitLong (long l)
{
  if (l >= int.MinValue && l <= int.MaxValue) {
    EmitIntConstant (unchecked ((int) l));
    ig.Emit (OpCodes.Conv_I8);
  } else if (l >= 0 && l <= uint.MaxValue) {
    EmitIntConstant (unchecked ((int) l));
    ig.Emit (OpCodes.Conv_U8);
  } else {
    ig.Emit (OpCodes.Ldc_I8, l);
  }
}

Предупреждение PVS-Studio: V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. mcs-net_4_x codegen.cs 742

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

• l >= int.MinValue && l <= int.MaxValue
• l >= 0 && l <= uint.MaxValue

Для обоих этих выражений общим является диапазон [0, Int32.MaxValue], так что если переменная l имеет значение, лежащее в этом диапазоне, будет выполнено первое условие, несмотря на то, что второе тоже могло бы выполниться.

Схожие предупреждения:

• V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP51932.cs 437
• V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP932.cs 552
• V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x ISO2022JP.cs 460

Доступ к элементу коллекции по константному индексу, осуществляемый внутри цикла

Практика давать счётчикам циклов короткие имена распространена повсеместно. Нет ничего плохого в том, чтобы назвать счётчик цикла i или j — сразу понятно, в качестве чего выступает эта переменная (за исключением тех случаев, когда счётчики требуют все же более осмысленных имён). Но бывают случаи, когда в качестве индекса используют не счётчик цикла, и числовой литерал. Понятно, что иногда это делается специально, но порой это свидетельствует об ошибке. Диагностическое правило V3102 ищет подобные ошибочные места.

public void ConvertGlobalAttributes (
  TypeContainer member, 
  NamespaceContainer currentNamespace, 
  bool isGlobal)
{
  var member_explicit_targets = member.ValidAttributeTargets;
  for (int i = 0; i < Attrs.Count; ++i) {
    var attr = Attrs[0];
    if (attr.ExplicitTarget == null)
      continue;
    ....
  }
}

Предупреждение PVS-Studio: V3102 Suspicious access to element of ‘Attrs’ object by a constant index inside a loop. mcs-net_4_x attribute.cs 1272

На каждой итерации цикла переменная attr инициализируется одним и тем же значением — Attrs[0]. В дальнейшем с ней осуществляется некоторая работа (вызываются свойства, передаётся в метод). Навряд ли на всех итерациях цикла хотели работать с одним и тем же значением, так что, полагаю, правильная инициализация должна выглядеть следующим образом:

var attr = Attrs[i];

Схожие ошибки встретились ещё в 2-ух местах:

• V3102 Suspicious access to element of ‘seq’ object by a constant index inside a loop. System.Xml-net_4_x XmlQueryRuntime.cs 679
• V3102 Suspicious access to element of ‘state’ object by a constant index inside a loop. System.Web-net_4_x Login.cs 1223

Небезопасные блокировки

Во многих проверяемых проектах приходится сталкиваться с кодом вида lock(this) или lock(typeof(….)). Это не лучший способ блокировки, так как он может привести к возникновению взаимоблокировки. Но давайте обо всем по порядку. Для начала посмотрим на опасный код:

public RegistryKey Ensure (....)
{
  lock (typeof (KeyHandler)){
    ....
  }
}

Предупреждение PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 245

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

В чем же проблема с оператором typeof? Данный оператор всегда возвращает экземпляр типа Type, причем один и тот же для одного и того же аргумента, следовательно, нарушается правило, описанное выше — возникает проблема блокировки по одному и тому же объекту.

В коде проекта встретилось несколько таких мест:

• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 245
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 261
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 383
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 404
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 451
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 469
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 683
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. corlib-net_4_x UnixRegistryApi.cs 698
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. System-net_4_x NetworkChange.cs 66
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. System-net_4_x NetworkChange.cs 74
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. System-net_4_x NetworkChange.cs 85
• V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. System-net_4_x NetworkChange.cs 93

Ситуация с методом GetType() принципиально ничем не отличается от вышеописанной:

void ConfigureHttpChannel (HttpContext context)
{
  lock (GetType())
  {
    ....
  }
}

Предупреждение PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same ‘Type’ object. System.Runtime.Remoting-net_4_x HttpRemotingHandlerFactory.cs 61

Метод GetType() также возвращает экземпляр типа Type, поэтому, если в другом месте блокировка будет осуществляться с использованием метода GetType() или оператора typeof, применительно к объекту того же типа — возникает вероятность взаимоблокировки.

Отдельно хотелось бы рассмотреть блокировку по объектам типа String, так как в этом случае ситуация становится куда интереснее, а ошибки — более вероятными и трудными в обнаружении.

const string Profiles_SettingsPropertyCollection = 
               "Profiles.SettingsPropertyCollection";
....
static void InitProperties ()
{
  ....
  lock (Profiles_SettingsPropertyCollection) {
  if (_properties == null)
    _properties = properties;
  }
}

Предупреждение PVS-Studio: V3090 Unsafe locking on an object of type ‘String’. System.Web-net_4_x ProfileBase.cs 95

Суть проблемы остается неизменной — блокировка по одному и тому же объекту, а вот детали уже более интересны. Доступ к объектам типа String можно получить даже из другого домена приложения (это связано с механизмом интернирования строк). Представьте, каково отлаживать взаимоблокировку, возникшую в результате того, что в разных доменах приложений использовалась одна и та же строка в качестве объекта блокировки. Совет краток — не используйте в качестве объектов блокировки объекты типа String (и Thread, к слову, тоже). Эти и другие проблемы, связанные с использованием механизмов синхронизации (а также способы их избежать), описаны в документации к диагностическому правилу V3090.

Ошибочное сравнение

public bool Equals (CounterSample other)
{
  return
    rawValue == other.rawValue &&
    baseValue == other.counterFrequency &&
    counterFrequency == other.counterFrequency &&
    systemFrequency == other.systemFrequency &&
    timeStamp == other.timeStamp &&
    timeStamp100nSec == other.timeStamp100nSec &&
    counterTimeStamp == other.counterTimeStamp &&
    counterType == other.counterType;
}

Предупреждение PVS-Studio: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression ‘baseValue == other.counterFrequency’. System-net_4_x CounterSample.cs 139

Я умышленно не стал изменять форматирование кода, оставив его в таком же виде, каким оно было в первоисточнике. Замысел метода понятен и проблем для понимания не вызывает — просто сравниваются поля текущего объекта и объекта, пришедшего в качестве аргумента. Но даже такой, казалось бы, простой метод содержит ошибку. Уверен, что если бы форматирование кода было выполнено лучше, заметить её было бы проще. Так выглядит этот же фрагмент кода, но отформатированный:

 public bool Equals (CounterSample other)
{
  return
    rawValue         == other.rawValue         &&
    baseValue        == other.counterFrequency && // <=
    counterFrequency == other.counterFrequency && // <=
    systemFrequency  == other.systemFrequency  &&
    timeStamp        == other.timeStamp        &&
    timeStamp100nSec == other.timeStamp100nSec &&
    counterTimeStamp == other.counterTimeStamp &&
    counterType      == other.counterType;
}

Думаю, с таким форматированием ошибку заметить куда проще — видно, что с разными полями текущего объекта сравнивается одно и то же поле сравниваемого. Как следствие — нарушается логика работы программы.
Из этого фрагмента кода хорошо видно — форматирование играет важную роль! И если компилятору безразлично, как оформлен ваш код, для программистов это важно и позволит избежать досадных ошибок, а также упростит восприятие кода и понимание логики работы программы.

Как всё это править?

Хорошо, ошибки нашли. Много ошибок. Если посчитать ошибки, описанные в статье, а также те, которые были приведены в файлах, выходит 167 штук! Хочу отметить, что это далеко не все ошибочные / подозрительные места — я просто выбрал достаточно материала для статьи (что не составило проблем). Многие другие ошибки, пожалуй, даже большинство, оставил за рамками статьи, потому что она и так вышла достаточно объёмной.

Может возникнуть резонный вопрос, как всё это исправлять? Как внедрить статический анализатор в проект?

Маловероятно, что будет выделена отдельная команда, которая будет заниматься исключительно исправлением ошибок (разве что мы сами эти ошибки и будем править, как было в случае с Unreal Engine). Правильно будет зафиксировать имеющиеся ошибки, которые будут постепенно исправляться, и не допускать появления новых.

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

Режим инкрементального анализа призван решить вторую проблему. Этот режим запускает анализ на машине разработчика сразу же после компиляции, позволяя обнаруживать ещё свежие ошибки и исправлять их до попадания в систему контроля версий (СКВ).

Так или иначе, скорее всего какой-то процент ошибок попадёт в СКВ. Чтобы обнаруживать ошибочный код максимально быстро после попадания в СКВ, хорошим решением будет внедрение статического анализа в ночные сборки и использование результатов анализа. Каким образом? Можно, например, оповещать человека, ответственного за проект, а также программиста, который допустил попадание этой ошибки в репозиторий.

Важно исправлять такие ошибки незамедлительно, пока они не ‘обросли’ дополнительным кодом, и задача исправления не усложнилась в несколько раз.

Сочетая описанные выше советы (внедрение статического анализа как на билд-сервере, так и на машинах разработчиков) позволит сократить расходы на исправление ошибок, так как при правильной организации процесса они будут исправляться максимально быстро (не доходя до тестеров и уж тем более, не попадая в релизы).

Подробнее про внедрение статического анализа в процесс разработки можно прочитать в статье «Как быстро внедрить статический анализ в большой проект?».

Заключение

В комментариях к одной из статей писали: «Вы пишете, что проверяете open-source продукты вашим анализатором, но на самом деле вы проверяете ваш анализатор open-source продуктами!». В этом есть истина.

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

Но мы все же проверяем проекты и, что важно, находим там реальные ошибки. Не просто находим, а стараемся донести эту информацию до разработчиков и всех интересующихся. А уж как этой информацией пользоваться — личное дело каждого. Думаю, польза от нашей работы для open-source сообщества однозначно есть. Не так давно мы перевалили за отметку 10,000 найденных ошибок!

А вообще, наша главная цель — популяризация методологии статического анализа в целом и анализатора PVS-Studio в частности. И наши статьи являются отличным способом демонстрации пользы этой методологии.

Попробуйте PVS-Studio на своём проекте: http://www.viva64.com/ru/pvs-studio/

Конечно, немного грустно, что не удалось проверить скомпилированный проект, из-за чего действительно полного и глубокого анализа не вышло. С другой стороны — материала на статью хватило и без этого, так что разработчикам стоит задуматься об исправлении ошибок и внедрении статического анализа в процесс разработки. А мы всегда рады им в этом помочь.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Searching for bugs in Mono: there are hundreds of them!.

• Введение
• RavenDB
• Command Line Interface
• Rider
• А на десерт
  • Всего лишь дополнительная проверка?
  • Типичный null
  • Снова лишняя проверка?
  • Возможный NRE
  • А если копнуть глубже?
  • Традиционный copy-paste
  • Ох уж этот «??»
  • Передача параметров
  • Never continue
  • Доверяй, но проверяй
  • Nullable, который никогда не null
  • Странности
  • Не тот логический оператор
  • Странный try-метод
  • null != null?
  • First or null
  • Always true
• Заключение

PVS-Studio – известный статический анализатор кода, позволяющий найти множество каверзных ошибок, скрытых в исходниках. Недавно завершился бета-тест новой версии, в которой появилась возможность анализа C# проектов под Linux и macOS. Кроме того, теперь анализатор можно интегрировать с кроссплатформенной IDE от JetBrains – Rider. Данная статья позволит познакомиться с этими возможностями на примере проверки open source проекта RavenDB.

Введение

Некоторое время назад мой коллега Сергей Васильев написал заметку об открытии бета-теста новой версии разрабатываемого нами статического анализатора PVS-Studio. Сейчас бета-тест завершён и новую версию можно загрузить, перейдя по ссылке. В этой же статье мы рассмотрим, как проводится анализ C# проектов под Linux/macOS с использованием консольного интерфейса и Rider. После этого проведём традиционный разбор некоторых интересных срабатываний анализатора.

RavenDB

Для проверки был выбран открытый проект RavenDB, репозиторий которого насчитывает почти 5 тысяч файлов исходного кода. Он представляет собой достаточно популярную NoSQL-базу данных. Подробности можно узнать на сайте. Нетрудно догадаться, что меня привлёк его размер, ведь в столь большом и, вне всяких сомнений, серьёзном проекте наверняка найдётся что-нибудь интересное.

Command Line Interface

Для начала рассмотрим, как производится анализ через консоль. Этот раздел, на мой взгляд, будет особенно интересен тем, кто желает интегрировать анализатор в CI-систему. У команды, запускающей анализ, есть ряд интересных опций, однако в общем случае всё достаточно тривиально. Чтобы провести анализ RavenDB, я перехожу в папку с проектом и ввожу в консоли следующее:

pvs-studio-dotnet -t ./RavenDB.sln

Флаг -t (сокращение от target) служит для указания файла решения или проекта для проверки. Представленная выше строка запускает анализ и в результате работы формирует файл, содержащий найденные ошибки. Всё просто, не правда ли?

Rider

Работа с анализатором в Rider представляет собой примерно то же самое, что и в Visual Studio. Простой и понятный интерфейс плагина позволит проверить проект буквально в пару кликов. Это не преувеличение – для проведения анализа RavenDB мне было достаточно кликнуть в верхнем меню Tools, навести на «PVS-Studio» и кликнуть «Check Current Solution/Project».

Результаты анализа будут отображены в нижней части окна на вкладке PVS-Studio (ну а на какой же ещё? )

Как и в случае с плагином для Visual Studio, двойной клик по предупреждению отобразит место, с которым оно связано. Всё удобно и понятно.

Что ещё более важно, инструмент PVS-Studio не просто указывает на ошибки, а имеет инфраструктуру, позволяющую легко внедрить методологию статического анализа даже в большой старый проект.

Общая идея в следующем. Пользователь запустил анализатор и получил множество предупреждений. Раз проект, разрабатываемый много лет, жив, развивается и приносит деньги, то, скорее всего, в отчёте не будет много предупреждений, указывающих на критические дефекты. Другими словами, критические баги так или иначе уже поправлены более дорогими способами или благодаря фидбеку от клиентов. Соответственно, всё, что сейчас находит анализатор, можно считать техническим долгом, который непрактично стараться устранить сразу. Рационально пока игнорировать эти предупреждения, но писать новый код уже выполняя регулярный анализ.

Можно указать PVS-Studio считать эти предупреждения пока неактуальными (отложить технический долг на потом), и больше их не показывать. Анализатор создаёт специальный файл, где сохраняет информацию о пока неинтересных ошибках. И теперь PVS-Studio будет выдавать предупреждения только на новый или измененный код. Причем, всё это реализовано умно. Если, например, в начало некоего файла будет добавлена пустая строка, то анализатор понимает, что, по сути, ничего не изменилось, и по-прежнему будет молчать. Этот файл разметки можно заложить в систему контроля версий. Файл большой, но это не страшно, так как часто его закладывать смысла нет.

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

Для подавления предупреждений на существующий код в Rider необходимо просто перейти в верхнем меню в Tools->PVS-Studio и кликнуть «Suppress All Messages».

В появившемся окне, предупреждающем о том, что в suppress-лист попадут все текущие срабатывания, кликаем на «Ok». В результате в папке с проектом будет создан suppress-файл, который будет учитываться анализатором при дальнейшей работе.

Надо отметить, что в Rider уже есть анализатор, который успешно подсвечивает некоторые ошибки. Таким образом, ряд предупреждений PVS-Studio указывает на код, который выглядит подозрительно и с точки зрения редактора. Тем не менее, PVS-Studio довольно часто находит ошибки, которые смогли уйти от чуткого взора анализатора от JetBrains. Именно поэтому наиболее эффективным решением будет позволить им работать в команде.

А на десерт

Теперь, как и обещал, рассмотрим, какие интересные предупреждения отобразил анализатор по результатам проверки. Проект содержит огромное количество файлов исходного кода, поэтому неудивительно было найти в нём множество подозрительных моментов. Ничего не поделаешь – все совершают ошибки, однако важно прикладывать максимум усилий к их своевременному обнаружению и исправлению. Статический анализ позволяет существенно упростить эту задачу.

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

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

Конечно, не все срабатывания указывают на супер-страшные ошибки. Если бы это было так, вряд ли бы в проекте хоть что-то работало :). Но важно понимать – раз анализатор «ругается», значит код выглядит странно, и его стоит внимательно изучить.

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

Всего лишь дополнительная проверка?

public static void EnsurePathExists(string file)
{
  var dirpath = Path.GetDirectoryName(file);
  List<string> dirsToCreate = new List<string>();
  while (Directory.Exists(dirpath) == false)
  {
    dirsToCreate.Add(dirpath);
    dirpath = Directory.GetParent(dirpath).ToString();
    if (dirpath == null)                                  // <=
      break;
  }
  dirsToCreate.ForEach(x => Directory.CreateDirectory(x));
}

Предупреждение анализатора: V3022 Expression ‘dirpath == null’ is always false. PosixHelper.cs(124) Voron

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

Возможно, разработчик и правда не знал, что ToString никогда не вернёт null. Если же это не так, то можно сделать предположение о том, чего он хотел добиться.

Быть может, break должен вызываться, когда не удаётся получить родителя для рассматриваемой директории. Если это так, то проверка на null обретает смысл. Однако рассматривать нужно не результат работы ToString, а значение, возвращаемое методом GetParent:

dirsToCreate.Add(dirpath);
var dir = Directory.GetParent(dirpath);    
if (dir == null)
  break;

dirpath = dir.ToString();

В противном случае, возвращение null методом GetParent приводит к исключению при вызове ToString.

Типичный null

public long ScanOldest()
{
  ....
  for (int i = 0; i < copy.Length; i++)
  {
    var item = copy[i].Value;
    if (item != null || item == InvalidLowLevelTransaction) // <=
    {
      if (val > item.Id)                                    // <=
        val = item.Id;
    }
  }
  ....
}

Предупреждение анализатора: V3125 The ‘item’ object was used after it was verified against null. Check lines: 249, 247. ActiveTransactions.cs(249), ActiveTransactions.cs(247) Voron

Код выглядит странно из-за происходящего в том случае, когда item действительно null. Действительно, если InvalidLowLevelTransaction тоже окажется null, условие окажется истинным и попытка получения item.Id приведёт к выбрасыванию исключения. А если InvalidLowLevelTransaction не может оказаться null, то условие «item == InvalidLowLevelTransaction» попросту лишнее. Всё потому, что оно проверяется лишь в случае, когда item == null. Ну а если вдруг item не может быть null, то вообще всё условие теряет смысл и лишь добавляет ненужную вложенность.

Я думаю, что здесь, возможно, выбран неверный логический оператор. Если заменить в условии «||» на «&&», то код сразу начинает выглядеть логично. Ну и никаких проблем в таком случае возникнуть не может.

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

Снова лишняя проверка?

public void WriteObjectEnd()
{
  ....
  if (_continuationState.Count > 1)
  {
    var outerState = 
      _continuationState.Count > 0 ? _continuationState.Pop() : currentState
    ;
    if (outerState.FirstWrite == -1)
      outerState.FirstWrite = start;
    _continuationState.Push(outerState);
  }  
   ....
}

Предупреждение анализатора: V3022 Expression ‘_continuationState.Count > 0’ is always true. ManualBlittableJsonDocumentBuilder.cs(152) Sparrow

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

Возможный NRE

protected override Expression VisitIndex(IndexExpression node)
{
  if (node.Object != null)
  {
    Visit(node.Object);
  }
  else
  {
    Out(node.Indexer.DeclaringType.Name); // <=
  }
  if (node.Indexer != null)               // <=
  {
    Out(".");
    Out(node.Indexer.Name);
  }
  VisitExpressions('[', node.Arguments, ']');
  return node;
}

Предупреждение анализатора: V3095 The ‘node.Indexer’ object was used before it was verified against null. Check lines: 1180, 1182. ExpressionStringBuilder.cs(1180), ExpressionStringBuilder.cs(1182) Raven.Client

На самом деле, это ещё одно место, которое считает подозрительным и PVS-Studio, и Rider. Правда, формулировки несколько отличаются: анализатор от JetBrains просто подсвечивает node.Indexer.DeclaringType с комментарием «Possible NullReferenceException».

Оба проверяющих утверждают, что в этом фрагменте действительно может быть выброшено исключение. Должен заметить, что предупреждение от PVS-Studio не только говорит, что тут возможна ошибка, но и объясняет, почему у него сложилось такое мнение. Вроде мелочь, а приятно.

На самом деле, не факт, что тут и правда допущена ошибка. Вполне возможно, что если node.Object == null, то node.Indexer точно задан. При этом допустима ситуация, когда node.Object и node.Indexer оба не равны null. Только в таком случае можно считать данное срабатывание обоих анализаторов ложным. Поэтому стоит внимательно проанализировать все возможные варианты.

А если копнуть глубже?

private async Task LoadStartingWithInternal(....)
{
  ....
  var command = operation.CreateRequest();
  if (command != null)                       // <=
  {
    await RequestExecutor
      .ExecuteAsync(command, Context, SessionInfo, token)
      .ConfigureAwait(false)
    ;

    if (stream != null)
      Context.Write(stream, command.Result.Results.Parent);
    else
      operation.SetResult(command.Result);
  }
  ....
}

Предупреждение анализатора: V3022 Expression ‘command != null’ is always true. AsyncDocumentSession.Load.cs(175) Raven.Client

Предупреждение выдаётся из-за того, что метод CreateRequest никогда не возвращает null. В самом деле, достаточно взглянуть на его код, чтобы убедиться в этом:

public GetDocumentsCommand CreateRequest()
{
  _session.IncrementRequestCount();
  if (Logger.IsInfoEnabled)
    Logger.Info(....);

  return new GetDocumentsCommand(....);
}

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

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

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

public void IncrementRequestCount()
{
  if (++NumberOfRequests > MaxNumberOfRequestsPerSession)
    throw new InvalidOperationException(....);
}

Затем – в конструкторе GetDocumentsCommand:

public GetDocumentsCommand(string startWith, ....)
{
  _startWith = startWith ?? throw new ArgumentNullException(nameof(startWith));
  ....
}

Традиционный copy-paste

public override void WriteTo(StringBuilder writer)
{
  ....
  if (SqlConnectionStringsUpdated)
    json[nameof(SqlConnectionStringsUpdated)] = SqlConnectionStringsUpdated;

  if (ClientConfigurationUpdated)
    json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;

  if (ConflictSolverConfigUpdated)
    json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;

  if (PeriodicBackupsUpdated)
    json[nameof(PeriodicBackupsUpdated)] = PeriodicBackupsUpdated;

  if (ExternalReplicationsUpdated)
    json[nameof(ExternalReplicationsUpdated)] = ExternalReplicationsUpdated;
  ....
}

Предупреждение анализатора: V3127 Two similar code fragments were found. Perhaps, this is a typo. SmugglerResult.cs(256), SmugglerResult.cs(253) Raven.Client

Сильно сомневаюсь, что хоть кто-нибудь увидел бы тут странность, взглянув на код. Функция состоит из 14 подобных условий и все переменные заканчиваются на Updated. Даже когда тут приведена малая её часть, ошибку видно не сразу.

Человеческий мозг буквально отказывается искать в таком коде что-либо. В то же время, PVS-Studio с легкостью обнаружил, что присваивается, скорее всего, совершенно не то, должно:

if (ClientConfigurationUpdated)
    json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;

if (ConflictSolverConfigUpdated)
    json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;

Логично, что в нижней строке справа от оператора присваивания должно быть ConflictSolverConfigUpdated. Я уверен, что без статического анализа данная странность была бы найдена только в случае, если бы из-за неё сломалось что-то достаточно серьёзное. Программист сможет заметить, что в этой функции скрыта проблема, разве что только если будет знать об этом заранее.

Ох уж этот «??»

public int Count => 
  _documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count ?? 0;

Предупреждение анализатора: V3123 Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. InMemoryDocumentSessionOperations.cs(1952) Raven.Client

Конечно, сохраняется возможность, что это вовсе не ошибка, что так всё и задумано. И всё же это место выглядит очень подозрительно. Ведь логично предположить, что идея функции состоит вовсе не в том, чтобы возвращать 0 при _onBeforeStoreDocumentsByEntity == null.

Я думаю, что здесь действительно присутствует ошибка, связанная с приоритетами операторов. В этом случае, необходимо поставить скобки:

_documentsByEntity.Count + (_onBeforeStoreDocumentsByEntity?.Count ?? 0)

Ну а если вдруг и правда всё так и задумано, то, возможно, стоит указать на это явно – тогда у анализатора и программистов, читающих код, вопросов не возникнет:

(_documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count) ?? 0

Но это уже дело вкуса, конечно.

Передача параметров

private static void UpdateEnvironmentVariableLicenseString(....)
{
  ....
  if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
    return;
  ....
}

Предупреждение анализатора: V3066 Possible incorrect order of arguments passed to ‘ValidateLicense’ method: ‘newLicense’ and ‘oldLicense’. LicenseHelper.cs(177) Raven.Server

Аргументы переданы в метод в подозрительном порядке. В самом деле, взглянем на объявление:

private static bool ValidateLicense(
  License oldLicense, 
  RSAParameters rsaParameters, 
  License newLicense
)

Весьма приятно, что PVS-Studio способен находить даже такие ошибки. Это отличный пример к вопросу о преимуществах статического анализа перед динамическим.

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

Never continue

private List<CounterOperation> GetCounterOperationsFor(RavenEtlItem item)
{
  ....
  for (var i = 0; i < counters.Count; i++)
  {
    counters.GetPropertyByIndex(i, ref prop);

    if (
      GetCounterValueAndCheckIfShouldSkip(
        item.DocumentId, 
        null, 
        prop, 
        out long value, 
        out bool delete
      )
    ) continue;
    ....
  }
  ....
}

Предупреждение анализатора: V3022 Expression ‘GetCounterValueAndCheckIfShouldSkip(item.DocumentId, null, prop, out long value, out bool delete)’ is always false. RavenEtlDocumentTransformer.cs(362) Raven.Server

Полностью данный метод можно рассмотреть, перейдя по ссылке.

Предупреждение говорит о том, что вызов continue в этом цикле недостижим. И если это так, то фрагмент действительно странный. Но может это просто ложное срабатывание? Всё же Rider по этому поводу не ругается.

Взглянем на метод GetCounterValueAndCheckIfShouldSkip:

private bool GetCounterValueAndCheckIfShouldSkip(
  LazyStringValue docId, 
  string function, 
  BlittableJsonReaderObject.PropertyDetails prop, 
  out long value, 
  out bool delete
)
{
  value = 0;

  if (prop.Value is LazyStringValue)
  {
    delete = true;
  }

  else
  {
    delete = false;
    value = CountersStorage.InternalGetCounterValue(
      prop.Value as BlittableJsonReaderObject.RawBlob, 
      docId, 
      prop.Name
    );

    if (function != null)
    {
      using (var result = BehaviorsScript.Run(
        Context, 
        Context, 
        function, 
        new object[] { docId, prop.Name }
      ))
      {
        if (result.BooleanValue != true)
          return true;
      }
    }
  }

  return false;
}

Очевидно, этот метод может вернуть true только в том случае, если function != null. В коде же, рассмотренном ранее, на место этого параметра передаётся именно нулевой указатель. Значит, вызов continue действительно недостижим.

Этот момент может быть как безобидным упущением, так и проблемой, связанной с какой-нибудь ошибкой в условии. Так или иначе, стоит обратить на него внимание.

Доверяй, но проверяй

public LicenseType Type
{
  get
  {
    if (ErrorMessage != null)
      return LicenseType.Invalid;

    if (Attributes == null)
      return LicenseType.None;

    if (Attributes != null &&                             // <=
        Attributes.TryGetValue("type", out object type) &&
        type is int
    )
    {
      var typeAsInt = (int)type;
      if (Enum.IsDefined(typeof(LicenseType), typeAsInt))
        return (LicenseType)typeAsInt;
    }

    return LicenseType.Community;
  }
}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: Attributes != null. LicenseStatus.cs(28) Raven.Server

Крайне странный фрагмент. Обычно, лишние проверки хоть как-то отделены — здесь же соответствие переменной null-указателю проверяется прям в соседних строках. Создаётся впечатление, что код, вполне вероятно, делает совсем не то, что хотел программист.

Nullable, который никогда не null

public Task SuspendObserver()
{
  if (ServerStore.IsLeader())
  {
    var suspend = GetBoolValueQueryString("value");
    if (suspend.HasValue)                                  // <=
    {
      Server.ServerStore.Observer.Suspended = suspend.Value;
    }

    NoContentStatus();
    return Task.CompletedTask;
  }

  RedirectToLeader();

  return Task.CompletedTask;
}

Предупреждение анализатора: V3022 Expression ‘suspend.HasValue’ is always true. RachisAdminHandler.cs(116) Raven.Server

Очередная с виду безобидная «лишняя» проверка. Хотя пока не ясно, с чего анализатор считает её таковой.

Обратимся к GetBoolValueQueryString:

protected bool? GetBoolValueQueryString(string name, bool required = true)
{
  var boolAsString = GetStringQueryString(name, required);
  if (boolAsString == null)
    return null;

  if (bool.TryParse(boolAsString, out bool result) == false)
    ThrowInvalidBoolean(name, boolAsString);

  return result;
}

Действительно, иногда эта функция возвращает null. Да и Rider не считал ту проверку лишней. Неужели Единорог подвёл?

А что, если взглянуть на метод GetStringQueryString?

protected string GetStringQueryString(string name, bool required = true)
{
  var val = HttpContext.Request.Query[name];
  if (val.Count == 0 || string.IsNullOrWhiteSpace(val[0]))
  {
    if (required)
      ThrowRequiredMember(name);

    return null;
  }

  return val[0];
}

Хм, если параметр required == true, то будет вызван метод ThrowRequiredMember. Интересно, что же он делает? Ну, чтобы уж точно не осталось сомнений:

private static void ThrowRequiredMember(string name)
{
  throw new ArgumentException(
    $"Query string {name} is mandatory, but wasn't specified."
  );
}

Итак, резюмируем. Разработчик вызывает метод GetBoolValueQueryString. Вероятно, он считает, что потенциально метод не получит необходимое значение. Ну и как итог – вернёт null. Внутри вызывается GetStringQueryString. В случае если возникают проблемы, он либо вернёт null, либо выбросит исключение. Второе происходит в том случае, если параметр required установлен как true. При этом это его значение по умолчанию. В то же время, при вызове GetBoolValueQueryString он, если посмотреть на код выше, как раз не передаётся.

Давайте рассмотрим ещё раз код метода SuspendObserver, на фрагмент которого и ругался анализатор:

public Task SuspendObserver()
{
  if (ServerStore.IsLeader())
  {
    var suspend = GetBoolValueQueryString("value");
    if (suspend.HasValue)
    {
      Server.ServerStore.Observer.Suspended = suspend.Value;
    }

    NoContentStatus();
    return Task.CompletedTask;
  }

  RedirectToLeader();

  return Task.CompletedTask;
}

Создаётся впечатление, что по задумке поток выполнения тут не должен прерываться, если GetBoolValueQueryString не смог получить значение. В самом деле, после блока с проверкой на null производятся различные действия и возвращается значение. Полагаю, по задумке эти действия производятся независимо от успешности работы метода GetBoolValueQueryString. Что же произойдёт на самом деле? Поток выполнения будет прерван исключением.

Чтобы этот момент поправить, нужно при вызове GetBoolValueQueryString передать в качестве второго параметра required значение false. Тогда всё действительно будет работать так, как ожидается.

Как я и говорил ранее, иногда кажется, что анализатор ошибается (чего греха таить, бывает и такое). Также достаточно часто предупреждение выглядит незначительным. Казалось бы, есть тут лишняя проверка, да и ладно. Ну или ещё лучше – уберём её и никаких проблем – предупреждение ведь пропадёт!

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

Странности

private async Task<int> WriteDocumentsJsonAsync(...., int numberOfResults) // <=
{
  using (
    var writer = new AsyncBlittableJsonTextWriter(
      context, 
      ResponseBodyStream(), 
      Database.DatabaseShutdown
    )
  )
  {
    writer.WriteStartObject();
    writer.WritePropertyName(nameof(GetDocumentsResult.Results));
    numberOfResults = await writer.WriteDocumentsAsync(                    // <=
      context, 
      documentsToWrite, 
      metadataOnly
    );

    ....
  }
  return numberOfResults;
}

Предупреждение анализатора: V3061 Parameter ‘numberOfResults’ is always rewritten in method body before being used. DocumentHandler.cs(273), DocumentHandler.cs(267) Raven.Server

Параметр, передаваемый в функцию, не используется, а сразу перезаписывается. Зачем он тут нужен? Его хотели передавать через ref?

Мне стало любопытно взглянуть на то, как используется этот метод в существующем коде. Я понадеялся, что раз он приватный, их не должно быть особо много. Спасибо Rider, я с лёгкостью нашёл, где же производится вызов. Это было единственное место:

private async Task GetDocumentsByIdAsync(....)
{
  ....            
  int numberOfResults = 0;

  numberOfResults = await WriteDocumentsJsonAsync(
    context, 
    metadataOnly, 
    documents, 
    includes, 
    includeCounters?.Results, 
    numberOfResults
  );

  ....
}

Переменной присваивается 0, затем она передаётся в метод, результат работы которого присваивается ей же. И внутри метода этот параметр никак не используется. Эм. Зачем всё это?

Не тот логический оператор

private OrderByField ExtractOrderByFromMethod(....)
{
  ....
  if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
    throw new InvalidQueryException(....);
  ....
}

Предупреждение анализатора: V3022 Expression ‘me.Arguments.Count < 2 && me.Arguments.Count > 3’ is always false. Probably the ‘||’ operator should be used here. QueryMetadata.cs(861) Raven.Server

Полностью данный метод можно посмотреть здесь.

В этот раз перед нами очевидная ошибка, состоящая в использовании неправильного логического оператора. В текущем виде проверка количества аргументов попросту не работает, ведь нет такого значения, которое одновременно и меньше 2, и больше 3. Истинные же намерения разработчика легко раскрываются первым аргументом, передаваемым в конструктор исключения:

"Invalid ORDER BY 'spatial.distance(from, to, roundFactor)' call, 
expected 2-3 arguments, got " + me.Arguments.Count

Чтобы проверка работала корректно, нужно просто заменить «&&» на «||».

Странный try-метод

private bool Operator(OperatorField fieldOption, out QueryExpression op)
{ 
  ....
  switch (match)
  {
    ....
    case "(":
      var isMethod = Method(field, out var method); // <=
      op = method;

      if (isMethod && Operator(OperatorField.Optional, out var methodOperator))
      {
        ....
      }

      return isMethod;
    ....
  }
}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: isMethod. QueryParser.cs(1797) Raven.Server

Полностью данный метод можно посмотреть здесь.

Конструкция var isMethod = Method(field, out var method) напомнила мне о стандартных методах вроде Int.TryParse. Такие методы производят попытку получить результат и записать её в out-переменную, а флаг успешности проведения операции является возвращаемым значением. Код, использующий такие функции, обычно производит проверку возвращаемого значения, а затем на её основе выполняет те или иные операции.

На мой взгляд, функция Method используется здесь именно таким образом. Результат работы Method, кроме того, является возвращаемым значением метода Operator, вызывающего её.

Анализатор же указывает, что переменная isMethod всегда будет иметь значение true и её проверка в условии бессмысленна. Это означает, что функция Method никогда не возвращает false. Какой же тогда смысл в использовании такой конструкции?

Для начала, убедимся в том, что анализатор не ошибся:

private bool Method(FieldExpression field, out MethodExpression op)
{
  var args = ReadMethodArguments();

  op = new MethodExpression(field.FieldValue, args);
  return true;
}

Действительно, возвращаемое значение этого метода всегда true. И если всё так и задумывалось, то это… странно, но не беда, в принципе. Но что, если нет?

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

Создаётся впечатление, что код, вызывающий функцию Method, не рассчитан на выбрасывание исключений. Скорее всего предполагается, что когда корректно выполнить получение значения out-переменной не удаётся, функция Method вернёт false. Однако при текущей реализации вместо этого будет выброшено исключение.

Как бы то ни было, стоит обратить внимание на данный фрагмент.

null != null?

private Address GetNextEdge()
{
  if (m_curEdgeBlock == null || m_curEdgeBlock.Count <= m_curEdgeIdx)
  {
    m_curEdgeBlock = null;
    if (m_edgeBlocks.Count == 0)
    {
      throw new ApplicationException(
        "Error not enough edge data.  Giving up on heap dump."
      );
    }

    var nextEdgeBlock = m_edgeBlocks.Dequeue();
    if (
      m_curEdgeBlock != null &&                       // <=
      nextEdgeBlock.Index != m_curEdgeBlock.Index + 1
    )
    {
      throw new ApplicationException(
        "Error expected Node Index " + (m_curEdgeBlock.Index + 1) + 
        " Got " + nextEdgeBlock.Index + " Giving up on heap dump."
      );
    }

    m_curEdgeBlock = nextEdgeBlock;
    m_curEdgeIdx = 0;
  }
  return m_curEdgeBlock.Values(m_curEdgeIdx++).Target;
}

Предупреждение анализатора: V3063 A part of conditional expression is always false if it is evaluated: m_curEdgeBlock != null. DotNetHeapDumpGraphReader.cs(803) Raven.Debug

Переменной присваивается нулевой указатель, а затем через несколько строчек производится проверка на её неравенство null. При этом бессмысленным становится код, производящий проверку nextEdgeBlock.Index != m_curEdgeBlock.Index + 1. Кроме того, никогда не произойдёт выбрасывание исключения.

Логично предположить, что тут что-то работает не так, как должно, ведь фрагмент выглядит очень странно. Либо проверка тут вообще не нужна, либо она реализована как-то неправильно.

Можно рассмотреть срабатывание и с другой стороны – пойти от обратного. Попробуем представить ситуацию, при которой это срабатывание является ложным. Я думаю, что это возможно, только если значение переменной может быть изменено при вызове Deque. Однако m_curEdgeBlock является приватным полем, а m_edgeBlocks – это стандартная очередь, которая инициализируется в этом же классе. Таким образом, весьма сомнительно, что вызов Dequeue как-то может повлиять на значение m_curEdgeBlock. Следовательно, срабатывание, скорее всего, не является ложным.

First or null

public HashSet<string> FindSpecialColumns(string tableSchema, string tableName)
{
  var mainSchema = GetTable(tableSchema, tableName);

  var result = new HashSet<string>();
  mainSchema.PrimaryKeyColumns.ForEach(x => result.Add(x)); // <=

  foreach (var fkCandidate in Tables)
    foreach (var tableReference in fkCandidate.References.Where(
        x => x.Table == tableName && x.Schema == tableSchema
      )
    )
    {
      tableReference.Columns.ForEach(x => result.Add(x));
    }

  return result;
}

Предупреждение анализатора: V3146 Possible null dereference of ‘mainSchema’. The ‘Tables.FirstOrDefault’ can return default null value. DatabaseSchema.cs(31) Raven.Server

Срабатывание, на первый взгляд, может показаться непонятным. Действительно, при чём тут вообще FirstOrDefault? Чтобы стало ясно, отчего же анализатор ругается, необходимо взглянуть на функцию GetTable:

public TableSchema GetTable(string schema, string tableName)
{
  return Tables.FirstOrDefault(
    x => x.Schema == schema && x.TableName == tableName
  );
}

Вызов метода FirstOrDefault вместо First может быть обусловлен тем, что в коллекции может не быть элементов, соответствующих заданному условию. В таком случае FirstOrDefault, а, следовательно, и GetTable вернёт null, так как TableSchema – это ссылочный тип. Именно поэтому PVS-Studio и говорит о том, что в данном коде может произойти попытка разыменования нулевого указателя.

Возможно, всё же стоит проверять такой случай, чтобы выполнение не прерывалось с NullReferenceException. Если же вариант, при котором Tables.FirstOrDefault вернёт null невозможен, то тогда нет смысла в использовании FirstOrDefault вместо First.

Always true

public override void VerifyCanExecuteCommand(
  ServerStore store, TransactionOperationContext context, bool isClusterAdmin
)
{
  using (context.OpenReadTransaction())
  {
    var read = store.Cluster.GetCertificateByThumbprint(context, Name);
    if (read == null)
      return;

    var definition = JsonDeserializationServer.CertificateDefinition(read);
    if (
      definition.SecurityClearance != SecurityClearance.ClusterAdmin || // <=
      definition.SecurityClearance != SecurityClearance.ClusterNode     // <=
    )
      return;
  }

  AssertClusterAdmin(isClusterAdmin);
}

Предупреждение анализатора: V3022 Expression is always true. Probably the ‘&&’ operator should be used here. DeleteCertificateFromClusterCommand.cs(21) Raven.Server

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

Я полагаю, что «||» должен быть заменён на «&&». Тогда в написанном будет смысл. Если логический оператор всё же выбран верно, то скорее всего в одном из условий должно происходить сравнение других переменных. Так или иначе, этот фрагмент очень подозрительно выглядит и его обязательно нужно проанализировать.

Заключение

В первую очередь хочу поблагодарить всех, кто добрался до этого места. Статья вышла достаточно объёмная, но, надеюсь, вам было интересно вместе со мной разбираться с новой версией анализатора PVS-Studio и изучать найденные ошибки.

Важно помнить, что разработчик должен основной своей целью ставить вовсе не уменьшение количества срабатываний. Не ради достижения пустого лога ошибок нужно использовать PVS-Studio. Бороться со срабатываниями – всё равно что бороться с симптомами болезни, от которой страдает исходный код.

Рассматривая сообщения анализатора, всегда нужно стараться понять, почему выдаётся то или иное предупреждение. Лишь осознав логику, по которой анализатор выдал предупреждение, можно сделать вывод о том, указывает оно на ошибку или нет. Именно в этом случае вы будете бороться не с симптомом, а с болезнью. И именно таким образом ваш код станет чище и здоровее. И конечно же, проблем с таким прекрасным исходником будет намного меньше. Хотя лучше я пожелаю вам, чтобы их не было вообще

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

И если ошибки персонала и программ уже давно смогли категоризировать, то почему-то ошибки управленцев (в том числе проектного менеджмента) классифицировать никто не спешит. А ведь ошибки руководителей, менеджеров проектов и менеджеров продуктов чаще всего имеют самые серьёзные последствия.

Ниже попытаемся исправить данную несправедливость.

Зачем нужна градация ошибок?

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

Но документирование – не первопричина. Важнее – накопление опыта.

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

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

В рамках крупного предприятия/компании процесс управления рисками и ошибками называется риск-менеджментом – смотрите статью «Система управления рисками в компании».

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

Если ошибки предотвратимые или имеют некритичные последствия для предприятия, то ответственные лица могут быть наказаны за них в соответствие с заранее разработанной градацией – в рамках премирования/депремирования.

Концепция «вина-ответственность» всегда остается актуальной.

Классификация рисков в IT-проектах

IT-шники и программисты больше всех любят всё детализировать и классифицировать. Но когда ты работаешь с новым проектом, нужно быть готовым ко всему. Несмотря на то, что во многих гибких методологиях управления проектами (топ-10 методологий управления) контроль и выявление ошибок – это сквозной и непрерывный процесс, да и многие команды обязательно организуют тестирование своих продуктов, ошибки всё равно возникают. И возникают они как на стадии реализации проекта, так и после его сдачи.

Как ускорить обработку ошибок и их устранения? Логично категоризировать их на входе (при получении обратной связи от заказчиков или от конечных пользователей), чтобы в будущем распределять функции исправления по ответственным за них сотрудникам, желательно, обладающим необходимой квалификацией и полномочиями.

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

Сэм Канер в книге «Тестирование программного обеспечения» приводит следующую общую классификацию ошибок внутри программных продуктов:

• Ошибки интерфейса (UX/UI).
• Логика обработки ошибок (для облегчения их обнаружения).
• Ошибки вычислений.
• Ошибки обработки/интерпретации данных (очень близки по смыслу к вычислениям).
• Проблемы начального и конечного состояния.
• Превышение нагрузки.
• Ситуация спешки.
• Ошибки аппаратного обеспечения и несовместимость.
• Контроль идентификаторов (процессов).
• Ошибки, связанные с граничными условиями.

Но и это далеко не эталон. С появлением DevOps-подхода логика работы программ стала заметно сложнее. Ведь у вас в распоряжении могут быть контролируемые и неконтролируемые среды: сетевые стеки, сторонние сервисы и службы, и т.п.

Как можно классифицировать ошибки проектного менеджмента?

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

Наиболее универсальный подход в классификации любых ошибок – опирание на функции и задачи.

За что отвечает менеджер проекта? Ранее мы рассматривали обязанности администратора проекта, которые во многом дублируют обязанности менеджера (ведь администратор – правая рука руководителя команды).

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

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

Группы ошибок проектного менеджмента в зависимости от их источника

Руководитель проекта:

• Проблемы управления рисками или полное отсутствие такого управления.
• Отсутствие управления ожиданиями заказчиков (стейкхолдеров).
• Неправильная оценка ресурсов (имеющихся или необходимых для реализации задач).
• Отсутствие управления изменениями.
• Неправильно выстроенная система коммуникаций и мотивации.
• Неправильный режим управления (например, бессистемность, аврал, слишком жесткий стиль/слишком мягкий контроль и т.п.).

Заказчик:

• Неправильные критерии оценки проекта.
• Завышенные ожидания.
• Непонимание отдельных внутренних процессов работы проектной команды.

Команда в целом (включая в том числе руководителя):

• Неправильная оценка сроков реализации (слишком оптимистично, слишком пессимистично, нетипичная задача, которая повышает уровень неопределённости).
• Неправильная мотивация (отсутствие заинтересованности).
• Нежелание адаптироваться к новым задачам и целям (особенно, при отсутствии конкретной программы управления изменениями).

Внешняя среда:

• Нестабильная экономическая ситуация на рынке или в конкретной (важной для проекта) сфере.
• Изменение условий поставщиков (а поставщики есть даже в IT-проектах).

Как снизить количество ошибок проектного менеджмента?

Во-первых, на качество планирования и организацию работ существенно влияет опыт и квалификация руководителя проекта. Именно он задаёт тон во всех направлениях деятельности. Самоорганизующиеся команды – это практически несбыточная мечта. Поэтому весь груз ответственности за любые решения внутри команды так или иначе взваливается на плечи лидера (гласного или негласного).

Во-вторых, все потенциальные ошибки управления должны быть идентифицированы (как в матрице рисков) и для каждой конкретной ошибки должна быть определена вероятность наступления + определён эффект последствий (цена ошибки). Так вы сможете заранее оценить потенциальные проблемы и спланировать пути их обхода.

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

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

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

Если вы хотите ознакомиться с примерами типовых ошибок в проектах, у нас есть отдельный материал – «Почему стартапы терпят неудачи».