В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальнейшие планы. И, конечно, в статье будут приведены первые испытания анализатора на открытых проектах.
PVS-Studio
Для Java разработчиков, которые ранее не слышали об инструменте PVS-Studio, приведу совсем краткое его описание.
PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в среде Windows, Linux и macOS.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять дефекты. Тем, кто интересуется, как именно PVS-Studio ищет ошибки, предлагаю ознакомиться со статьёй "Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей".
Начало
Я бы мог придумать умную историю, как мы два года размышляли о том, какой следующий язык поддержать в PVS-Studio. О том, что Java — это разумный выбор, основанный на высокой популярности этого языка и так далее.
Однако как это бывает в жизни, всё решил не глубокий анализ, а эксперимент :). Да, мы размышляли, в какую сторону дальше развивать анализатор PVS-Studio. Рассматривались такие языки программирования, как: Java, PHP, Python, JavaScript, IBM RPG. Причём мы склонялись именно к языку Java, но окончательный выбор так ещё и не был сделан. Тех, у кого взгляд застрял на незнакомом IBM RPG, отсылаю к вот этой заметке, из которой всё станет ясно.
В конце 2017 года коллега Егор Бредихин посмотрел, какие есть готовые библиотеки разбора кода (проще говоря — парсеры) под интересные нам новые направления. И наткнулся на несколько проектов для разбора Java кода. На основе Spoon, ему довольно быстро удалось сделать прототип анализатора с парой диагностик. Более того, стало понятно, что мы сможем использовать в Java анализаторе некоторые механизмы C++ анализатора с помощью SWIG. Мы посмотрели на то, что получилось, и поняли, что наш следующий анализатор будет для Java.
Спасибо Егору за его начинание и активную работу, проделанную им над Java анализатором. Как именно шла разработка он описал в статье "Разработка нового статического анализатора: PVS-Studio Java".
Конкуренты?
В мире существует множество бесплатных и коммерческих статических анализаторов кода для Java. Перечислять их все в статье не имеет смысла, и я просто оставлю ссылку на "List of tools for static code analysis" (смотрите раздел Java и Multi-language).
Однако я знаю, что в первую очередь нас спросят про IntelliJ IDEA, FindBugs и SonarQube (SonarJava).
IntelliJ IDEA
В IntelliJ IDEA встроен очень мощный статический анализатор кода. Причем анализатор развивается, а его авторы внимательно следят за нашей деятельностью. С IntelliJ IDEA нам будет сложнее всего. Превзойти IntelliJ IDEA в диагностических возможностях мы не сможем, по крайней мере, сейчас. Поэтому мы постараемся сконцентрироваться на других наших преимуществах.
Статический анализ в IntelliJ IDEA – это, в первую очередь, одна из фишек среды разработки, что накладывает на него определённые ограничения. Мы же свободны в том, что можем делать с нашим анализатором. Например, мы можем быстро адаптировать анализатор под конкретные нужды заказчика. Быстрая и глубокая поддержка — наше конкурентное преимущество. Наши клиенты напрямую общаются непосредственно с программистами, разрабатывающими ту или иную часть PVS-Studio.
В PVS-Studio много возможностей по интеграции его в цикл разработки больших старых проектов. Это интеграция с SonarQube. Это массовое подавление сообщений анализатора, что позволяет сразу начать использовать анализатор в большом проекте для отслеживания ошибок только в новом или изменённом коде. PVS-Studio встраивается в процесс непрерывной интеграции. Думаю, именно эти и другие возможности помогут нашему анализатору найти место под солнцем в Java мире.
FindBugs
Проект FindBugs заброшен. Но его следует вспомнить по той причине, что это, пожалуй, наиболее известный бесплатный статический анализатор Java кода.
Преемником FindBugs можно назвать проект SpotBugs. Однако он менее популярен, и что с ним будет, тоже пока не совсем понятно.
В общем, мы считаем, что, хотя FindBugs был и остаётся крайне популярным, да к тому же ещё и бесплатным анализатором, думать про него нам не следует. Этот проект просто тихо и спокойно уйдёт в прошлое.
P.S. Кстати, теперь PVS-Studio также можно использовать бесплатно при работе с открытыми проектами.
SonarQube (SonarJava)
Мы считаем, что не конкурируем с SonarQube, а дополняем его. PVS-Studio интегрируется с SonarQube, что позволяет разработчикам находить большее количество ошибок и потенциальных уязвимостей в своих проектах. Как интегрировать в SonarQube инструмент PVS-Studio и другие анализаторы, мы регулярно рассказываем на мастер-классах, которые проводим в рамках различных конференций (пример).
Как запустить PVS-Studio для Java
Мы сделали доступными для пользователей самые популярные способы интеграции анализатора в сборочную систему:
- Плагин для Maven;
- Плагин для Gradle;
- Плагин для IntelliJ IDEA
На этапе тестирования мы встретили много пользователей, имеющих самописные сборочные системы, особенно в мобильной разработке. Им понравилась возможность запускать анализатор напрямую, перечислив исходники и classpath.
Подробную информацию о всех способах запуска анализатора вы можете найти на странице документации "Как запустить PVS-Studio Java".
Мы не могли обойти стороной платформу контроля качества кода SonarQube, так популярную среди Java разработчиков, поэтому добавили поддержку языка Java в наш плагин для SonarQube.
Дальнейшие планы
У нас есть много идей, требующих дополнительного изучения, но некоторые планы, характерные для любого из наших анализаторов, выглядят так:
- Создание новых диагностик и доработка существующих;
- Развитие Dataflow-анализа;
- Повышение надёжности и удобства использования.
Возможно, мы найдём время адаптировать плагин IntelliJ IDEA для CLion. Привет C++ разработчикам, читающим про анализатор Java :-)
Примеры ошибок, найденных в открытых проектах
Я буду не я, если не покажу в статье какие-то ошибки, найденные с помощью нового анализатора. Можно было бы взять какой-то большой открытый Java-проект и написать классическую статью с разбором ошибок, как мы обычно и делаем.
Однако я сразу предвижу вопросы, а сможем ли мы найти что-то в таких проектах как IntelliJ IDEA, FindBugs и так далее. Поэтому у меня просто нет выхода, и я начну именно с этих проектов. Итак, я решил бегло проверить и выписать несколько интересных примеров ошибок из следующих проектов:
- IntelliJ IDEA Community Edition. Думаю, не нужно объяснять, почему был выбран этот проект :).
- SpotBugs. Как я уже писал ранее, проект FindBugs не развивается. Поэтому заглянем в проект SpotBugs, который является преемником FindBugs. SpotBugs — это классический статический анализатор Java-кода.
- Что-то из проектов компании SonarSource, которая разрабатывает программное обеспечение для непрерывного контроля качества кода. Заглянем в проект SonarQube и SonarJava.
Написать про баги в этих проектах является сложной задачей. Дело в том, что эти проекты очень качественные. Собственно, это не удивительно. Как показывают наши наблюдения, код статических анализаторов всегда хорошо протестирован и проверен с помощью других инструментов.
Несмотря на всё это, мне придётся начать именно с этих проектов. Второго шанса написать что-то про них у меня не будет. Я уверен, что после выхода релиза PVS-Studio для Java, разработчики перечисленных проектов возьмут PVS-Studio на вооружение и начнут его использовать для регулярных или, по крайней мере, для периодических проверок своего кода. Например, я знаю, что Тагир Валеев (lany), один из разработчиков JetBrains, занимающийся статическим анализатором кода IntelliJ IDEA, в тот момент, пока я пишу статью, уже во всю играется с Beta-версией PVS-Studio. Он написал нам уже около 15 писем с баг-репортами и рекомендациями. Спасибо, Тагир!
К счастью, мне не требуется найти как можно больше ошибок в одном конкретном проекте. Сейчас моя задача показать, что анализатор PVS-Studio для Java появился не зря и сможет пополнить линейку других инструментов, предназначенных для улучшения качества кода. Я просто бегло просмотрел отчёты анализатора и выписал несколько ошибок, которые показались мне интересными. По возможности я старался выписывать ошибки разного типа. Давайте посмотрим, что получилось.
IntelliJ IDEA, целочисленное деление
private static boolean checkSentenceCapitalization(@NotNull String value) {
List<String> words = StringUtil.split(value, " ");
....
int capitalized = 1;
....
return capitalized / words.size() < 0.2; // allow reasonable amount of
// capitalized words
}Предупреждение PVS-Studio: V6011 [CWE-682] The '0.2' literal of the 'double' type is compared to a value of the 'int' type. TitleCapitalizationInspection.java 169
По задумке, функция должна возвращать истину, если менее 20% слов начинаются с заглавной буквы. На самом деле, проверка не работает, так как происходит целочисленное деление. В результате деления можно получить только два значения: 0 или 1.
Функция вернёт ложное значение, только если все слова будут начинаться с заглавной буквы. Во всех остальных случаях при делении будет получаться 0, и функция будет возвращать истинное значение.
IntelliJ IDEA, подозрительный цикл
public int findPreviousIndex(int current) {
int count = myPainter.getErrorStripeCount();
int foundIndex = -1;
int foundLayer = 0;
if (0 <= current && current < count) {
current--;
for (int index = count - 1; index >= 0; index++) { // <=
int layer = getLayer(index);
if (layer > foundLayer) {
foundIndex = index;
foundLayer = layer;
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'index >= 0' is always true. Updater.java 184
Вначале посмотрите на условие (0 <= current && current < count). Оно выполняется только в том случае, если значение переменной count больше 0.
Теперь посмотрим на цикл:
for (int index = count - 1; index >= 0; index++)Переменная index инициализируется выражением count — 1. Так как переменная count больше 0, то начальное значение переменной index всегда больше или равно 0. Получается, что цикл будет выполняться до тех пор, пока не произойдёт переполнение переменной index.
Скорее всего, это просто опечатка и должен выполняться не инкремент, а декремент переменной:
for (int index = count - 1; index >= 0; index--)IntelliJ IDEA, Copy-Paste
@NonNls public static final String BEFORE_STR_OLD = "before:";
@NonNls public static final String AFTER_STR_OLD = "after:";
private static boolean isBeforeOrAfterKeyword(String str, boolean trimKeyword) {
return (trimKeyword ? LoadingOrder.BEFORE_STR.trim() :
LoadingOrder.BEFORE_STR).equalsIgnoreCase(str) ||
(trimKeyword ? LoadingOrder.AFTER_STR.trim() :
LoadingOrder.AFTER_STR).equalsIgnoreCase(str) ||
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) || // <=
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str); // <=
}Предупреждение PVS-Studio: V6001 [CWE-570] There are identical sub-expressions 'LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str)' to the left and to the right of the '||' operator. Check lines: 127, 128. ExtensionOrderConverter.java 127
Старый добрый эффект последней строки. Программист поторопился и, размножив строчку кода, забыл её исправить. В результате, дважды строка str сравнивается с BEFORE_STR_OLD. Скорее всего, одно из сравнений должно быть с AFTER_STR_OLD.
IntelliJ IDEA, опечатка
public synchronized boolean isIdentifier(@NotNull String name,
final Project project) {
if (!StringUtil.startsWithChar(name,'\'') &&
!StringUtil.startsWithChar(name,'\"')) {
name = "\"" + name;
}
if (!StringUtil.endsWithChar(name,'"') &&
!StringUtil.endsWithChar(name,'\"')) {
name += "\"";
}
....
}Предупреждение PVS-Studio: V6001 [CWE-571] There are identical sub-expressions '!StringUtil.endsWithChar(name,'"')' to the left and to the right of the '&&' operator. JsonNamesValidator.java 27
Данный фрагмент кода проверяет, что имя взято в одинарные или двойные кавычки. Если это не так, то двойные кавычки добавляются автоматически.
Из-за опечатки, окончание имени проверяется только на наличие двойных кавычек. В результате, имя, взятое в одинарные кавычки, будет обработано некорректно.
Имя
'Abcd'из-за добавления лишних двойных кавычек превратится в:
'Abcd'"IntelliJ IDEA, неправильная защита от выхода за границу массива
static Context parse(....) {
....
for (int i = offset; i < endOffset; i++) {
char c = text.charAt(i);
if (c == '<' && i < endOffset && text.charAt(i + 1) == '/'
&& startTag != null
&& CharArrayUtil.regionMatches(text, i + 2, endOffset, startTag))
{
endTagStartOffset = i;
break;
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'i < endOffset' is always true. EnterAfterJavadocTagHandler.java 183
Подвыражение i < endOffset в условии оператора if не имеет смысла. Переменная i и так всегда меньше endOffset, что следует из условия выполнения цикла.
Скорее всего, программист хотел защититься от выхода за границу строки при вызове функций:
- text.charAt(i + 1)
- CharArrayUtil.regionMatches(text, i + 2, endOffset, startTag)
В этом случае подвыражение для проверки индекса должно быть таким: i < endOffset — 2.
IntelliJ IDEA, повторяющаяся проверка
public static String generateWarningMessage(....)
{
....
if (buffer.length() > 0) {
if (buffer.length() > 0) {
buffer.append(" ").append(
IdeBundle.message("prompt.delete.and")).append(" ");
}
}
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'buffer.length() > 0' is always true. DeleteUtil.java 62
Это может быть как безобидным избыточным кодом, так и серьезной ошибкой.
Если проверка-дубликат появилась случайно, например, в ходе рефакторинга, то ничего плохого в этом нет. Вторую проверку можно просто удалить.
Но возможен и другой сценарий. Вторая проверка должна быть совсем другой и код ведёт себя не так, как задумано. Тогда это настоящая ошибка.
Примечание. Кстати, разных избыточных проверок находится очень много. Причём часто видно, что это не ошибка. Однако и назвать сообщения анализатора ложными срабатываниями тоже нельзя. Для пояснения приведу вот такой пример, также взятый из IntelliJ IDEA:
private static boolean isMultiline(PsiElement element) {
String text = element.getText();
return text.contains("\n") || text.contains("\r") || text.contains("\r\n");
}Анализатор говорит, что функция text.contains("\r\n") всегда возвращает ложь. И действительно, если не найден символ "\n" и "\r", то и нет смысла искать "\r\n". Это не ошибка, и код плох только тем, что работает чуть медленнее, выполняя бессмысленный поиск подстроки.
Как быть с подобным кодом, в каждом конкретном случае уже решать программистам. Я при написании статей, как правило, просто не обращаю внимания на подобный код.
IntelliJ IDEA, что-то не так
public boolean satisfiedBy(@NotNull PsiElement element) {
....
@NonNls final String text = expression.getText().replaceAll("_", "");
if (text == null || text.length() < 2) {
return false;
}
if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {
return false;
}
return text.charAt(0) == '0';
}Предупреждение PVS-Studio: V6007 [CWE-570] Expression '«0».equals(text)' is always false. ConvertIntegerToDecimalPredicate.java 46
Этот код точно содержит логическую ошибку. Но я затрудняюсь сказать, что именно хотел проверить программист, и как следует исправлять недочёт. Поэтому здесь я только укажу на бессмысленную проверку.
В начале проверятся, что строка должна содержать не менее двух символов. Если это не так, то функция возвращает false.
Далее следует проверка «0».equals(text). Она бессмысленна, так как строка не может содержать только один символ.
В общем, что-то здесь не так и код следует поправить.
SpotBugs (преемник FindBugs), ошибка ограничения по количеству итераций
public static String getXMLType(@WillNotClose InputStream in) throws IOException
{
....
String s;
int count = 0;
while (count < 4) {
s = r.readLine();
if (s == null) {
break;
}
Matcher m = tag.matcher(s);
if (m.find()) {
return m.group(1);
}
}
throw new IOException("Didn't find xml tag");
....
}Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'count < 4' is always true. Util.java 394
По задумке, поиск xml-тега должен осуществляться только в первых четырёх строчках файла. Но из-за того, что забыли инкрементировать переменную count, будет прочитан весь файл.
Во-первых, это может оказаться очень медленной операцией, а во-вторых, где-то в середине файла может быть найдено нечто, что будет воспринято как xml-тег, но при этом таковым являться не будет.
SpotBugs (преемник FindBugs), затирание значения
private void reportBug() {
int priority = LOW_PRIORITY;
String pattern = "NS_NON_SHORT_CIRCUIT";
if (sawDangerOld) {
if (sawNullTestVeryOld) {
priority = HIGH_PRIORITY; // <=
}
if (sawMethodCallOld || sawNumericTestVeryOld && sawArrayDangerOld) {
priority = HIGH_PRIORITY; // <=
pattern = "NS_DANGEROUS_NON_SHORT_CIRCUIT";
} else {
priority = NORMAL_PRIORITY; // <=
}
}
bugAccumulator.accumulateBug(
new BugInstance(this, pattern, priority).addClassAndMethod(this), this);
}Предупреждение PVS-Studio: V6021 [CWE-563] The value is assigned to the 'priority' variable but is not used. FindNonShortCircuit.java 197
Значение переменной priority выставляется в зависимости от значения переменной sawNullTestVeryOld. Однако это не играет никакой роли. Далее переменной priority в любом случае будет присвоено другое значение. Явная ошибка в логике работы функции.
SonarQube, Copy-Paste
public class RuleDto {
....
private final RuleDefinitionDto definition;
private final RuleMetadataDto metadata;
....
private void setUpdatedAtFromDefinition(@Nullable Long updatedAt) {
if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
setUpdatedAt(updatedAt);
}
}
private void setUpdatedAtFromMetadata(@Nullable Long updatedAt) {
if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
setUpdatedAt(updatedAt);
}
}
....
}PVS-Studio: V6032 It is odd that the body of method 'setUpdatedAtFromDefinition' is fully equivalent to the body of another method 'setUpdatedAtFromMetadata'. Check lines: 396, 405. RuleDto.java 396
В методе setUpdatedAtFromMetadata используется поле definition. Скорее всего, должно использоваться поле metadata. Это очень похоже на последствия неудачного Copy-Paste.
SonarJava, дубликаты при инициализации Map
private final Map<JavaPunctuator, Tree.Kind> assignmentOperators =
Maps.newEnumMap(JavaPunctuator.class);
public KindMaps() {
....
assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
....
assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
....
}Предупреждение PVS-Studio: V6033 [CWE-462] An item with the same key 'JavaPunctuator.PLUSEQU' has already been added. Check lines: 104, 100. KindMaps.java 104
Дважды в карту помещается одна и та же пара ключ-значение. Скорее всего, это получилось по невнимательности, и на самом деле никакой настоящей ошибки нет. Однако этот код в любом случает требуется проверить, так как, возможно, забыли добавить какую-то другую пару.
Заключение
Да какое тут может быть заключение?! Приглашаю всех, не откладывая, скачать PVS-Studio и попробовать проверить свои рабочие проекты на языке Java! Скачать PVS-Studio.
Спасибо всем за внимание. Надеюсь, скоро мы порадуем читателей циклом статей, посвящённых проверке различных открытых Java-проектов.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio for Java.
Комментарии (39)
lany
17.01.2019 14:55+7Да, тут напрашивается ответ, конечно :-) Во-первых, поздравляю вас с релизом, вы молодцы и делаете нужное дело. Пройдёмся по предупреждениям
return capitalized / words.size() < 0.2;Это наглядный пример, что конкуренция — двигатель прогресса! Я заметил во время бета-тестирования, что вы тут умнее нас. Но очевидно, это не ваше достоинство, а наша недоработка :-) Я сообщил ответственному человеку, и тот уже исправил, в следующей версии будем это репортить.
for (int index = count - 1; index >= 0; index++) {Мы знаем, что
countв этом месте положительный иcount - 1неотрицательный. Кстати, в этом можно убедиться, нажав в IDE два раза Ctrl+Shift+P:
Однако наш dataflow аккуратно вычисляет все переполнения и не считает, что условие цикла всегда истинно, потому что это не так.
Зато недавно мы сделали другую инспекцию, которая здесь срабатывает. В принципе никакой разницы нет, с какого значения мы начинаем этот цикл, главное — это связка операторов в условии и инкременте. Связки
>/++и</--— это почти всегда баги, о которых и сообщает новая инспекция. Будет в следующей версии. Так что эту проблему мы находим, хоть и по-другому.
LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) ||и следующаяЭта инспекция почему-то выключена по умолчанию. Надо будет разобраться с этим. Если работает нормально, то включим.
for (int i = offset; i < endOffset; i++)
Это мы видим, да. Но исправить всем лень. И, кстати, вы правы, я смог сломать Идейку, вызвав исключение в этом месте! Если кому интересно, надо в конце файла набрать
/** * <p><
Встать курсором между
><и нажать Enter! Если вы это повторите, у вас замигает в правом нижнем углу восклицательный знак. Report to JetBrains нажимать необязательно, мы уже в курсе :-) Вот, кстати, вам хороший пример пользы от статического анализа. Теперь-то исправим.
if (buffer.length() > 0) {Это видим тоже. Не очень давно, кстати, может с прошлой версии. Думаю, в данном случае это действительно просто лишняя проверка.
text.contains("\n") || text.contains("\r") || text.contains("\r\n");
Вот это прям очень круто было. Если у вас dataflow это выводит, а не паттерн, то поднимаю лапки и признаюсь, что нам такое слабо пока.
if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {
Это место я прекрасно помню. Очень радовался больше года назад, когда научил Идейку его находить. А исправить что-то всем лень. Надо заняться, да.
EvgeniyRyzhkov
17.01.2019 14:59+5Лайкнул коммент.
И, Тагир, огромное спасибо за участие в бета-тестировании! Ребята до сих пор разбирают кучу комментариев и рекомендаций, до релиза не все успели проработать разумеется.
tuxi
17.01.2019 22:30Тагир, не знаю, по адресу я обращаюсь или нет. Я года полтора назад репортил баг в валидаторе xslt шаблонов у idea, который реально мешает жить. Там речь про использовании внутри шаблона конструкций из javasript, валидатор ругался на семантику xsl внутри js. Но похоже xslt не в приоритете и его так и не поправили.
lany
18.01.2019 03:47Слушайте, ну если вы ожидаете от другого человека помощи, почему бы не сделать максимум работы самому? Если вы репортили баг, дайте хотя бы ссылку на него. У нас десятки тысяч открытых багов, мне сейчас идти и искать по вашему расплывчатому описанию?
Кстати, если уж хочется привлечь внимание к тикету, есть способы лучше, чем писать разработчику совсем другой подсистемы. Например, твиттер. Особо хитрые жалуются на реддит. Ещё можно подговорить всех коллег проголосовать за баг. Только не сразу, а где-нибудь раз в неделю. Или можно время от времени писать дополнительные комментарии типа "проблема всё ещё актуальна в свежей версии, пожалуйста почините". Бывает, что работает. Только я вам этого не говорил :-)
lany
18.01.2019 05:51Зато недавно мы сделали другую инспекцию, которая здесь срабатывает
Перепутал ссылку на тикет, вот правильная.
stgunholy
17.01.2019 15:06-1Далее следует проверка «0».equals(text). Она бессмысленна, так как строка не может содержать только один символ.
По-моему может спокойно…
igormich88
17.01.2019 15:21+1Там перед этим выход из функции для строк короче двух.
if (text == null || text.length() < 2) { return false; }
datacompboy
17.01.2019 16:06А я только подготовил скрипты-аналоги того же что для плюсов делал, но даже не успел запустить тестовый прогон :(
У меня сейчас лапки, но я буду рад еще раз поклянчить код когда буду готов посвятить этому время продуктивно, а не по часу вечером :(
У вас отличный отлов копипасты, и я уверен что это очень мощная штукень.
stranger777
17.01.2019 16:50Очень приятная новость, особенно с тем учётом, что в последнее время переехал на Джава и назад не очень хочу. Спасибо.
По моим наблюдениям упомянутые здесь анализаторы, в отличие от PVS, занимаются поверхностными проверками: лишнее поле, неверное именование, не рекомендуемое употребление конструкции и пр., тогда как PVS держит «в уме» не столько формальные проверки, сколько смысловые.
И вот вопрос: не приходила ли в голову идея сделать качественный идиоматичный кодогенератор для какого-либо языка? Либо настолько же «вдумчивый» конвертер кода.
Andrey2008 Автор
17.01.2019 17:04Пока мы не думаем в какую-либо сторону кроме анализа кода. Т.е. может быть мы будем думать, дальше в php или javascript, но не про кодогенерацию.
asm0dey
18.01.2019 11:36Это не ошибка, и код плох только тем, что работает чуть медленнее, выполняя бессмысленный поиск подстроки
Это неправда потому что в джаве
||— short-circuitMikailBag
18.01.2019 11:38Если в тексте нет ни \n, ни \r, то будет выполнен третий поиск, который гарантированно ничего не найдет.
Andrey2008 Автор
18.01.2019 12:04+2Как это часто бывает, анализатор вновь оказался внимательнее человека. :) Вот поэтому анализаторы и нужны. :)
lany
18.01.2019 12:35Справедливости ради замечу, что анализатор не сказал «код работает чуть медленнее, чем мог бы», это уже человек, анализируя сообщение, пришёл к такому выводу. И asm0dey оспорил вывод человека, а не программы.
В целом лишнюю проверку вида condition is always true компилятор нередко удалит так же легко, как анализатор заметит, поэтому не всегда имеется ухудшение производительности. В данном случае это, конечно, вряд ли сработает, но классифицировать такие случаи на вредные или нейтральные для производительности ваш анализатор вроде бы пока не научился :-)EvgeniyRyzhkov
18.01.2019 12:39Комментарий для сторонних читаталей, Тагир сам это и так знает конечно.
Вообще все сообщения анализатора следует воспринимать как «Здесь что-то подозрительное, кажется вот это, но не уверен. Проверьте.» Очень нередки ситуации, когда анализатора об ошибке говорит не теми словами, которыми хотелось бы. Но он говорит все же об ошибке.lany
18.01.2019 12:41Всё правильно, но к нашей ситуации не относится. Ещё раз: asm0dey оспорил не предупреждение анализатора, а ухудшение производительности. Предупреждение анализатора он не оспаривал, поэтому утверждать, что анализатор внимательнее человека, в данном случае неверно. Я ровно это хотел сказать.
HighPredator
18.01.2019 16:25Скажите, раз вы смогли успешно интегрироваться в IntelliJ платформу, стоит ли ожидать появление плагина PVS-Studio С++ для CLion?
konsoletyper
19.01.2019 22:09Я так понял, PVS-Studio анализирует исходники. А почему не байт-код, как это делает findbugs? Это позволило бы одним инструментом покрыть много разных JVM-языков. Ведь наверняка PVS-Studio парсит исходники и строит какой-нибудь control flow graph, ищет def-use chains (а то и в SSA всё перегоняет). Всё то же самое можно прекрасно делать с байт-кодом. Или в подобных рассуждениях есть какой-то подвох?
Andrey2008 Автор
19.01.2019 22:16Подвох есть. В байт-коде потеряна часть информации, которая помогает понять, аномален код или нет. Мы решили пусть мы будем анализировать только Java, но зато сделаем это более глубоко.
konsoletyper
19.01.2019 22:18А можно пример кода, который при компиляции даёт байт-код, в котором потеряна информация, необходимая для статического анализа?
Andrey2008 Автор
19.01.2019 22:30+2Конкретные примеры сейчас не покажу, но вот некоторые мысли.
- Может исчезнуть недостижимый код, а, следовательно, он не будет найден. Как узнать, что он там был? Все равно надо анализировать изначальный исходный код.
- Могут исчезать бессмысленные (всегда истинные/ложные) условия. Они могут «оптимизироваться в ничто» и их не будет в исходном коде. Например, можно удалить в условии (x.a == y.a && x.b == x.b) правую часть. Это классическая опечатка и имелось в виду x.b == y.b. А как узнать из байта кода, что что-то было удалено? Все равно надо анализировать изначальный исходный код.
- Упрощение. Выражение (A < 1/2) может превратится в байт коде (A < 0). Как узнать из бйткода, что это подозрительное сравнение? Все равно надо анализировать изначальный исходный код.
- И т.д.
В общем всё сводится к анализу именно исходного кода. Иначе многие виды диагностик недоступны.
Maccimo
20.01.2019 08:35Какие-то сложные примеры с допущениями.
У вас же есть инспекции, учитывающие отступы в исходном тексте?
Отступы в скомпилированном коде исчезают всегда, без всяких «если».
Чтобы два раза не вставать, вопрос: После просмотра этого доклада создалось впечатление, что у вас под капотом не SSA. Если это так, то почему?Andrey2008 Автор
20.01.2019 09:56Механизм, реализованный в PVS-Studio похож на SSA, но является он таковым с формальной точки зрения или нет, я не знаю. Да и не интересно, главное, что решаются нужные нам задачи. По поводу пробелов: да пробелы учитываются, но в очень малом количестве диагностик.
ZyXI
20.01.2019 13:42+1В скомпилированном виде может сохраняться отладочная информация. Именно отступы оттуда вытащить легко, вот только вопрос сколько будет ложных срабатываний из?за особенностей компиляции в байткод. Тем более, что у каждого языка будут свои особенности.
Maccimo
20.01.2019 20:00Если мы говорим о JVM и
*.class-файлах, то каким образом?
Аттрибут LineNumberTable содержит информацию только о номерах строк, аттрибут SourceDebugExtension, насколько я помню, тоже.
UbuRus
Конкуренция — это хорошо, больше инструментов — лучше инструменты.
Я думаю что все что вы нашли, могла найти и Idea. Дело в том что множество инспекций отключено по умолчанию, из-за компромиса между скоростью работы/найденными ошибками.
staticlab
Да, по умолчанию в IDEA многие инспекции выключены. Но, например, в кейсе с целочисленным делением IDEA может только сказать, что сравниваются double и int, тогда как PVS вычисляет области значений и более явно предупреждает о косяке. Случай с циклом IDEA вообще не опознаёт. Ошибку копипаста, да, определить может, но не с настройками по умолчанию. Интересно, что проверки вида "expression is always true/false" производятся уже с настройками по умолчанию, но авторы IDEA их не заметили. Специально отключают эту или все инспекции?
lany
Нет, не отключаем. Думаю, дело в том, что мы сильно толще вас. У нас только в мастер community 500 коммитов в неделю. А если ещё закрытая часть, которая не меньше. Представьте, что я улучшаю инспекцию, и она находит не одно или два, а сто новых подозрительных мест в коде. Это, кстати, не предел, мой личный рекорд — 800 новых предупреждений (речь о инспекциях категории Probable bugs, а не о каких-нибудь стилистических). Причём исправлять их надо вдумчиво, автоматом не получится. И что, бросать всё и начинать править? Часто проблемы находятся в коде пятнадцатилетней давности, который относится к поддержке какого-нибудь устаревшего фреймворка, у которого полтора пользователя теперь, и никто особо не в курсе даже что надо сделать в Идейке, чтобы метод с новым предупреждением выполнился. Да, можно тратить силы на это, но кажется, что можно потратить их более рационально.
Ещё бывает, что мы импортируем кодовую базу. Например, кто-то разрабатывал плагин независимо, а потом мы договорились с автором, устроили его на работу и вставили его код к себе. Возможно, в нём немало предупреждений статического анализатора. Стоит ли в первую очередь их вычищать? Непонятно.
Ну и, к сожалению, местами просто не хватает культуры. Я не коммичу код с новыми варнингами и стараюсь исправлять варнинги в любом коде, который трогаю. Но не все так делают, и это не форсируется у нас. Последнее время мы стали исправлять ситуацию. Некоторые инспекции у нас включены в так называемый список Zero tolerance inspections. Предупреждения от этих инспекций рисуются у нас кроваво-красным цветом, их сложно игнорировать. А если код с таким предупреждением закоммитить, то падает один из билдов, мне приходит письмо, и я с этим разбираюсь. Постепенно мы увеличиваем список таких инспекций, но, конечно, до идеала далеко. В частности, например, сообщений вида condition is always true пока слишком много, чтобы вот так легко их все исправить.
sabio
Было бы интересно взглянуть на профайл инспекций, которые вы используете для проверки своего кода. Понятно, что, в общем случае, там всё зависит от проекта. Но кому как не создателям инспекций лучше знать, какие из них важнее других? Даже если глобально, для всех включить их по-умолчанию не очень оправдано.
Это ведь не какая-то секретная информация?
Или, быть может, это в вашем Community-проекте на GitHub можно посмотреть?
lany
В коммюнити-проекте есть. Но ориентироваться на него неправильно. Во-первых, что-то в этом профиле может быть по историческим причинам. Или каких-то предупреждений слишком много на нашей кодовой базе, и они шумят, но если делаешь проект с нуля, они полезны. Поэтому у нас выключены, а по умолчанию включены.
lany
Бывает до смешного доходит. Пользователь присылает отчёт об ошибке, что в Идейке произошёл NullPointerException, мы смотрим стектрейс, а в этом месте сама Идейка подсвечивает код, что NullPointerException здесь возможен. Мол, «я же вас предупреждала, а вы!»
EvgeniyRyzhkov
А теперь серьезно.
Когда меня спрашивают о том, не сошли ли мы с ума делая статический анализатор кода для Java, я как никогда спокоен и уверен в себе. Да, IDEA – the best (ни капли сарказма), а разработчики из JetBrains – элита индустрии. В этом нет сомнений и мы не претендуем на это.
Но статический анализ кода – это не только собственно технологии анализа кода. Как построить дерево разбора и как по нему пробежаться знают многие. В конце концов это описано в книгах!
Статический анализ кода – это еще и инфраструктура, то есть куча вспомогательных инструментов. Это и методология использования, и, главное, методология внедрения. Философия статического анализа кода – это как потратить деньги на статический анализ с пользой.
Многие думают, что статический анализ – это вот статьи моих коллег про «смотрите какую ошибку мы нашли в Chrome». Это все прикольно, вызывает интерес аудитории, но это вишенка на торте. Сам же торт намного больше и вкуснее.
Этой темой мы занимаемся более 10 лет. И мы в ней сильны. Поэтому нет, я не боюсь конкуренции ни с JetBrains, ни с SonarQube, ни с кем-либо еще. В этой области у нас есть крутейшая экспертиза. Это знаю я, знает команда и знают те клиенты, которые из года в год продлевают лицензии на PVS-Studio.
Про это я кстати подробно буду рассказывать на конференциях по Java в 2019 году. Приходите на наши доклады!