Внутри Java Enterprise кода: проверка Flowable

• Очень страшные слова
• Мгновенное исполнение
• BigDecimal
• Пробел всегда в конце
• Бессмертные тесты
• Сам себе родитель
• Единственная итерация
• Непустая коллекция
• Поле без значения
• Возможный null
• ClassCastException
• Правим code style
• Лишняя проверка
• Заключение

Язык Java известен широким применением в бизнесе. Бизнес-процессами необходимо управлять, в чём может помочь платформа Flowable, которая, так сложилось, написана на Java, так ещё и с открытым исходным кодом! А значит, мы можем изучить его с помощью PVS-Studio и попробовать найти ошибки.

Очень страшные слова

Упомянутое в аннотации — это во многом это всё, что я могу сказать о проекте, не начав просто пересказывать написанное в Википедии или на официальном сайте. Потому хорошей идеей будет обратиться к нему: Flowable: Business Process Automation | Low-code | Workflow Automation.

Бизнес значит Enterprise. А Java и Enterprise — известная связка! Следовательно, большое количество строк кода совпадает по длине с эссе. В связи с этим мне пришлось заняться своевольным форматированием для того, чтобы сделать код более читаемым. Имейте в виду, что большая часть представленных фрагментов в оригинале, скорее всего, выглядит иначе.

Чтобы убедиться, что мы работаем с Java, предлагаю посмотреть несколько коротких наименований классов:

• TimerChangeProcessDefinitionSuspensionStateJobHandler;
• IntermediateThrowCompensationEventActivityBehavior;
• AbstractExternalInvocationBpmnParseHandler;
• ProcessInstanceStartEventSubscriptionModificationBuilderImpl;
• UnfinishedHistoricActivityInstanceByProcessInstanceIdMatcher;
• ParallelMultiInstanceWithNoWaitStatesAsyncLeaveJobHandler;
• EventSubProcessVariableListenerlStartEventActivityBehavior.

Но это не всё. Рекордсменом по длине стал класс UpdateChannelDefinitionTypeAndImplementationForAllChannelDefinitionsCmd. А с тестами побеждает это же наименование с дополнительным суффиксом Test.

Посмотрев на Enterprise наименования, предлагаю заглянуть в кодовую базу. Для этого, согласно традиции, воспользуемся PVS-Studio и попробуем найти самое интересное. Надеваем пиджаки и отправляемся в путь.

Мгновенное исполнение

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

Причём про поиск ошибок в Unit-тестах мы писали совсем недавно в отдельной статье! Рекомендую тестировщикам к ознакомлению.

А здесь предлагаю взглянуть на следующий фрагмент:

@Test
@CmmnDeployment
public void testExpressionReturnsFuture() {
  CountDownLatch latch = new CountDownLatch(2);
  TestBeanReturnsFuture testBean = new TestBeanReturnsFuture(latch, 
    cmmnEngineConfiguration.getAsyncTaskExecutor());
  CaseInstance caseInstance = cmmnRuntimeService.createCaseInstanceBuilder()
    .caseDefinitionKey("myCase")
    .transientVariable("bean", testBean)
    .start();

  assertCaseInstanceEnded(caseInstance);

  if (....) {
    HistoricCaseInstance historicCaseInstance = ....;

    // Every service task sleeps for 1s, 
    // but when we use futures it should take less then 2s.
    long durationInMillis = historicCaseInstance.getEndTime().getTime() – 
      historicCaseInstance.getEndTime().getTime();
    assertThat(durationInMillis).isLessThan(1500);
  }
}

Тест проверяет время исполнения сервиса. К сожалению, значение durationInMillis всегда равно нулю, поскольку вычитается getEndTime() из getEndTime(). Соответственно, сам тест, проверяющий, что это значение меньше 1500, всегда будет положительным, ведь 0 < 1500, что легко доказать после прочтения Principia Mathematica.

Исправление крайне простое:

long durationInMillis = historicCaseInstance.getEndTime().getTime() –
  historicCaseInstance.getStartTime().getTime();

Я перезапустил тест с этим исправлением, и он успешно прошёл. А теперь посмотрим на сообщение анализатора PVS-Studio, которое помогло найти проблему:

V6001 There are identical sub-expressions historicCaseInstance.getEndTime().getTime()' to the left and to the right of the '-' operator. ServiceTaskWithFuturesTest.java 76

BigDecimal

Далее на очереди легендарный в финтехе BigDecimal:

public class ELExecutionContextBuilder {
  ....
  protected static void preProcessInputVariables(DecisionTable decisionTable,
    Map<String, Object> inputVariables) {
    ....
    // check if transformation is needed
    for (Map.Entry<String, Object> inputVariable : inputVariables.entrySet()) {
      String inputVariableName = inputVariable.getKey();
      try {
        Object inputVariableValue = inputVariable.getValue();
        if (inputVariableValue instanceof LocalDate) {
          Date transformedDate = ((LocalDate) inputVariableValue).toDate();
          inputVariables.put(inputVariableName, transformedDate);
        } else if (inputVariableValue instanceof java.time.LocalDate) {
          Date transformedDate = 
            Date.from(((java.time.LocalDate) inputVariableValue).atStartOfDay()
              .atZone(ZoneId.systemDefault())
              .toInstant());
          inputVariables.put(inputVariableName, transformedDate);
        } else if (....) {
          BigInteger transformedNumber = 
            new BigInteger(inputVariableValue.toString());
          inputVariables.put(inputVariableName, transformedNumber);
        } else if (inputVariableValue instanceof Double) {
          BigDecimal transformedNumber = 
            new BigDecimal((Double) inputVariableValue);
          inputVariables.put(inputVariableName, transformedNumber);
        } else if (inputVariableValue instanceof Float) {
          double doubleValue = 
            Double.parseDouble(inputVariableValue.toString());
          BigDecimal transformedNumber = new BigDecimal(doubleValue);   // <=
          inputVariables.put(inputVariableName, transformedNumber);
        }
      } catch (Exception ex) {
        throw new FlowableException(....);
      }
    }
  }
}

Рекомендую в код долго не всматриваться, иначе посмотрит в ответ. Нас интересует работа с уже упомянутым BigDecimal. Для выяснения деталей необходимо обратиться к примечанию в JavaDoc:

The results of this constructor can be somewhat unpredictable. One might assume that writing new BigDecimal(0.1) in Java creates a BigDecimal which is exactly equal to 0.1 (an unscaled value of 1, with a scale of 1), but it is actually equal to 0.1000000000000000055511151231257827021181583404541015625. This is because 0.1 cannot be represented exactly as a double (or, for that matter, as a binary fraction of any finite length). Thus, the value that is being passed in to the constructor is not exactly equal to 0.1, appearances notwithstanding.

В этом же документе указана рекомендация к применению BigDecimal.valueOf(double) или new BigDecimal(String). При этом в указанном фрагменте значение парсят в String, а затем вновь в double. Изменится ли от этого что-то? Предлагаю посмотреть интереса ради:

void main() {
  double d = 0.1;
  BigDecimal bigDecimal;

  bigDecimal = new BigDecimal(d);
  println("new BigDecimal(double): \t" + bigDecimal.toPlainString());

  bigDecimal = new BigDecimal(String.valueOf(d));
  println("new BigDecimal(double -> string): \t" + 
    bigDecimal.toPlainString());

  bigDecimal = new BigDecimal(Double.parseDouble(String.valueOf(d)));
  println("new BigDecimal(double -> string -> double): \t" + 
    bigDecimal.toPlainString());

  bigDecimal = BigDecimal.valueOf(d);
  println("BigDecimal.valueOf(double): \t" + bigDecimal.toPlainString());
}

Получаем следующий вывод:

new BigDecimal(double): 
  0.1000000000000000055511151231257827021181583404541015625
new BigDecimal(double -> string): 0.1
new BigDecimal(double -> string -> double):
  0.1000000000000000055511151231257827021181583404541015625
BigDecimal.valueOf(double): 0.1

Что ж, от дополнительных преобразований double в строку и обратно поведение не меняется. В любом случае следует использовать либо valueOf, либо конструктор со String аргументом, если мы хотим получить 0.1 из вводимого 0.1 значения (исходя из названия метода, это обработка input variables).

В результате получаем, что можно не только избавиться от потенциально неожиданного значения, но и устранить лишнюю операцию парсинга String к double:

new BigDecimal(inputVariableValue.toString());

Напоследок срабатывание анализатора PVS-Studio:

V6068 Constructor call can result in imprecise representation of the initialized value. ELExecutionContextBuilder.java 137

Пробел всегда в конце

public class CronExpression implements Serializable, Cloneable {
  ....
  protected int findNextWhiteSpace(int i, String s) {
    for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) {
    }

    return i;
  }
  ....
}

Сразу отмечу, что и в оригинале тело цикла пустое (иначе бы там стояли ....).

Уже встречавшаяся ошибка в других проектах — одно из выражений всегда истинно. Речь о s.charAt(i) != ' ' || s.charAt(i) != '\t'.

Для того, чтобы получить ложный результат оператора ИЛИ, необходимо, чтобы символ был одновременно равен двум разным символам. Что, естественно, невозможно.

В результате получается, что цикл выполняется, пока условие i < s.length() является true, и вывод метода — это всегда индекс конца строки, а не символа пробела.

Предупреждение PVS-Studio:

V6007 Expression 's.charAt(i) != '\t'' is always true. CronExpression.java 963

Бессмертные тесты

@Test
void convertJsonToModelKeyWithFixedValue() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    .isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getFixedValue().equals("customer");
    });
}

@Test
void convertJsonToModelKeyWithDelegateExpression() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    .isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getDelegateExpression().equals("${test}");
    });
}

@Test
void convertJsonToModelKeyWithEventField() {
  ChannelModel channelModel = readJson(....);
  assertThat(channelModel)
    B.isInstanceOfSatisfying(KafkaOutboundChannelModel.class, model -> {
      model.getRecordKey().getEventField().equals("customer");
    });
}

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

• V6010 The return value of function 'equals' is required to be utilized. KafkaOutboundChannelJsonConverterTest.java 45
• V6010 The return value of function 'equals' is required to be utilized. KafkaOutboundChannelJsonConverterTest.java 53
• V6010 The return value of function 'equals' is required to be utilized. KafkaOutboundChannelJsonConverterTest.java 61

Вероятно, разработчик по некой причине ожидал, что вторым аргументом метода isInstanceOfSatisfying является Function<T, Boolean>. Хотя я не знаю, почему могло быть такое ожидание, на деле же метод принимает Consumer<T>. Возможно, дело в простой невнимательности, и разработчику могло показаться, что метод equals эквивалентен AssertEquals.

Для того, чтобы тест упал, необходимо выбросить исключение. Просто обернём вызовы equals в assertThat(....).isEqualTo(....). Например:

assertThat(model.getRecordKey().getEventField()).isEqualTo("customer");

Сам себе родитель

public class ProcessExecutionLogger {
  protected void internalPopulateExecutionTree(....) {
    if (parentMapping.containsKey(parentNode.getId())) {
      for (....) {
        ....

        childNode.setParentNode(childNode);                        // <=
        parentNode.getChildNodes().add(childNode);

        internalPopulateExecutionTree(childNode, parentMapping);
      }
    }
  }
}

Здесь нарушенная иерархия приводит к тому, что у объекта childNode родителем является он сам. На необходимую зависимость косвенно указывает строка, следующая сразу после – parentNode.getChildNodes().add(childNote). То есть родитель о ребёнке знает, но не наоборот. Любопытно, существует ли риск при итерации по таким нодам выйти в StackOverflowError?

Сообщение анализатора PVS-Studio о странности:

V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the 'setParentNode' method. ProcessExecutionLogger.java 107

Единственная итерация

protected String getTargetNameSpace(XMLStreamReader xmlStreamReader) {
  String targetNameSpace = null;
  try {
    while (xmlStreamReader.hasNext()) {
      try {
        xmlStreamReader.next();
      } catch (Exception e) {
        LOGGER.debug("Error reading XML document", e);
        throw new DmnXMLException("Error reading XML", e);
      }
      targetNameSpace = xmlStreamReader.getNamespaceURI();
      break;
    }
  } catch (XMLStreamException e) {
    LOGGER.error("Error processing DMN document", e);
    throw new DmnXMLException("Error processing DMN document", e);
  }

  return targetNameSpace;
}

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

Небольшое обращение к git позволяет отыскать предыдущую версию метода (он был переименован):

protected boolean isDMN12(XMLStreamReader xtr) {
    try {
        while (xtr.hasNext()) {
            try {
                xtr.next();
            } catch (Exception e) {
                LOGGER.debug("Error reading XML document", e);
                throw new DmnXMLException("Error reading XML", e);
            }
            return DMN_12_TARGET_NAMESPACE.equals(xtr.getNamespaceURI());
        }
        return false;
    } catch (XMLStreamException e) {
        LOGGER.error("Error processing DMN document", e);
        throw new DmnXMLException("Error processing DMN document", e);
    }
}

В этой версии вместо break используется return, и вновь без условия. Я думаю, что в этом методе просто решили использовать while, потому что вызывается метод xtr.hasNext().

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

V6037 An unconditional 'break' within a loop. DmnXMLConverter.java 187

Непустая коллекция

protected void addRecipient(MimeMessage message, 
  Collection<String> recipients, 
  Message.RecipientType recipientType) {
  if (recipients == null || recipients.isEmpty()) {
    return;
  }
  Collection<String> newRecipients = recipients;
  Collection<String> forceRecipients = defaultsConfiguration.forceTo();
  if (forceRecipients != null && !forceRecipients.isEmpty()) {
    newRecipients = forceRecipients;
  }
  if (!newRecipients.isEmpty()) {
    for (String t : newRecipients) {
      try {
        message.addRecipient(recipientType, createInternetAddress(t));
      } catch (MessagingException e) {
        throw new FlowableMailException(....);
      }
    }
  }
}

Бессмысленная проверка. newRecipients принимает значение либо forceRecipients, либо recipients. Оба перед этим проверяются на isEmpty.

Бессмысленности прибавляет и то, что цикл for (String t : newRecipients) и так не стал бы итерировать по пустой коллекции.

На такой излишний код указал анализатор PVS-Studio:

Expression '!newRecipients.isEmpty()' is always true. JakartaMailFlowableMailClient.java 194

Поле без значения

public class CallbackData {
  protected String callbackId;
  protected String callbackType;
  protected String instanceId;
  protected String oldState;
  protected String newState;
  protected Map<String, Object> additionalData = null;

  public CallbackData(String callbackId, String callbackType, 
      String instanceId, String oldState, 
      String newState, Map<String, Object> additionalData) {
    this.callbackId = callbackId;
    this.callbackType = callbackType;
    this.instanceId = instanceId;
    this.oldState = oldState;
    this.newState = newState;
  }

  ....

  public Map<String, Object> getAdditionalData() {
    return additionalData;
  }

  public void setAdditionalData(Map<String, Object> additionalData) {
    this.additionalData = additionalData;
  }
}

Срабатывание PVS-Studio:

V6022 Parameter 'additionalData' is not used inside constructor body. CallbackData.java 29

Несколько теоретический случай. Использование разработчиком указанного конструктора может привести к неприятным последствиям, ведь при передаче в него additionalData это значение не будет записано. Произойдёт неприятное рождение NullPointerException. Например, при использовании метода getAdditionalData(), если ожидается, что он не вернёт null.

Возможный null

protected void endAllHistoricActivities(....) {
  ProcessEngineConfigurationImpl processEngineConfiguration = CommandContextUtil
      .getProcessEngineConfiguration();
  if (!processEngineConfiguration.getHistoryLevel()
        .isAtLeast(HistoryLevel.ACTIVITY)) {
    return;
  }

  List<HistoricActivityInstanceEntity> historicActivityInstances = ....;

  Clock clock = processEngineConfiguration.getClock();
  for (var historicActivityInstance : historicActivityInstances) {
    historicActivityInstance.markEnded(deleteReason, clock.getCurrentTime());

    // Fire event
    FlowableEventDispatcher eventDispatcher = null;
    if (processEngineConfiguration != null) {                        // <=
      eventDispatcher = processEngineConfiguration.getEventDispatcher();
    }
    if (eventDispatcher != null && eventDispatcher.isEnabled()) {
      seventDispatcher.dispatchEvent(....),
      processEngineConfiguration.getEngineCfgKey());                 // <=
    }
  }
}

CommandContextUtil.getProcessEngineConfiguration() может вернуть null. И с учётом, что проверка на null в коде происходит, это ожидается. Чтобы прибавить уверенности, посмотрим на этот метод:

public static ProcessEngineConfigurationImpl getProcessEngineConfiguration() {
    return getProcessEngineConfiguration(getCommandContext());
}

public static ProcessEngineConfigurationImpl
  getProcessEngineConfiguration(CommandContext commandContext) {
    if (commandContext != null) {
        return ....
    }
    return null;
}

Может ли commandContext быть null? Продолжаем проход по цепочке вызовов:

public static CommandContext getCommandContext() {
    return Context.getCommandContext();
}

public static CommandContext getCommandContext() {
    Stack<CommandContext> stack = getStack(commandContextThreadLocal);
    if (stack.isEmpty()) {
        return null;
    }
    return stack.peek();
}

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

В результате вызов processEngineConfiguration.getHistoryLevel() рискует обернуться NullPointerException.

Предупреждение анализатора:

V6060 The 'processEngineConfiguration' reference was utilized before it was verified against null. TerminateEndEventActivityBehavior.java 166, TerminateEndEventActivityBehavior.java 179

ClassCastException

public class IntegerDataObject extends ValuedDataObject {

  @Override
  public void setValue(Object value) {
    if (value instanceof String &&
      !StringUtils.isEmpty(((String) value).trim())) {
      this.value = Integer.valueOf(value.toString());
    } else if (value instanceof Number) {
      this.value = (Integer) value;                  // <=
    }
  }

  @Override
  public IntegerDataObject clone() {
    IntegerDataObject clone = new IntegerDataObject();
    clone.setValues(this);
    return clone;
  }
}

Странная проверка на Number с последующим кастом в Integer (а также Long и Double в других классах-наследниках ValuedDataObject). Возможно, разработчик ожидал, что это возымеет эффект, или, подобно примитивным типам, произойдёт соответствующая конвертация. Однако на самом деле исполнение следующей программы закончится ClassCastException:

void main() {
    Number integer = 528;
    println((Float)integer);
}

Результат:

Exception in thread "main" java.lang.ClassCastException: 
  class java.lang.Integer cannot be cast to class java.lang.Float 
  (java.lang.Integer and java.lang.Float 
    are in module java.base of loader 'bootstrap')
        at Exception.main(Exception.java:3)

Потому, думаю, необходимо пересмотреть происходящее в этих методах.

Предупреждения PVS-Studio:

• V6042 The expression is checked for compatibility with type 'java.lang.Number' but is cast to type 'java.lang.Integer'. IntegerDataObject.java 27
• V6042 The expression is checked for compatibility with type 'java.lang.Number' but is cast to type 'java.lang.Long'. LongDataObject.java 27
• V6042 The expression is checked for compatibility with type 'java.lang.Number' but is cast to type 'java.lang.Double'. DoubleDataObject.java 27

Правим code style

@Override
protected DmnElement convertXMLToElement(XMLStreamReader xtr, 
  ConversionHelper conversionHelper) throws Exception {
  DecisionService decisionService = new DecisionService();
  decisionService.setId(xtr.getAttributeValue(null, ATTRIBUTE_ID));
  decisionService.setName(xtr.getAttributeValue(null, ATTRIBUTE_NAME));
  decisionService.setLabel(xtr.getAttributeValue(null, ATTRIBUTE_LABEL));

  boolean readyWithDecisionService = false;
  try {
    while (!readyWithDecisionService && xtr.hasNext()) {
      xtr.next();
      if (xtr.isStartElement() &&
          ELEMENT_OUTPUT_DECISION.equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addOutputDecision(ref);
      } if (xtr.isStartElement() &&
            ELEMENT_ENCAPSULATED_DECISION
              .equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addEncapsulatedDecision(ref);
      } if (xtr.isStartElement() && 
            ELEMENT_INPUT_DATA.equalsIgnoreCase(xtr.getLocalName())) {
        DmnElementReference ref = new DmnElementReference();
        ref.setHref(xtr.getAttributeValue(null, ATTRIBUTE_HREF));
        decisionService.addInputData(ref);
      } else if (xtr.isEndElement() && 
          getXMLElementName().equalsIgnoreCase(xtr.getLocalName())) {
        readyWithDecisionService = true;
      }
    }
  } catch (Exception e) {
    LOGGER.warn("Error parsing output entry", e);
  }
  return decisionService;
}

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

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

Срабатывания PVS-Studio:

• V6086 Suspicious code formatting. 'else' keyword is probably missing. DecisionServiceXMLConverter.java 45
• V6086 Suspicious code formatting. 'else' keyword is probably missing. DecisionServiceXMLConverter.java 49

Лишняя проверка

protected void leaveFlowNode(FlowNode flowNode) {
  ....

  // Determine which sequence flows can be used for leaving
  List<SequenceFlow> outgoingSequenceFlows = new ArrayList<>();
  for (SequenceFlow sequenceFlow : flowNode.getOutgoingFlows()) {

    String skipExpressionString = sequenceFlow.getSkipExpression();
    if (!SkipExpressionUtil.isSkipExpressionEnabled(
         skipExpressionString, sequenceFlow.getId(), 
         execution, commandContext)
      ) {

      if (!evaluateConditions                                        // <=
      || (evaluateConditions &&                                      // <=
          ConditionUtil.hasTrueCondition(sequenceFlow, execution) &&
         (defaultSequenceFlowId == null ||
          !defaultSequenceFlowId.equals(sequenceFlow.getId())))) {
        outgoingSequenceFlows.add(sequenceFlow);
      }

    } else if (flowNode.getOutgoingFlows().size() == 1 ||
        SkipExpressionUtil.shouldSkipFlowElement(
          skipExpressionString, sequenceFlow.getId(), execution, commandContext)
        ) {

      ....
      outgoingSequenceFlows.add(sequenceFlow);
    }
  }
  ....
}

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

V6007 Expression 'evaluateConditions' is always true. TakeOutgoingSequenceFlowsOperation.java 220

Вход во второе выражение в операторе или происходит только в случае, если evaluateConditions является false. Соответственно, в проверке (evaluateConditions && ...) выражение слева бессмысленно.

Заключение

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

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

При этом попробовать PVS-Studio можно совершенно бесплатно :)