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

Предыстория

Сейчас мы занимаемся активной разработкой статического анализатора для JavaScript и TypeScript. Он находится в стадии EAP, на который ещё можно записаться. Мне довелось поработать над правилом V7001, которое ищет опечатки в виде одинаковых операндов бинарных выражений:

let baz = foo – foo // foo - bar
if (foo == foo) { // foo == bar
  // ....
}

В фокусе и более сложные случаи, например вложенные выражения:

(a && b) || (c && d) || (a && b)

И не идентичные, но эквивалентные выражения:

(a * b) > (b * a)

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

От очарования до разочарования

Когда я только закончил писать это диагностическое правило, то запустил анализ на нашей базе проектов и нашёл такой фрагмент в pdf.js:

flagValues: Object.freeze({
  applyOverPrint: 1,
  applySoftProofSettings: 1 << 1,
  applyWorkingColorSpaces: 1 << 2,
  emitHalftones: 1 << 3,
  emitPostScriptXObjects: 1 << 4,
  emitFormsAsPSForms: 1 << 5,
  maxJP2KRes: 1 << 6,
  setPageSize: 1 << 7,
  suppressBG: 1 << 8,
  suppressCenter: 1 << 9,
  suppressCJKFontSubst: 1 << 10,
  suppressCropClip: 1 << 1,
  suppressRotate: 1 << 12,
  suppressTransfer: 1 << 13,
  suppressUCR: 1 << 14,
  useTrapAnnots: 1 << 15,
  usePrintersMarks: 1 << 16,
}),

Здесь определяют набор флагов управления конвертацией для печати. Каждый флаг — одна опция, и через побитовые операции их можно свободно комбинировать. Флаги делятся на группы: цветокоррекция, подавление трансформаций, шрифты и прочее. Однако с JavaScript кодом здесь что-то не так. Заметили?

Предупреждение PVS-Studio: V7001 The operands of the ‘<<’ operator in the ‘1 << 1’ expression are equivalent. print_params.js 90

Срабатывание ссылается на строчку между 10-м и 12-м флагом:

suppressCJKFontSubst: 1 << 10,
suppressCropClip: 1 << 1,
suppressRotate: 1 << 12,

Вполне вероятно, что этот код писался с помощью Ctrl+C и Ctrl+V, после чего в каждой строке имя поля и номер флага заменялись вручную. Однако в одном месте 1 << 1 забыли заменить на 1 << 11. Вероятно, недостающая единица просто ускользнула от взора, ведь её и правда легко не заметить в ряду единиц.

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

Бочка дёгтя

Тем не менее, запустив PVS-Studio для JavaScript или TypeScript, этого срабатывания вы не увидите. Почему? Во время разработки диагностика нашла очень много подобных срабатываний, но подавляющая часть из них выглядит вот так:

Angular

export enum InputFlags {
  None = 0,
  SignalBased = 1 << 0,
  HasDecoratorInputTransform = 1 << 1,
}

Предупреждение PVS-Studio: V7001 The operands of the ‘<<’ operator in the ‘1 << 1’ expression are equivalent. core.ts 49

Babel

const enum PRINTER_FLAGS {
  EMPTY = 0,
  PRESERVE_FORMAT = 1 << 0,
  COMPACT = 1 << 1,
  CONCISE = 1 << 2,
  RETAIN_LINES = 1 << 3,
  RETAIN_FUNCTION_PARENS = 1 << 4,
  AUX_COMMENTS = 1 << 5,
}

Предупреждение PVS-Studio: V7001 The operands of the ‘<<’ operator in the ‘1 << 1’ expression are equivalent. index.ts 10

Снова pdf.js

const ON_CURVE_POINT = 1 << 0;
const X_SHORT_VECTOR = 1 << 1;
const Y_SHORT_VECTOR = 1 << 2;
const REPEAT_FLAG = 1 << 3;
const X_IS_SAME_OR_POSITIVE_X_SHORT_VECTOR = 1 << 4;
const Y_IS_SAME_OR_POSITIVE_Y_SHORT_VECTOR = 1 << 5;
const OVERLAP_SIMPLE = 1 << 6;

Предупреждение PVS-Studio: V7001 The operands of the ‘<<’ operator in the ‘1 << 1’ expression are equivalent. glyf.js 17

Это всего лишь три из 42 таких же срабатываний при общем числе в 126 V7001 в тестовой базе. Как можно заметить, везде анализатор смущает 1 << 1. И формально он прав: обычно большого смысла сдвигать число на само себя нет, за исключением очень нишевых случаев. Результатом 1 << 1 операции всегда будет 2. Всё, что такая запись делает, это заставляет интерпретатор свёртывать константу при компиляции в машинный код.

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

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

applyOverPrint: 1,
applySoftProofSettings: 1 << 1,
applyWorkingColorSpaces: 1 << 2,

Анализатор тоже выдаст ложное срабатывание на 1 << 1 — ещё одна ложка дёгтя в эту бочку.

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

Нешумный анализ

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

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

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

Так как мы стараемся придерживаться unsound-подхода и не шуметь сотнями и тысячами ложных срабатываний, нам приходиться вникать в существующие подходы и делать эвристические исключения для них. Так в диагностике V7001 и появилось исключение на 1 << 1, хотя пришлось потерять хорошее срабатывание.

Почему не сделать более точное исключение?

Можно было бы попытаться придумать что-то более хитрое. Например, проверять, нет ли по соседству других битовых флагов. Но что такое “по соседству” для статического анализатора кода? Анализатор работает с синтаксическим деревом, и мы могли бы попробовать искать соседей по перечислению:

Но, во-первых, такую эвристику легко “обмануть”:

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

• поменять порядок определения флагов;

• записать флаги в промежуточные переменные.

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

Во-вторых, если вы обратите внимание, то приведённые выше срабатывания были в трёх разных контекстах:

• в enum;

• в инициализации поля внутри object expression;

• в константных переменных.

И это не считая экзотических случаев вроде битовых флагов в полях класса. Таких случаев много, особенно в JavaScript/TypeScript, и их очень легко пропустить.

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

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

Послесловие

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Konstantin Volohovsky. Error that we had to ignore.