Анализ кода CUBA Platform с помощью PVS-Studio

    Picture 2

    Для Java программистов существуют полезные инструменты, помогающие писать качественный код, например, мощная среда разработки IntelliJ IDEA, бесплатные анализаторы SpotBugs, PMD и другие. Всё это уже используется в разработке проекта CUBA Platform, и в этом обзоре найденных дефектов кода я расскажу, как ещё можно улучшить качество проекта, используя статический анализатор кода PVS-Studio.

    О проекте и анализаторе


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

    PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках C, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS. Для удобства Java-программистов мы разработали плагины для Maven, Gradle и IntelliJ IDEA. Проект CUBA Platform легко проверился с помощью плагина для Gradle.

    Ошибки в условиях


    Предупреждение 1

    V6007 Expression 'StringUtils.isNotEmpty(«handleTabKey»)' is always true. SourceCodeEditorLoader.java(60)

    @Override
    public void loadComponent() {
      ....
      String handleTabKey = element.attributeValue("handleTabKey");
      if (StringUtils.isNotEmpty("handleTabKey")) {
        resultComponent.setHandleTabKey(Boolean.parseBoolean(handleTabKey));
      }
      ....
    }

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

    Есть ещё одна аналогичная ошибка в файле AbstractTableLoader.java:

    • V6007 Expression 'StringUtils.isNotEmpty(«editable»)' is always true. AbstractTableLoader.java(596)

    Предупреждение 2

    V6007 Expression 'previousMenuItemFlatIndex >= 0' is always true. CubaSideMenuWidget.java(328)

    protected MenuItemWidget findNextMenuItem(MenuItemWidget currentItem) {
      List<MenuTreeNode> menuTree = buildVisibleTree(this);
      List<MenuItemWidget> menuItemWidgets = menuTreeToList(menuTree);
    
      int menuItemFlatIndex = menuItemWidgets.indexOf(currentItem);
      int previousMenuItemFlatIndex = menuItemFlatIndex + 1;
      if (previousMenuItemFlatIndex >= 0) {
          return menuItemWidgets.get(previousMenuItemFlatIndex);
      }
      return null;
    }

    Функция indexOf может вернуть значение -1, если в списке не будет найден элемент. Затем к индексу прибавляется единица, скрывая таким образом ситуацию, когда нужный элемент отсутствует. Другой потенциальной проблемой может стать тот факт, что переменная previousMenuItemFlatIndex будет всегда больше или равна нулю. Если, например, список menuItemWidgets будет пустым, то становится возможным выход за границу массива.

    Предупреждение 3

    V6009 The 'delete' function could receive the '-1' value while non-negative value is expected. Inspect argument: 1. AbstractCollectionDatasource.java(556)

    protected DataLoadContextQuery createDataQuery(....) {
      ....
      StringBuilder orderBy = new StringBuilder();
      ....
      if (orderBy.length() > 0) {
          orderBy.delete(orderBy.length() - 2, orderBy.length());
          orderBy.insert(0, " order by ");
      }
      ....
    }

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

    Предупреждение 4

    V6013 Objects 'masterCollection' and 'entities' are compared by reference. Possibly an equality comparison was intended. CollectionPropertyContainerImpl.java(81)

    @Override
    public void setItems(@Nullable Collection<E> entities) {
      super.setItems(entities);
      Entity masterItem = master.getItemOrNull();
      if (masterItem != null) {
        MetaProperty masterProperty = getMasterProperty();
        Collection masterCollection = masterItem.getValue(masterProperty.getName());
        if (masterCollection != entities) {
          updateMasterCollection(masterProperty, masterCollection, entities);
        }
      }
    }

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

    Предупреждение 5

    V6013 Objects 'value' and 'oldValue' are compared by reference. Possibly an equality comparison was intended. WebOptionsList.java(278)

    protected boolean isCollectionValuesChanged(Collection<I> value,
                                                Collection<I> oldValue) {
      return value != oldValue;
    }

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

    Избыточные условия


    Предупреждение 1

    V6007 Expression 'mask.charAt(i + offset) != placeHolder' is always true. DatePickerDocument.java(238)

    private String calculateFormattedString(int offset, String text) .... {
      ....
      if ((mask.charAt(i + offset) == placeHolder)) {         // <=
        ....
      } else if ((mask.charAt(i + offset) != placeHolder) &&  // <=
                 (Character.isDigit(text.charAt(i)))) {
        ....
      }
      ....
    }

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

    V6007 Expression 'connector == null' is always false. HTML5Support.java(169)

    private boolean validate(NativeEvent event) {
      ....
      while (connector == null) {
        widget = widget.getParent();
        connector = Util.findConnectorFor(widget);
      }
    
      if (this.connector == connector) {
          return true;
      } else if (connector == null) {             // <=
          return false;
      } else if (connector.getWidget() instanceof VDDHasDropHandler) {
          return false;
      }
      return true;
    }

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

    Ещё одно подозрительное место, на которое стоит обратить внимание разработчикам:

    • V6007 Expression 'StringUtils.isBlank(strValue)' is always true. Param.java(818)

    Недостижимый код в тестах


    V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(283)

    private void throwException() {
      throw new RuntimeException(TEST_EXCEPTION_MSG);
    }
    
    @Test
    public void testSuspendRollback() {
      Transaction tx = cont.persistence().createTransaction();
      try {
        ....
        Transaction tx1 = cont.persistence().createTransaction();
        try {
          EntityManager em1 = cont.persistence().getEntityManager();
          assertTrue(em != em1);
          Server server1 = em1.find(Server.class, server.getId());
          assertNull(server1);
          throwException();        // <=
          tx1.commit();            // <=
        } catch (Exception e) {
          //
        } finally {
          tx1.end();
        }
    
        tx.commit();
      } finally {
        tx.end();
      }
    }

    Функция throwException бросает исключение, из-за которого не выполняется вызов функции tx1.commit. Возможно, эти строки стоит поменять местами.

    Ещё несколько похожих мест в других тестах:

    • V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(218)
    • V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(163)
    • V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(203)
    • V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(137)
    • V6019 Unreachable code detected. It is possible that an error is present. UpdateDetachedTest.java(153)
    • V6019 Unreachable code detected. It is possible that an error is present. EclipseLinkDetachedTest.java(132)
    • V6019 Unreachable code detected. It is possible that an error is present. PersistenceTest.java(223)

    Подозрительные аргументы функций


    Предупреждение 1

    V6023 Parameter 'salt' is always rewritten in method body before being used. BCryptEncryptionModule.java(47)

    @Override
    public String getHash(String content, String salt) {
      salt = BCrypt.gensalt();
      return BCrypt.hashpw(content, salt);
    }

    В криптографии salt — строка данных, которая передаётся хеш-функции вместе с паролем. Главным образом используется для защиты от перебора по словарю и атак с использованием радужных таблиц, а также сокрытия одинаковых паролей. Подробнее: Соль (криптография).

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

    Предупреждение 2

    Для рассматриваемой функции анализатор выдаёт сразу два предупреждения:

    • V6023 Parameter 'offsetWidth' is always rewritten in method body before being used. CubaSuggestionFieldWidget.java(433)
    • V6023 Parameter 'offsetHeight' is always rewritten in method body before being used. CubaSuggestionFieldWidget.java(433)


    @Override
    public void setPosition(int offsetWidth, int offsetHeight) {
      offsetHeight = getOffsetHeight();
      ....
      if (offsetHeight + getPopupTop() > ....)) {
        ....
      }
      ....
      offsetWidth = containerFirstChild.getOffsetWidth();
      if (offsetWidth + getPopupLeft() > ....)) {
          ....
      } else {
          left = getPopupLeft();
      }
      setPopupPosition(left, top);
    }

    Занятный код. Функция принимает всего две переменных: offsetWidth и offsetHeight, обе перезаписываются перед использованием.

    Предупреждение 3

    V6022 Parameter 'shortcut' is not used inside constructor body. DeclarativeTrackingAction.java(47)

    public DeclarativeTrackingAction(String id, String caption, String description,
                                     String icon, String enable, String visible,
                                     String methodName, @Nullable String shortcut,
                                     ActionsHolder holder) {
      super(id);
      this.caption = caption;
      this.description = description;
      this.icon = icon;
    
      setEnabled(enable == null || Boolean.parseBoolean(enable));
      setVisible(visible == null || Boolean.parseBoolean(visible));
    
      this.methodName = methodName;
      checkActionsHolder(holder);
    }

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

    Ещё несколько подобных мест:

    • V6022 Parameter 'type' is not used inside constructor body. QueryNode.java(36)
    • V6022 Parameter 'text2' is not used inside constructor body. MarkerAddition.java(22)
    • V6022 Parameter 'selection' is not used inside constructor body. AceEditor.java(114)
    • V6022 Parameter 'options' is not used inside constructor body. EntitySerialization.java(379)

    Разные функции с одинаковым кодом


    Предупреждение 1

    V6032 It is odd that the body of method 'firstItemId' is fully equivalent to the body of another method 'lastItemId'. ContainerTableItems.java(213), ContainerTableItems.java(219)

    @Override
    public Object firstItemId() {
      List<E> items = container.getItems();
      return items.isEmpty() ? null : items.get(0).getId();
    }
    
    @Override
    public Object lastItemId() {
      List<E> items = container.getItems();
      return items.isEmpty() ? null : items.get(0).getId();
    }

    Функции firstItemId и lastItemId имеют одинаковую реализацию. Скорее всего, в последней необходимо было получать элемент не с индексом 0, а вычислять индекс последнего элемента.

    Предупреждение 2

    V6032 It is odd that the body of method is fully equivalent to the body of another method. SearchComboBoxPainter.java(495), SearchComboBoxPainter.java(501)

    private void paintBackgroundDisabledAndEditable(Graphics2D g) {
      rect = decodeRect1();
      g.setPaint(color53);
      g.fill(rect);
    }
    
    private void paintBackgroundEnabledAndEditable(Graphics2D g) {
      rect = decodeRect1();
      g.setPaint(color53);
      g.fill(rect);
    }

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

    Обращение по нулевой ссылке


    Предупреждение 1

    V6060 The 'descriptionPopup' reference was utilized before it was verified against null. SuggestPopup.java(252), SuggestPopup.java(251)

    protected void updateDescriptionPopupPosition() {
      int x = getAbsoluteLeft() + WIDTH;
      int y = getAbsoluteTop();
      descriptionPopup.setPopupPosition(x, y);
      if (descriptionPopup!=null) {
        descriptionPopup.setPopupPosition(x, y);
      }
    }

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

    Предупреждения 2

    V6060 The 'tableModel' reference was utilized before it was verified against null. DesktopAbstractTable.java(1580), DesktopAbstractTable.java(1564)

    protected Column addRuntimeGeneratedColumn(String columnId) {
      // store old cell editors / renderers
      TableCellEditor[] cellEditors =
        new TableCellEditor[tableModel.getColumnCount() + 1];         // <=
      TableCellRenderer[] cellRenderers =
        new TableCellRenderer[tableModel.getColumnCount() + 1];       // <=
    
      for (int i = 0; i < tableModel.getColumnCount(); i++) {         // <=
          Column tableModelColumn = tableModel.getColumn(i);
    
          if (tableModel.isGeneratedColumn(tableModelColumn)) {       // <=
              TableColumn tableColumn = getColumn(tableModelColumn);
              cellEditors[i] = tableColumn.getCellEditor();
              cellRenderers[i] = tableColumn.getCellRenderer();
          }
      }
    
      Column col = new Column(columnId, columnId);
      col.setEditable(false);
    
      columns.put(col.getId(), col);
    
      if (tableModel != null) {                                       // <=
          tableModel.addColumn(col);
      }
      ....
    }

    Похожая ситуация и в этой функции. После многочисленных обращений к объекту tableModel выполняется проверка, равен он null или нет.

    Ещё один пример:

    • V6060 The 'tableModel' reference was utilized before it was verified against null. DesktopAbstractTable.java(596), DesktopAbstractTable.java(579)

    Возможно, логическая ошибка


    V6026 This value is already assigned to the 'sortAscending' variable. CubaScrollTableWidget.java(488)

    @Override
    protected void sortColumn() {
      ....
      if (sortAscending) {
        if (sortClickCounter < 2) {
          // special case for initial revert sorting instead of reset sort order
          if (sortClickCounter == 0) {
            client.updateVariable(paintableId, "sortascending", false, false);
          } else {
            reloadDataFromServer = false;
            sortClickCounter = 0;
            sortColumn = null;
            sortAscending = true;   // <=
    
            client.updateVariable(paintableId, "resetsortorder", "", true);
          }
        } else {
          client.updateVariable(paintableId, "sortascending", false, false);
        }
      } else {
        if (sortClickCounter < 2) {
          // special case for initial revert sorting instead of reset sort order
          if (sortClickCounter == 0) {
            client.updateVariable(paintableId, "sortascending", true, false);
          } else {
            reloadDataFromServer = false;
            sortClickCounter = 0;
            sortColumn = null;
            sortAscending = true;
    
            client.updateVariable(paintableId, "resetsortorder", "", true);
          }
        } else {
          reloadDataFromServer = false;
          sortClickCounter = 0;
          sortColumn = null;
          sortAscending = true;
    
          client.updateVariable(paintableId, "resetsortorder", "", true);
        }
      }
      ....
    }

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

    Похожий пример из другого файла:

    • V6026 This value is already assigned to the 'sortAscending' variable. CubaTreeTableWidget.java(444)

    Странные возвращаемые значения функций


    Предупреждение 1

    V6037 An unconditional 'return' within a loop. QueryCacheManager.java(128)

    public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) {
      ....
      for (Object id : queryResult.getResult()) {
        return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
      }
      ....
    }

    Анализатор обнаружил безусловный вызов оператора return на первой же итерации цикла for. Возможно, тут ошибка, или нужно переписать код на использование оператора if.

    Предупреждение 2

    V6014 It's odd that this method always returns one and the same value. DefaultExceptionHandler.java(40)

    @Override
    public boolean handle(ErrorEvent event, App app) {
      Throwable t = event.getThrowable();
      if (t instanceof SocketException
          || ExceptionUtils.getRootCause(t) instanceof SocketException) {
        return true;
      }
      if (ExceptionUtils.getThrowableList(t).stream()
          .anyMatch(o -> o.getClass().getName().equals("...."))) {
        return true;
      }
      if (StringUtils.contains(ExceptionUtils.getMessage(t), "....")) {
        return true;
      }
      AppUI ui = AppUI.getCurrent();
      if (ui == null) {
        return true;
      }
      if (t != null) {
        if (app.getConnection().getSession() != null) {
          showDialog(app, t);
        } else {
          showNotification(app, t);
        }
      }
      return true;
    }

    Эта функция во всех случаях возвращает значение true. Но вот в самой последней строчке напрашивается возврат значения false. Возможно, тут допущена ошибка.

    Весь список подозрительных функций с похожим кодом:

    • V6014 It's odd that this method always returns one and the same value. ErrorNodesFinder.java(31)
    • V6014 It's odd that this method always returns one and the same value. FileDownloadController.java(69)
    • V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(73)
    • V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(48)
    • V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(67)
    • V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(46)
    • V6014 It's odd that this method always returns one and the same value. JoinVariableNode.java(57)

    Предупреждение 3

    V6007 Expression 'needReload' is always false. WebAbstractTable.java(2702)

    protected boolean handleSpecificVariables(Map<String, Object> variables) {
      boolean needReload = false;
    
      if (isUsePresentations() && presentations != null) {
        Presentations p = getPresentations();
    
        if (p.getCurrent() != null && p.isAutoSave(p.getCurrent())
            && needUpdatePresentation(variables)) {
          Element e = p.getSettings(p.getCurrent());
          saveSettings(e);
          p.setSettings(p.getCurrent(), e);
        }
      }
      return needReload;
    }

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

    Предупреждение 4

    V6062 Possible infinite recursion inside the 'isFocused' method. GwtAceEditor.java(189), GwtAceEditor.java(190)

    public final native void focus() /*-{
      this.focus();
    }-*/;
    
    public final boolean isFocused() {
      return this.isFocused();
    }

    Анализатор обнаружил функцию, которая вызывается рекурсивно без условия остановки рекурсии. В этом файле много функций, которые помечены ключевым словом native и содержат закомментированный код. Скорее всего, файл в данный момент переписывается и вскоре разработчики обратят внимание и на функцию isFocused.

    Разные предупреждения


    Предупреждение 1

    V6002 The switch statement does not cover all values of the 'Operation' enum: ADD. DesktopAbstractTable.java(665)

    /**
     * Operation which caused the datasource change.
     */
    enum Operation {
        REFRESH,
        CLEAR,
        ADD,
        REMOVE,
        UPDATE
    }
    
    @Override
    public void setDatasource(final CollectionDatasource datasource) {
      ....
      collectionChangeListener = e -> {
        switch (e.getOperation()) {
          case CLEAR:
          case REFRESH:
            fieldDatasources.clear();
            break;
    
          case UPDATE:
          case REMOVE:
            for (Object entity : e.getItems()) {
              fieldDatasources.remove(entity);
            }
            break;
        }
      };
      ....
    }

    В операторе switch не рассмотрено значение ADD. Оно является единственным нерассмотренным, поэтому стоит проверить, ошибка это или нет.

    Предупреждение 2

    V6021 Variable 'source' is not used. DefaultHorizontalLayoutDropHandler.java(177)

    @Override
    protected void handleHTML5Drop(DragAndDropEvent event) {
      LayoutBoundTransferable transferable = (LayoutBoundTransferable) event
              .getTransferable();
      HorizontalLayoutTargetDetails details = (HorizontalLayoutTargetDetails) event
              .getTargetDetails();
      AbstractOrderedLayout layout = (AbstractOrderedLayout) details
              .getTarget();
      Component source = event.getTransferable().getSourceComponent(); // <=
      int idx = (details).getOverIndex();
    
      HorizontalDropLocation loc = (details).getDropLocation();
      if (loc == HorizontalDropLocation.CENTER
              || loc == HorizontalDropLocation.RIGHT) {
          idx++;
      }
      Component comp = resolveComponentFromHTML5Drop(event);
      if (idx >= 0) {
        layout.addComponent(comp, idx);
      } else {
        layout.addComponent(comp);
      }
      if (dropAlignment != null) {
        layout.setComponentAlignment(comp, dropAlignment);
      }
    }

    В коде объявляется и не используется переменная source. Возможно, как и другую переменную comp этого же типа, source забыли добавить в layout.

    Ещё функции с неиспользуемыми переменными:

    • V6021 Variable 'source' is not used. DefaultHorizontalLayoutDropHandler.java(175)
    • V6021 The value is assigned to the 'r' variable but is not used. ExcelExporter.java(262)
    • V6021 Variable 'over' is not used. DefaultCssLayoutDropHandler.java(49)
    • V6021 Variable 'transferable' is not used. DefaultHorizontalLayoutDropHandler.java(171)
    • V6021 Variable 'transferable' is not used. DefaultHorizontalLayoutDropHandler.java(169)
    • V6021 Variable 'beanLocator' is not used. ScreenEventMixin.java(28)

    Предупреждение 3

    V6054 Classes should not be compared by their name. MessageTools.java(283)

    public boolean hasPropertyCaption(MetaProperty property) {
      Class<?> declaringClass = property.getDeclaringClass();
      if (declaringClass == null)
        return false;
    
      String caption = getPropertyCaption(property);
      int i = caption.indexOf('.');
      if (i > 0 && declaringClass.getSimpleName().equals(caption.substring(0, i)))
        return false;
      else
        return true;
    }

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

    Отзыв разработчиков CUBA Platform


    Безусловно, в любом крупном проекте бывают баги. Именно поэтому мы с радостью согласились на предложение команды PVS-Studio проверить наш проект. В репозиторий CUBA включены форки некоторых сторонних OSS библиотек под лицензией Apache 2 и, кажется, нам нужно уделить этому коду больше внимания, анализатор нашёл довольно много проблем в этих исходниках. Сейчас мы используем SpotBugs в качестве основного анализатора, и он не находит некоторые существенные проблемы, найденные PVS-Studio. Самое время пойти и написать дополнительные проверки самим. Большое спасибо команде PVS-Studio за проделанную работу.

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

    Ещё команда PVS-Studio не может пройти мимо фразы «самое время пойти и написать дополнительные проверки самим» и не оставить эту картинку :)

    Picture 1

    Заключение


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



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code of CUBA Platform with PVS-Studio
    PVS-Studio
    161.86
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 9

      +5

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


        StringBuilder orderBy = new StringBuilder();
        ....
        if (orderBy.length() > 0) {
            orderBy.delete(orderBy.length() - 2, orderBy.length());
            orderBy.insert(0, " order by ");
        }

      Сразу же видна разница в экспертизе :-) Я себе мгновенно представил, что в опущенном коде происходит: там в цикле что-то дописывается в StringBuilder, и в конец приписывается ", ". И так и оказалось! В этом StringBuilder никогда не может быть одного символа. Либо ноль (если в цикл ни разу не зашли), либо не меньше двух (потому что как минимум одна запятая с пробелом). Вы можете сколько угодно говорить, что код выглядит опасно, ненадёжно и т. д., но это чистый false-positive.


      @Override
      public boolean handle(ErrorEvent event, App app) {
        Throwable t = event.getThrowable();
       ..
        return true;

      Хвастаетесь же, что у вас по десять исключений в каждой инспекции :-) Этот метод реализует метод интерфейса. В этом случае он действует в соответствии с документацией интерфейса: всегда возвращает true, сигнализируя о том, что исключение всегда обработано. В данном случае это не ошибка. Не стоит выдавать это предупреждение для реализаций интерфейсов.


      public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) {
        ....
        for (Object id : queryResult.getResult()) {
          return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
        }
        ....
      }

      Для циклов for-each это может быть спорный, но вполне используемый паттерн в джаве. Аналогичный код без цикла будет:


        Iterator<?> it = queryResult.getResult().iterator();
        if (it.hasNext()) {
          Object id = it.next();
          return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
        }

      Итог: две дополнительных строчки и дополнительная переменная в скоупе. И ради чего? Мы по умолчанию в данном случае предупреждение не выдаём (хотя опция есть выдавать), потому что тех, кто так пишет, можно понять. Во всех остальных типах циклов кроме for-each предупреждение выдаётся.


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

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




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

        0
          StringBuilder orderBy = new StringBuilder();
          ....
          if (orderBy.length() > 0) {
            ....
          }
        Я со своей колокольни не назвал бы это ложным предупреждением.
        Допустим, над проектом поработает не очень опытный разработчик, или просто ужасно спешащий, который закомментирует дописывание в StringBuilder символов.
        Или спецификация измениться, и разработчик не удосужиться проскроллить текст функции вниз.
        В общем ИМХО для чистоты я бы код таки поправил.
          +2
          Вот когда закомментирует, тогда и надо выдавать предупреждение. А для чистоты надо на Stream API переписать, что авторы кода уже и сделали.
            0
            Вы издеваетесь, да?
            Когда закомментят это уже будет ошибка.
            А идея анализатора — уберечь от существующих и будущих ошибок.
              +3
              Это вы, похоже, издеваетесь. Давайте в пустом проекте подчеркнём и напишем «в этом проекте в будущем возможны ошибки». Любой код можно испортить так, чтобы там была ошибка в будущем.
        +3

        Интересно, кстати, это вы вручную преаннотировали org.apache.commons.lang3.StringUtils.isNotEmpty? Вряд ли у вас такой крутой межпроцедурный анализ :-)

          0
          А можно и в IDEA это как-то проаннотировать для всех разработчиков в проекте? Догадываюсь, что можно контракты как-то сохранить в XML файлах проекта, но буду рад ссылкам на мануал.
            +2

            У нас можно аннотировать контракты через действие Edit method contract, и они сохраняются в специальные XML-ки. Их можно как коммитить в VCS, так и деплоить в бинарный репозиторий и подкачивать вместе с библиотекой. Вот тут можно почитать про внешние аннотации, а вот тут конкретно про контракты.


            К сожалению, такой контракт как у isNotEmpty целиком не опишешь внешней аннотацией, можно только null -> false написать, но полностью поведение метода не специфицируешь. Такие методы мы можем захардкодить при необходимости (внутренний язык контрактов богаче, чем опубликованный для всех). Есть предложение расширить общий язык контрактов, чтобы можно было писать (param1 != null && param1.length() > 0) -> true; () -> false, но никаких обещаний, будет ли это сделано, когда и в какой форме.

              0
              Будем ждать контракты commons-lang3 в IntelliJ )

        Only users with full accounts can post comments. Log in, please.