Двойная негация: баг, который ждал месяц

Двойная негация: баг, который ждал месяц
// Формируем движения по партиям для возврата
Движения = РаспределитьПоПартиям(-Количество, ПараметрыРаспределения);

Этот код выглядит правильно. Возврат товара -- количество со знаком минус, передаём в функцию распределения по партиям. Логично. Очевидно. Работает.

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

Код, который выглядит правильно

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

Модуль проведения документа возврата вызывает функцию распределения по партиям. Логика такая: нужно понять, из каких партий были отгружены товары, и сделать обратные движения именно по этим партиям. Не просто "минус 3 штуки откуда-нибудь", а "минус 2 штуки из партии от 15.01, минус 1 штука из партии от 22.01".

Код вызова:

Процедура СформироватьДвиженияВозврата(Движения, ТаблицаТоваров, ПараметрыПроведения)
    
    Для Каждого СтрокаТовара Из ТаблицаТоваров Цикл
        
        ПараметрыРаспределения = Новый Структура;
        ПараметрыРаспределения.Вставить("Номенклатура", СтрокаТовара.Номенклатура);
        ПараметрыРаспределения.Вставить("Склад", СтрокаТовара.Склад);
        ПараметрыРаспределения.Вставить("Заказ", ПараметрыПроведения.Заказ);
        
        // Возврат -- значит минус
        КоличествоВозврата = -СтрокаТовара.Количество;
        
        РезультатРаспределения = РаспределитьПоПартиям(
            КоличествоВозврата, ПараметрыРаспределения);
        
        Для Каждого СтрокаПартии Из РезультатРаспределения Цикл
            Движение = Движения.РезервыТоваров.Добавить();
            Движение.ВидДвижения = ВидДвиженияНакопления.Расход;
            Движение.Номенклатура = СтрокаТовара.Номенклатура;
            Движение.Количество = СтрокаПартии.Количество;
            // ...
        КонецЦикла;
        
    КонецЦикла;
    
КонецПроцедуры

Посмотрите на строку КоличествоВозврата = -СтрокаТовара.Количество. Количество в документе возврата -- положительное число (3 штуки). Минус перед ним делает его отрицательным (-3). Передаём в функцию распределения. Пока всё нормально.

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

Функция РаспределитьПоПартиям(Количество, ПараметрыРаспределения)
    
    // ... запрос остатков по партиям ...
    
    ОсталосьРаспределить = -Количество; // <-- вот он
    
    Результат = Новый ТаблицаЗначений;
    Результат.Колонки.Добавить("Партия");
    Результат.Колонки.Добавить("Количество");
    
    Для Каждого СтрокаОстатков Из ВыборкаОстатков Цикл
        
        Если ОсталосьРаспределить <= 0 Тогда
            Прервать;
        КонецЕсли;
        
        РаспределяемоеКоличество = Мин(СтрокаОстатков.Количество, ОсталосьРаспределить);
        
        НоваяСтрока = Результат.Добавить();
        НоваяСтрока.Партия = СтрокаОстатков.Партия;
        НоваяСтрока.Количество = РаспределяемоеКоличество;
        
        ОсталосьРаспределить = ОсталосьРаспределить - РаспределяемоеКоличество;
        
    КонецЦикла;
    
    Возврат Результат;
    
КонецФункции
Фрагмент кода с двойной негацией: минус на входе и минус внутри функции

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

Что происходит по шагам:

  1. Количество в документе возврата: 3 (три штуки возвращаем)
  2. Вызывающий код: КоличествоВозврата = -3
  3. Функция: ОсталосьРаспределить = -(-3) = 3
  4. Функция распределяет 3 штуки по партиям и возвращает таблицу с положительными количествами
  5. Движения регистра создаются с положительными количествами

Минус на минус дал плюс. Возврат превратился в приход.

Почему проявился через месяц

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

Условие 1: частичный возврат. Полный возврат заказа обрабатывался другой веткой кода -- через сторнирование всего документа целиком. Функция РаспределитьПоПартиям вызывалась только при частичном возврате -- когда из заказа на 10 позиций возвращают 2.

Условие 2: оплаченный заказ. Для неоплаченных заказов резервы не создавались, и движения по регистру РезервыТоваров не формировались. Баг был именно в движениях по резервам.

Условие 3: конкретная номенклатура с партионным учётом. Для товаров без партионного учёта функция распределения не вызывалась -- количество списывалось напрямую.

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

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

Таблица сравнения: ожидаемые движения возврата минус три и фактические плюс три

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

Минус на минус = плюс, но не в бухгалтерии

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

Проблема в том, что каждый "минус" в отдельности выглядит логично. Разработчик, который писал вызов функции, рассуждал: "Возврат -- значит отрицательное количество". Правильно. Разработчик, который писал функцию, рассуждал: "Мне передадут отрицательное число, но распределять нужно положительное -- инвертирую". Тоже правильно. Каждый из них принял разумное решение. Вместе эти два разумных решения дали баг.

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

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

Похожая ситуация с неявными контрактами -- когда функция ожидает одно, а получает другое, и при этом не падает с ошибкой. Я разбирал этот паттерн в статье про API БСП: стандартный метод молча возвращал пустоту, потому что кастомный справочник нарушил неявный контракт.

Как найти подобное

Двойная негация -- баг, который не ловится стандартными средствами. Код компилируется. Тесты проходят (если тестируют типовой сценарий, а не edge case). Документы проводятся без ошибок. Движения создаются. Всё работает -- просто с неправильным знаком.

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

Шаг 1: зафиксировать симптом. Бухгалтер сказал: "Резервы завышены". Это симптом, не диагноз. Первое, что я сделал -- выгрузил движения регистра РезервыТоваров за последний месяц и отфильтровал по документам возврата. Увидел: все движения возврата имеют положительный знак. Должны быть отрицательные (расход). Симптом подтверждён, направление поиска понятно.

Шаг 2: проследить цепочку формирования значения. Количество в движении -- откуда оно? Из таблицы распределения. Количество в таблице распределения -- откуда? Из функции РаспределитьПоПартиям. Что на входе функции? Что на выходе? Не отладчиком -- простым Сообщить() на каждом шаге. Вход: -3. Выход: таблица с количеством 3. Внутри функции знак поменялся. Нашёл строку с инверсией.

Шаг 3: понять контракт. Функция ожидает отрицательное число и инвертирует. Вызывающий код передаёт уже отрицательное. Двойная негация. Кто "виноват"? Зависит от контракта функции. Если в комментарии написано "Количество может быть положительным или отрицательным" -- виноват вызывающий код, не нужно было инвертировать. Если написано "Количество должно быть положительным, знак определяется контекстом" -- виновата функция, не нужно было инвертировать внутри.

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

Общие рекомендации для поиска подобных багов:

  • Ищите парные инверсии. Если видите -Количество в одном месте -- проверьте, нет ли инверсии дальше по цепочке. Grep по -Количество и -Кол по модулю.
  • Проверяйте edge cases вручную. Возьмите конкретный документ. Подставьте числа. Пройдите по коду с калькулятором. 3 --> -3 --> 3. Если знак вернулся к исходному -- проблема.
  • Смотрите на движения регистров. Если документ возврата создаёт движения прихода -- что-то не так. Движения -- это конечный результат, по ним проще всего понять, что код делает на самом деле, а не что он должен делать.
  • Тестируйте редкие сценарии. Полный возврат. Частичный возврат. Возврат одной позиции из десяти. Возврат оплаченного заказа. Возврат неоплаченного. Каждый сценарий -- отдельная ветка кода, отдельный набор потенциальных багов. Как и с транзакциями -- ошибки проявляются не в типовом сценарии, а в граничном. Подробнее об этом -- в разборе бага с ACID в 1С.
Отладка: трассировка значения количества через всю цепочку вызовов

Один символ -- одна строка diff

Исправление заняло одну строку. Убрал инверсию из вызывающего кода:

// Было:
КоличествоВозврата = -СтрокаТовара.Количество;

// Стало:
КоличествоВозврата = СтрокаТовара.Количество;

Один символ. Минус. Удалён.

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

Добавил комментарий к функции:

// Функция РаспределитьПоПартиям
// Количество: положительное число. Функция самостоятельно
// определяет направление движения (приход/расход)
// на основании контекста распределения.
Функция РаспределитьПоПартиям(Количество, ПараметрыРаспределения)

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

После исправления нужно было разобраться с накопленными неправильными данными. 34 документа возврата за месяц создали движения с неправильным знаком. Для каждого -- перепровести. Звучит просто, но перепроведение в рабочей базе с партионным учётом -- это цепочка: перепроведение возврата меняет остатки, изменение остатков может повлиять на FIFO-очередь последующих отгрузок, и так далее.

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

Вот что я вынес из этой истории.

Контракт функции должен быть явным. Какой знак ожидается на входе? Какой знак будет на выходе? Это не мелочь -- это критическая часть интерфейса. Комментарий в две строки экономит часы отладки.

Ответственность за знак -- в одном месте. Либо вызывающий код инвертирует, либо функция. Никогда -- оба. Это правило, которое нужно проверять на code review.

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

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

Diff исправления: одна строка, один удалённый символ минус

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

Но никто не проверил. Потому что код выглядел правильно. Минус перед количеством возврата -- это же логично, правда?

Правда. Логично. И внутри функции -- тоже логично. По отдельности -- каждый прав. Вместе -- баг.