Комментарии 34
А зачем в updatePriority примере нужен был if? Если его убрать, то станет проще читаться и будет проще поддерживать (не придётся дублировать литералы). Правильно оно ругалось на код, по делу.
command.startsWith("--data", ignoreCase) -- тоже по делу ругалось. Если хочется пояснить ignoreCase, то почему бы не использовать для этого именованные параметры? Зачем переменную объявлять? Вот так хорошо: command.startsWith("--data", ignoreCase = true)
А зачем в updatePriority примере нужен был if?
Проблема уменьшенных примеров во всей красе. Например, перед when могло быть общее действие, которое надо сделать для всех трёх вариантов. Давай что-нибудь добавлю.
command.startsWith("--data", ignoreCase) -- тоже по делу ругалось
Например, у тебя двадцать вызовов с этим ignoreCase. И ты хочешь подчеркнуть, что он во всех вызовах одинаковый. Во всяком случае, если ты считаешь, что заводить переменную здесь не надо (а не все с тобой согласятся), то это отдельная стилистическая инспекция "лишняя переменная", она должна ругаться именно на это с предложением заинлайнить и не должна быть привязана к типу переменной.
Например, перед when могло быть общее действие, которое надо сделать для всех трёх вариантов. Давай что-нибудь добавлю
По-моему, всё равно if лишний. Убрать бы его. Получается, кто-то там может слать недопустимые значения, а мы их игнорируем. Конечно, так и до enum недалеко додуматься, но вопросы остаются.
Если уже 25 вызовов с одним значением ignoreCase, то пора extract method делать.
Разумеется, я поддерживаю, что ругаться должна инспекция "вы тут зря переменную завели, воспользуйтесь лучше именованными параметрами", а не "expression is always true", просто исходно код выглядит стрёмно, и зачастую ожидаемо, что анализатор будет что-то там бурчать, особенно, когда код стрёмный.
Получается, кто-то там может слать недопустимые значения, а мы их игнорируем.
Ну ты зануда. Ну представь, что там исключение кидается. Лучше будет?
просто исходно код выглядит стрёмно
Бывает и похуже, чего уж там. Если ругаться на весь код выглядящий стрёмно, никто не будет пользоваться статическим анализом. В данном случае сила инспекции в том, что она находит реальные баги, очень крутые и вкусные. Будет обидно, если они потонут в куче левых предупреждений.
Ну ты зануда
Ты меня раскусил
Ну представь, что там исключение кидается. Лучше будет?
Исключение прекрасно кинется в when else ветке. Ну, тут я полностью соглашусь, что корень проблемы не в "expression is always true", а в том, что тут возникло дублирование логики, и, вполне возможно, это дублирование следует убрать, чтобы перечни статусов не разошлись (ну, вдруг добавят ветку в when, а if забудут поправить?). Но, если бы у меня в том when "low" возникло предупреждение "condition is always true", то я бы порадовался и сказал: "а, действительно, как так получилось, что always true" и пошёл бы и убрал if или что там.
А есть режимчик, чтоб все эти левые таки отображались?
Пример про isOption, конечно, нужно вообще без return записывать. Там ни return ни let не нужно: `isOption(s: String?) : Boolean = s?.startsWith("--") ?: false`. Ну или на String.isOption переделать (но это уже от проекта зависит)
Твой встроенный оптимизатор отлично работает! А теперь придумай мне короткий пример, чтобы let был оправдан :-)
Возможно, что 99.42% всех таких примеров сведутся к тому, что незачем там return внутри let использовать, и оно должно ругаться словами "убирайте return" или "убирайте let"
Ну ещё раз говорю - это должна быть отдельная паттерновая инспекция, а не датафлоу.
Так-то даже случаи когда конфликт со смарткастом обычно можно переписать таким образом, что код станет проще и понятнее и конфликт исчезнет. Но это часто думать надо и автоматический фикс мы точно не сможем предложить. А человек не будет думать сам, он инспекцию отключит, если не может быстро исправить варнинг. Потому и говорю, что инспекцию надо глупее делать.
val empty = str?.isEmpty() ?: true if (!empty && str != null) { // 'str != null' is always true
Что только люди не напишут, лишь бы не делать по-простому (штатный механизм в stdlib): if (str.isNullOrEmpty()) { ... }
Так и знал, что ты к этому придерёшься. Даже написал, что пример искусственный, но не помогло :-) isNullOrEmpty анализатор не распознаёт (по крайней мере пока).
Тема статьи какая? "Вот нормальный код, анализатор пытался его обругать, но не надо было". А тут смотришь -- и между строк написано "тут должно было быть isNullOrEmpty".
Я другое спрошу (213.4631.20, 6 October). Почему на if (false) ругается, что "condition is always false", а на val x = false; if (x) уже нет? Бага получается? Я бы на if (false) и if (true) не ругался.
Хотя посмотрел на Java -- там if (false) ругается (почему бы?). Наверное, если уж делать, то однообразно.
Не понял. Ругается и в джаве, и в Котлине? В чём тогда проблема?
Думаю, ни там ни там не надо. Разумеется, "правильного ответа" тут нет, и как партия решит, так пусть и будет. Но я всё равно думаю, что подкрашивать условие if(false) это странно. Вполне нормально взять и написать while(false), if(false) и т.п. для отладки. Если оно более блеклым цветом отметит "никогда не выполняющиеся ветки кода", то это другой разговор. А подкрашивать "if(false) is always false" -- ну, зачем?
В Java language specification отдельно описано, что if (false) это годный код, и компилятор не должен ругаться на его недостижимость: https://docs.oracle.com/javase/specs/jls/se7/html/jls-14.html#jls-14.21
В статье речь про Котлин, странно приводить JLS в качестве аргумента.
В моём понимании «годный код» это такой, к которому нельзя предъявить обоснованных претензий. Отсутствие ошибок компиляции это необходимое, но не достаточное условие.
На мой взгляд static final boolean FLAG = ... if (FLAG) ... это нормально, а вот if (false) выглядит грязно. Лучше уж закомментировать.
Ну это когда было! Тогда и гита не было. Я иногда временно отключаю ветки кода в процессе отладки, но никогда такое не коммичу. И мне нравится, что редактор подсвечивает, предупреждая, что тут вот временная штука, которую надо будет вернуть на место.
Кажется, это другая инспекция, которая конкретно разворачивает if(true)/if(false) и больше ничего
Вообще это тонкий момент. Люди любят параметризовать свой код чем-нибудь типа val debug = true и потом бранчеваться по этому условию. И ты опять же скажешь, что это плохо. Но нам надо поддерживать все популярные стили написания кода, а не только твой. Поэтому выдавать тут кучу предупреждений не представляется возможным.
Вообще чем критиковать искусственные примеры, лучше бы проанализировал какой-нибудь реальный код и рассказал, что нашлось!
Может я чего-то не понимаю, но зачем вообще нужны предупреждения о константных выражениях? Вычисления константных выражений одна из самых очевидных и простых оптимизаций доступных компиляторам уже с незапамятных времён. И программисты активно используют константные выражения, чтобы писать более понятный код.
Например, const int kDefaultCacheSize = 5 * 1024 * 1024, чтобы выразить в читаемом виде 5 мегабайт. Или, например, 2 * kMillisecondsInMinute.
Опыт подсказывает, что такие предупреждения могут выявить (в дополнение к тривиальным) исключительно нетривиальные баги в программе. Для хорошего примера могу сослаться на наших коллег из PVS-Studio: 31 февраля. Или вот моя статья Статический анализ → уязвимость → профит. Хорошо написанный анализатор не выдаёт предупреждения на константах, которые введены для понятности кода, а выдаёт только на подозрительных константах.
if (x <= 0) { // ... return } assert(x > 0) // 'x > 0' is always true
Простите, а разве тут не case-выражение (или что там применяется для exhaustive перечислений в Kotlin) нужно использовать?
Да, иногда статический анализатор борщит
Сделать статический анализ умным — полдела, потом его надо делать глупым