Comments 33
При всём моём уважении к проекту и продукту, анализ кода проектов на C++ вы делаете куда корректнее… Чувствуется больший опыт, что ли.
Мы всегда работаем над собой, но в данном случае не очень понятно, что не так. :)
Разрешите поинтересоваться в целях повышения образованности — а что некорректно?
Issue 1.
Проверка на null даст разве что замену NullReferenceException на ArgumentNullException. Более того, этот код скорее всего будет размечен, так, чтобы аргумент был not nullable.
Вообще, .NET (и любая управляемая среда) более толерантна к исключениям и нулевым ссылкам в частности, чем native-среда. Более того, некоторые вещи в .NET построены на «специальных» исключениях. А в Python так вообще значения возвращают внутри исключения и это считается нормальной практикой.
В некоторых других примерах тоже передача null — не такая большая беда, как в статье описано.
Issue 5.
Профукали кривой рефакторинг. PVS Studio умеет понимать, что конструктор не может вернуть null? [ps: судя по следующим замечаниям — да] Остальное суть мелочи.
Issue 6. Больше похоже на ошибку копипасты. Хотелось бы посмотреть на объявление переменной numericValue.
И вообще в примерах кода не хватает объявлений, внешних по отношению к функциям. Складывается впечатление, что код толком не анализировался, а сразу пошло махание шашкой. Да, в данном случае идёт указание на ошибку, но анализ её не проведён. И вообще, switch не покрывает все варианты enum-а, что скорее всего тоже является ошибкой.
Issue 8. Нарушена рекомендация о недостаточности Assert-а: после него рекомендуется вставлять аналогичную проверку. Если мне память не изменяет, у Александреску такое было. Но опять-таки, ну проверили, дальше что? Поднять другое исключение?
Issue 13. Всё несколько сложнее. Сравнение на null в общем случае может вернуть true даже если ссылка не null. В данном конкретном случае аргумент имеет тип object, поэтому такая ситуация невозможна. Но стоит его заменить на любой аргумент, для которого будет определён оператор ==, как поведение может несколько измениться.
Кроме того, поскольку метод не полиморфный и объём его невелик (менее 16 байт), он с высокой вероятностью заинлайнится JIT-ом и не будет вызова со сбросом конвейера и кэша инструкций, в то время как подъём исключения гарантированно раздует метод и он точно не будет заинлайнен. Это по сути микрооптимизация, а не глупость. Опять же, она дозволена из-за толерантности к исключениям.
Issue 14. Не следует — это не запрещено. Например Nullable возвращает пустую строку для null-значения.
Если посмотреть на стектрейс исключения, то станет очевидно, что исключение не в дебрях возникло, а в вашем коде. Но имеет место быть не следование стандартной библиотекой своим же гайдлайнам.
И вообще, ToString() — это часто форматтер, который нужен для отладки, как в примере 17, например. Тут скорее хочется сказать, что использование ToString в .NET-е очень неконсистентно: то оно полезно (например StringBuilder.ToString), то чисто для отладки.
Issue 19. Классический кейс несогласованности документации с кодом :)
На деле выглядит как выброс второго исключения при несоблюдении инвариантов — больше похоже на перестраховку, чем на реальный кейс из жизни — лучше громко ругнуться, чем долго отлавливать ошибку. В Issue 20 тоже похоже на аналогичную ситуацию.
Что же касается возвращаемого значения, обратите внимание на сигнатуру метода. Там есть слово virtual. То есть предполагается, что могут быть реализации, которые вернут оба значения. Я склонен такую диагностику считать шумом. Какой там уровень у неё был?
Продолжение следует.
Проверка на null даст разве что замену NullReferenceException на ArgumentNullException. Более того, этот код скорее всего будет размечен, так, чтобы аргумент был not nullable.
Вообще, .NET (и любая управляемая среда) более толерантна к исключениям и нулевым ссылкам в частности, чем native-среда. Более того, некоторые вещи в .NET построены на «специальных» исключениях. А в Python так вообще значения возвращают внутри исключения и это считается нормальной практикой.
В некоторых других примерах тоже передача null — не такая большая беда, как в статье описано.
Issue 5.
Профукали кривой рефакторинг. PVS Studio умеет понимать, что конструктор не может вернуть null? [ps: судя по следующим замечаниям — да] Остальное суть мелочи.
using (FileStream f
= new FileStream(fileName, FileMode.Open,
FileAccess.Read, FileShare.Read))
{
Debug.Assert(f != null,
"File.OpenRead returned null instead of throwing an exception");
...
Issue 6. Больше похоже на ошибку копипасты. Хотелось бы посмотреть на объявление переменной numericValue.
И вообще в примерах кода не хватает объявлений, внешних по отношению к функциям. Складывается впечатление, что код толком не анализировался, а сразу пошло махание шашкой. Да, в данном случае идёт указание на ошибку, но анализ её не проведён. И вообще, switch не покрывает все варианты enum-а, что скорее всего тоже является ошибкой.
Issue 8. Нарушена рекомендация о недостаточности Assert-а: после него рекомендуется вставлять аналогичную проверку. Если мне память не изменяет, у Александреску такое было. Но опять-таки, ну проверили, дальше что? Поднять другое исключение?
Issue 13. Всё несколько сложнее. Сравнение на null в общем случае может вернуть true даже если ссылка не null. В данном конкретном случае аргумент имеет тип object, поэтому такая ситуация невозможна. Но стоит его заменить на любой аргумент, для которого будет определён оператор ==, как поведение может несколько измениться.
Кроме того, поскольку метод не полиморфный и объём его невелик (менее 16 байт), он с высокой вероятностью заинлайнится JIT-ом и не будет вызова со сбросом конвейера и кэша инструкций, в то время как подъём исключения гарантированно раздует метод и он точно не будет заинлайнен. Это по сути микрооптимизация, а не глупость. Опять же, она дозволена из-за толерантности к исключениям.
Issue 14. Не следует — это не запрещено. Например Nullable возвращает пустую строку для null-значения.
Если посмотреть на стектрейс исключения, то станет очевидно, что исключение не в дебрях возникло, а в вашем коде. Но имеет место быть не следование стандартной библиотекой своим же гайдлайнам.
И вообще, ToString() — это часто форматтер, который нужен для отладки, как в примере 17, например. Тут скорее хочется сказать, что использование ToString в .NET-е очень неконсистентно: то оно полезно (например StringBuilder.ToString), то чисто для отладки.
Issue 19. Классический кейс несогласованности документации с кодом :)
На деле выглядит как выброс второго исключения при несоблюдении инвариантов — больше похоже на перестраховку, чем на реальный кейс из жизни — лучше громко ругнуться, чем долго отлавливать ошибку. В Issue 20 тоже похоже на аналогичную ситуацию.
Что же касается возвращаемого значения, обратите внимание на сигнатуру метода. Там есть слово virtual. То есть предполагается, что могут быть реализации, которые вернут оба значения. Я склонен такую диагностику считать шумом. Какой там уровень у неё был?
Продолжение следует.
Проверка на null даст разве что замену NullReferenceException на ArgumentNullException. Более того, этот код скорее всего будет размечен, так, чтобы аргумент был not nullable.
Если context не может быть null, то в методе всё равно ошибка: лишний код.
У вас какое-то своеобразное понимание того что такое "ошибка в ПО", если вы в него записываете unreachable код.
unreachable код был кем-то написан с какой-то целью. И этот кто-то, скорее всего, не знал что он пишет unreachable код. Таким образом, имеется несоответствие между реальным поведением кода и мыслями автора о своем коде. Да, это именно что ошибка.
Сложно читать потому что ошибки не оценены на северити. Из-за этого постоянно думаешь «зачем я это читаю».
Но так вцелом надо это на гитхаб и пусть там микрософт триажит и сортирует по важности — их работа.
Пока вам неизвестно в чём цель, как вы можете определить достигает её написанный код или нет?
Допустим, цель — требовать чтобы в метод не передавали context==null. Реальное поведение программы в данном случае совпадает с желаемым вне зависимости от наличия в ней unreachable кода.
Как же оно совпадает, когда ожидалось InvalidOperationException, а вылетело NullPointerException?
> ожидалось InvalidOperationException
Вы продолжаете фантазировать за автора кода, остановитесь. Ему вполне может быть всё-равно какой именно случится эксепшен.
Вы продолжаете фантазировать за автора кода, остановитесь. Ему вполне может быть всё-равно какой именно случится эксепшен.
А ему не может быть всё равно, ведь возможные исключения перечислены в документации, и NullPointerException среди них нет.
Issue 21, 22
Логично: если в качестве аргумента нужен объект, а передаётся null, то логично, что будет выброшено исключение. Можно ArgumentNullException, можно NullReferenceException. Семантика у них будет одинаковая. Что касается OutOfMemoryException — это вполне официальное поведение при создании массива.
Issue 24.
Код выглядит как какое-то легаси, до которого не дошли руки на переписывание более выразительно с использованием более современного синтаксиса языка.
Issue 27.
Возможно, неверное приведение типа коллекции. Вероятно, во время рефакторинга. Раньше там, наверное, было seq as XmlQueryNodeSequence. Ошибка есть, да, но выводы получились поверхностные.
Issue 28.
Для совместимости или для унификации сигнатур с другими типами. Вызов может выполняться не только прямой, но и косвенный, например с помощью рефлексии — для операций сериализации это очень вероятный способ использования.
38 туда же.
Issue 29 перекликается с 14 и 17, о которых я писал выше.
В статьях стоит не просто вываливать список потенциальных багов, а как-то их группировать логически.
В данном случае имеет место поведение builder-а, отдающего строку — оно вполне логично исходя из семантики.
Issue 33.
Очевидно, что первый блок имеет побочный эффект в виде изменения объекта row. Такой код вполне может иметь смысл. Надесь, уровень критичности этой диагностики оценен как низкий.
Issue 34 может перекликаться с 20, если метод может быть полиморфным. Он не виртуальный, но может быть он прилетел из интерфейса — по тексту это невозможно сказать.
Issue 40.
Напрашивается проработка гипотезы об использовании неверного оператора: && вместо ||.
Issue 41. Опять напрашивается группировка.
Продолжение следует
Логично: если в качестве аргумента нужен объект, а передаётся null, то логично, что будет выброшено исключение. Можно ArgumentNullException, можно NullReferenceException. Семантика у них будет одинаковая. Что касается OutOfMemoryException — это вполне официальное поведение при создании массива.
Issue 24.
Код выглядит как какое-то легаси, до которого не дошли руки на переписывание более выразительно с использованием более современного синтаксиса языка.
Issue 27.
Возможно, неверное приведение типа коллекции. Вероятно, во время рефакторинга. Раньше там, наверное, было seq as XmlQueryNodeSequence. Ошибка есть, да, но выводы получились поверхностные.
Issue 28.
Для совместимости или для унификации сигнатур с другими типами. Вызов может выполняться не только прямой, но и косвенный, например с помощью рефлексии — для операций сериализации это очень вероятный способ использования.
38 туда же.
Issue 29 перекликается с 14 и 17, о которых я писал выше.
В статьях стоит не просто вываливать список потенциальных багов, а как-то их группировать логически.
В данном случае имеет место поведение builder-а, отдающего строку — оно вполне логично исходя из семантики.
Issue 33.
Очевидно, что первый блок имеет побочный эффект в виде изменения объекта row. Такой код вполне может иметь смысл. Надесь, уровень критичности этой диагностики оценен как низкий.
Issue 34 может перекликаться с 20, если метод может быть полиморфным. Он не виртуальный, но может быть он прилетел из интерфейса — по тексту это невозможно сказать.
Issue 40.
Напрашивается проработка гипотезы об использовании неверного оператора: && вместо ||.
Issue 41. Опять напрашивается группировка.
Продолжение следует
Граждане минусующие (есть гипотеза, что это один человек), а можно Вас попросить вступить таки в дискуссию? Если критика конструктивная — я тоже буду рад её услышать.
Алексей, добрый день.
Спасибо за развёрнутую критику. Наверное, из неё при желании можно составить контр-статью. :)
Если честно, не хочу вступать в дискуссии по обсуждению выписанных Issue — махать шашкой в комментариях и ударяться в дискуссии там же — не по мне. Не буду утверждать, что я абсолютно прав во всех своих утверждениях, так как этой мой взгляд на код со стороны, но если бы я не считал код ошибочным/интересным/подозрительным, я бы его не выписал в статью.
По поводу претензии про группировку предупреждений в статье — также спасибо за критику. Часто, когда пишу статьи, я группирую их по номерам предупреждений. В данном случае я решил сделать иначе, и выписывать предупреждения, показавшимися интересными, по проектам. Сделал для того, чтобы не утомлять читателя перечнями предупреждений одной категории, а разбавлять их.
Спасибо за развёрнутую критику. Наверное, из неё при желании можно составить контр-статью. :)
Если честно, не хочу вступать в дискуссии по обсуждению выписанных Issue — махать шашкой в комментариях и ударяться в дискуссии там же — не по мне. Не буду утверждать, что я абсолютно прав во всех своих утверждениях, так как этой мой взгляд на код со стороны, но если бы я не считал код ошибочным/интересным/подозрительным, я бы его не выписал в статью.
По поводу претензии про группировку предупреждений в статье — также спасибо за критику. Часто, когда пишу статьи, я группирую их по номерам предупреждений. В данном случае я решил сделать иначе, и выписывать предупреждения, показавшимися интересными, по проектам. Сделал для того, чтобы не утомлять читателя перечнями предупреждений одной категории, а разбавлять их.
Интересно, поучительно, с интригой. Каждый раз приятно читать, чудесная статья!
Issue 9
А не возникнет ли исключение IndexOutOfBoundsException в случае, когда files.Length == 0? Конкретно в инструкции: files[0]. Или в коде выше этого не допускается? Если я прав, то, возможно, стоит добавить в PVS-Studio соответствующую эвристику.
А не возникнет ли исключение IndexOutOfBoundsException в случае, когда files.Length == 0? Конкретно в инструкции: files[0]. Или в коде выше этого не допускается? Если я прав, то, возможно, стоит добавить в PVS-Studio соответствующую эвристику.
Я бы переписал как
В последнем случае можно функции getLastFileOrNone и isNotASpecialFile вынести из класса в библиотечный helper и протестировать отдельно.
private bool ContainsUnknownFiles(string directory)
{
....
return files.Length > 2 || isNotASpecialFile(getLastFile(files))
}
private File getLastFile(File files[])
{
return !files.Empty ? files[files.Length-1] : null;
}
private bool isNotASpecialFile(File file)
{
return file != null && !isIdFile(file) && !isInfoFile(file)
}
В последнем случае можно функции getLastFileOrNone и isNotASpecialFile вынести из класса в библиотечный helper и протестировать отдельно.
При всем великолепии содержания и подачи статьи, не могу удержаться от шутки, что большинство проблем выглядит следующим образом:
Гхмм… подозрительный порядок проверки. А что если передать сюда null?
@
NullReferenceException: 'Object reference not set to an instance of an object'
@
Surprised pikachu
Критику оставлю в ветке выше, а здесь хочу восхититься случаем 37 — это высший пилотаж! Снимаю шляпу!
Статья хорошая, но этот анализатор в полном объеме не поддерживает все стандарты MISRA C, а это делает его не актуальным для embedded. Лучше тогда использовать уже статический анализатор PC-lint.
Относительно null reference – вышла программа с сотрудниками Microsoft, участвующими в разработке Nullable Refecence Types здесь, в которой как раз рассказывается, как новые аннотации помогают в борьбе с null reference в CoreFX.
В продолжение темы проверки .NET Core и правок: PVS-Studio — двигатель прогресса.
Попроверяли, теперь можно и попользоваться :)
В наших планах выпустить PVS-Studio C# для .NET Core на платформах Linux и macOS в конце апреля — первой половине мая 2020. Beta-тест C# анализатора PVS-Studio на Linux и macOS.
В наших планах выпустить PVS-Studio C# для .NET Core на платформах Linux и macOS в конце апреля — первой половине мая 2020. Beta-тест C# анализатора PVS-Studio на Linux и macOS.
Планы реализованы: Как найти ошибки в C# проекте, работая под Linux и macOS.
Sign up to leave a comment.
Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio