У нас появилась экспериментальная версия анализатора PVS-Studio, который умеет анализировать C#-проекты и который можно показать миру. Это не Release, и даже не Beta. Это просто текущая сборка PVS-Studio. Мы хотим как можно раньше начать получать отзывы от наших пользователей или потенциальных пользователей касательно поддержки C#. Поэтому предлагаем энтузиастам попробовать новую версию PVS-Studio на своих C#-проектах и рассказать нам о результатах, недостатках и высказать свои пожелания. Ах да, и конечно в статье будут описаны результаты проверки очередного проекта — SharpDevelop.
PVS-Studio
Сейчас одним из важных вопросов является: «Зачем делать очередной инструмент анализа кода для C#?».
Попробуем ответить как потенциальным пользователям, так и самим себе, чтобы ясно понимать куда и зачем мы движемся.
Мы успешно создали и продолжаем развивать анализатор PVS-Studio для языка C/C++. В этом анализаторе реализовано много интересных и уникальных идей по выявлению разнообразных типов ошибок. Со временем стало понятно, что многие из реализованных диагностик никак не связаны с конкретным языком программирования. Не важно, какой язык вы используете. Всегда будут опечатки, ошибки из-за невнимательности или неудачного Copy-Paste.
И тогда мы решили попробовать применить свой опыт к другому языку программирования, к C#. Насколько это будет успешным начинанием, покажет время. Сами мы считаем, что постепенно сможем создать очень интересный инструмент, пользу от которого может получить большое количество C#-разработчиков.
Сейчас наша задача — начать как можно раньше получать отзывы от наших потенциальных пользователей. Полноценная версия анализатора PVS-Studio ещё не готова. Сейчас в ней мало диагностик (на момент написания статьи их было 36). Но эту версию можно установить и попробовать. И мы будем благодарны всем, кто это сделает. Нам важно убедиться, что мы вообще движемся в верном направлении и что анализатор в целом работоспособен. А у уж добавлять новые и новые диагностики мы будем очень быстро.
Итак, всем заинтересованным предлагаю скачать текущий вариант экспериментальной версии PVS-Studio по этой ссылке: http://files.viva64.com/beta/PVS-Studio_setup.exe.
Примечание. Со временем приведенная выше ссылка станет недействительной. Поэтому, если вы читаете эту статью по пришествию месяца или более с момента публикации, то предлагаем вам установить текущую версию дистрибутива: http://www.viva64.com/ru/pvs-studio-download/
Если читатель ещё не разу не пробовал PVS-Studio, то предлагаю ознакомиться со статьёй "PVS-Studio для Visual C++". Как вы видите, она ориентирована на C++, но на самом деле никакой разницы нет. С точки зрения интерфейса вообще почти нет никакой разницы, работаете вы с C++ или с C# проектами.
Чтобы отправить свои отзывы и пожелания, вы можете воспользоваться страницей обратной связи.
Проверка проекта SharpDevelop
Для программистов обыкновенная реклама не работает. Однако я знаю, как привлекать внимание этих серьезных и очень занятых творцов. Мы проверяем различные открытые проекты и пишем про это статьи. Нет лучшей рекламы, чем показать, что умеет наш инструмент.
Не вижу смысла изобретать велосипед. Теперь тем же методом я буду очаровывать C# программистов. И вот перед вами очередная статья про проверку открытого проекта SharpDevelop.
SharpDevelop — свободная среда разработки для C#, Visual Basic .NET, Boo, IronPython, IronRuby, F#, C++. Обычно используется как альтернатива Visual Studio .NET. Существует также форк на Mono/GTK+ — MonoDevelop.
Для нас важно, что проект написан полностью на C#. А значит мы можем его проверить экспериментальной версией PVS-Studio. В проекте 8522 файлов с расширением «cs», суммарный размер которых равен 45 мегабайт.
Наиболее подозрительные фрагменты кода
Фрагмент N1
public override string ToString()
{
return String.Format("Thread Name = {1} Suspended = {2}",
ID, Name, Suspended);
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. Thread.cs 235
Переменная ID никак не используется. Возможно никакой настоящей ошибки здесь нет. Однако это место явно стоит проверить. Возможно здесь планировалось сформировать совсем иную строку.
Фрагмент N2
public override string ToString ()
{
return
String.Format ("[Line {0}:{1,2}-{3,4}:{5}]",
File, Row, Column, EndRow, EndColumn, Offset);
}Предупреждение PVS-Studio: V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 4. Present: 6. MonoSymbolTable.cs 235
Более интересный случай. Что именно хотел сделать программист, мне непонятно. Возможно, он хотел сформировать сообщение вот такого вида:
[Line file.cs:10,20-30,40:7]
Но видимо он пропустил некоторые фигурные скобки. Поэтому получается, что ",2" и ",4" задаёт выравнивание полей, а вовсе не выводят значения переменных EndRow и EndColumn.
Рискну предположить, что правильной будет следующая строка форматирования:
String.Format ("[Line {0}:{1},{2}-{3},{4}:{5}]",
File, Row, Column, EndRow, EndColumn, Offset);Фрагмент N3
static MemberCore GetLaterDefinedMember(MemberSpec a, MemberSpec b)
{
var mc_a = a.MemberDefinition as MemberCore;
var mc_b = b.MemberDefinition as MemberCore;
....
if (mc_a.Location.File != mc_a.Location.File)
return mc_b;
return mc_b.Location.Row > mc_a.Location.Row ? mc_b : mc_a;
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'mc_a.Location.File' to the left and to the right of the '!=' operator. membercache.cs 1306
Здесь мы имеем дело с опечаткой. Я думаю, правильным вариантом будет следующее сравнение:
if (mc_a.Location.File != mc_b.Location.File)Фрагмент N4
public WhitespaceNode(string whiteSpaceText,
TextLocation startLocation)
{
this.WhiteSpaceText = WhiteSpaceText;
this.startLocation = startLocation;
}Предупреждение PVS-Studio: V3005 The 'this.WhiteSpaceText' variable is assigned to itself. WhitespaceNode.cs 65
Красивая ошибка. Здесь статический анализатор показал свою главную суть. Он внимателен и в отличии от человека не устаёт. Поэтому он заметил опечатку. А вы её видите? Согласитесь, найти ошибку не просто.
Итак, опечатка в одной букве. Надо было написать "=whiteSpaceText". А написано "=WhiteSpaceText". В результате значение 'WhiteSpaceText' в классе остаётся неизменным.
Вообще, это хороший пример того, как не надо именовать переменные. Плохая идея делать различие в именах только одной строчной/прописной буквой. Однако рассуждения о стиле кодирования выходит за тему статьи. Тем более это попахивает священной дискуссионной войной.
Фрагмент N5
new public bool Enabled {
get { return base.Enabled; }
set {
if (this.InvokeRequired) {
base.Enabled = this.VScrollBar.Enabled =
this.hexView.Enabled =this.textView.Enabled =
this.side.Enabled = this.header.Enabled = value;
} else {
base.Enabled = this.VScrollBar.Enabled =
this.hexView.Enabled = this.textView.Enabled =
this.side.Enabled = this.header.Enabled = value;
}
}
}Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. Editor.cs 225
Очень подозрительно, что независимо от значения 'this.InvokeRequired' будут выполнены одни и те-же действия. У меня сильные подозрения, что строка «base.Enabled = .....» была скопирована. А потом в ней что-то забыли изменить.
Фрагмент N6, N7, N8, N9
public override void Run()
{
....
ISolutionFolderNode solutionFolderNode =
node as ISolutionFolderNode;
if (node != null)
{
ISolutionFolder newSolutionFolder =
solutionFolderNode.Folder.CreateFolder(....);
solutionFolderNode.Solution.Save();
....
}Предупреждение PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'solutionFolderNode'. SolutionNodeCommands.cs 127
Хотели выполнить некоторые действия, если 'node' наследуется от интерфейса 'ISolutionFolderNode'. Но проверили не ту переменную. Правильный вариант:
ISolutionFolderNode solutionFolderNode =
node as ISolutionFolderNode;
if (solutionFolderNode != null)
{Кстати, это достаточно распространённый паттерн ошибки в C#-программах. Например, в проекте SharpDevelop встретилось ещё 3 таких ошибки:
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'geometry', 'g'. PathHandlerExtension.cs 578
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'oldTransform', 'tg'. ModelTools.cs 420
- V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'solutionFolderNode'. SolutionNodeCommands.cs 104
Фрагмент N10
public override void VisitInvocationExpression(....)
{
....
foundInvocations = (idExpression.Identifier == _varName);
foundInvocations = true;
....
}Предупреждение PVS-Studio: V3008 The 'foundInvocations' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 211, 209. RedundantAssignmentIssue.cs 211
Очень подозрительное повторное присваивание. Возможно, второе присваивание написали в процессе отладки кода, а потом забыли про него.
Ошибка N11
public static Snippet CreateAvalonEditSnippet(....)
{
....
int pos = 0;
foreach (Match m in pattern.Matches(snippetText)) {
if (pos < m.Index) {
snippet.Elements.Add(....);
pos = m.Index;
}
snippet.Elements.Add(....);
pos = m.Index + m.Length;
}
....
}Предупреждение PVS-Studio: V3008 The 'pos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 151, 148. CodeSnippet.cs 151
Ещё одно подозрительное повторное присваивание. Здесь или ошибка, или присваивание «pos = m.Index;» является лишним.
Фрагмент N12
....
public string Text { get; set; }
....
protected override void OnKeyUp(KeyEventArgs e)
{
....
editor.Text.Insert(editor.CaretIndex, Environment.NewLine);
....
}Предупреждение PVS-Studio: V3010 The return value of function 'Insert' is required to be utilized. InPlaceEditor.cs 166
Строки в языке C# являются неизменяемыми. Поэтому, если мы что-то делаем со строкой, результат нужно где-то сохранить. Однако про это легко забыть, как например это случилось здесь. Разработчик решил, что вызывая метод Insert() он что-то добавит к строке. Но это не так. Правильный вариант кода:
editor.Text =
editor.Text.Insert(editor.CaretIndex, Environment.NewLine);Фрагмент N13, N14
public IEnumerable<PropertyMapping>
GetMappingForTable(SSDL.EntityType.EntityType table)
{
var value = GetSpecificMappingForTable(table);
var baseMapping = BaseMapping;
if (baseMapping != null)
value.Union(baseMapping.GetMappingForTable(table));
return value;
}Предупреждение PVS-Studio: V3010 The return value of function 'Union' is required to be utilized. MappingBase.cs 274
Вообще, у меня складывается предчувствие, что в C# проектах я буду встречать достаточно много ошибок, связанных с тем, что программист ожидает изменения объекта, а этого не происходит.
Метод-расширение 'Union', определённый для коллекций, реализующих интерфейс IEnumerable, позволяет получить пересечение двух множество. Однако, контейнер 'value' при этом не изменяется. Правильный вариант:
value = value.Union(baseMapping.GetMappingForTable(table));Ещё одну такую ситуацию можно встретить здесь: V3010 The return value of function 'OrderBy' is required to be utilized. CodeCoverageMethodElement.cs 124
Фрагмент N15
Анализатор PVS-Studio пытается выявить ситуации, где программист мог что-то забыть сделать в switch(). Логика принятия решения, предупреждать или нет, достаточно сложная. Иногда получаются ложные срабатывания, иногда находятся явные ошибки. Рассмотрим одно из таких срабатываний.
Итак, в коде имеет место вот такое перечисление:
public enum TargetArchitecture {
I386,
AMD64,
IA64,
ARMv7,
}Местами используются все варианты этого перечисления:
TargetArchitecture ReadArchitecture ()
{
var machine = ReadUInt16 ();
switch (machine) {
case 0x014c:
return TargetArchitecture.I386;
case 0x8664:
return TargetArchitecture.AMD64;
case 0x0200:
return TargetArchitecture.IA64;
case 0x01c4:
return TargetArchitecture.ARMv7;
}
throw new NotSupportedException ();
}Однако есть и подозрительные места. Например, анализатор обратил моё внимание на следующий код:
ushort GetMachine ()
{
switch (module.Architecture) {
case TargetArchitecture.I386:
return 0x014c;
case TargetArchitecture.AMD64:
return 0x8664;
case TargetArchitecture.IA64:
return 0x0200;
}
throw new NotSupportedException ();
}Предупреждение PVS-Studio: V3002 The switch statement does not cover all values of the 'TargetArchitecture' enum: ARMv7. ImageWriter.cs 209
Как видите, не рассмотрен случай, если архитектурой является ARMv7. Я не знаю, ошибка это или нет. Но мне кажется, что это именно ошибка. Имя ARMv7 находится в конце перечисления, а значит добавлялось в последнюю очередь. В результате программист мог забыть поправить функцию GetMachine() и учесть эту архитектуру.
Фрагмент N15
void DetermineCurrentKind()
{
.....
else if (Brush is LinearGradientBrush) {
linearGradientBrush = Brush as LinearGradientBrush;
radialGradientBrush.GradientStops =
linearGradientBrush.GradientStops;
CurrentKind = BrushEditorKind.Linear;
}
else if (Brush is RadialGradientBrush) {
radialGradientBrush = Brush as RadialGradientBrush;
linearGradientBrush.GradientStops =
linearGradientBrush.GradientStops;
CurrentKind = BrushEditorKind.Radial;
}
}Предупреждение PVS-Studio: V3005 The 'linearGradientBrush.GradientStops' variable is assigned to itself. BrushEditor.cs 120
Достаточно тяжелый фрагмент кода для чтения. И видимо поэтому в нем допущена ошибка. Скорее всего код писался методом Copy-Paste и в одно месте был неправильно изменён.
По все видимости вместо:
linearGradientBrush.GradientStops =
linearGradientBrush.GradientStops;Должно было быть написано:
linearGradientBrush.GradientStops =
radialGradientBrush.GradientStops;Запахи
Многие фрагменты, на которые указывает анализатор, вряд ли являются настоящими ошибками. С другой стороны, сообщения, выданные на такой код, назвать ложными срабатываниями тоже нельзя. Обычно про такой код говорят, что он пахнет.
Выше я рассмотрел много кода, который по всей видимости содержит ошибки. Теперь приведу несколько примеров запахов. Все ситуации я рассматривать не буду, это не интересно. Ограничусь 3 примерами. С остальными запахами разработчики могут ознакомиться самостоятельно, проверив проект SharpDevelop.
Фрагмент кода с запахом N1
protected override bool CanExecuteCommand(ICommand command)
{
....
}
else if (command == DockableContentCommands.ShowAsDocument)
{
if (State == DockableContentState.Document)
{
return false;
}
}
....
else if (command == DockableContentCommands.ShowAsDocument)
{
if (State == DockableContentState.Document)
{
return false;
}
}
....
}Предупреждение PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 773, 798. DockableContent.cs 773
Как видите, программа содержит два идентичных блока. Условие нижнего блока 'if' никогда не выполнится. Но на мой взгляд это не ошибка. Мне кажется, что просто случайно продублировали блок, и он является лишним. Тем не менее это место, которое стоит посмотреть и исправить.
Фрагмент кода с запахом N2
void PropertyExpandButton_Click(object sender, RoutedEventArgs e)
{
....
ContentPropertyNode clickedNode =
clickedButton.DataContext as ContentPropertyNode;
clickedNode = clickedButton.DataContext as ContentPropertyNode;
if (clickedNode == null)
....
}Предупреждение PVS-Studio: V3008 The 'clickedNode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 105, 104. PositionedGraphNodeControl.xaml.cs 105
Код избыточный и его можно упростить до:
ContentPropertyNode clickedNode =
clickedButton.DataContext as ContentPropertyNode;
if (clickedNode == null)Фрагмент кода с запахом N3
IEnumerable<ICompletionData>
CreateConstructorCompletionData(IType hintType)
{
....
if (!(hintType.Kind == TypeKind.Interface &&
hintType.Kind != TypeKind.Array)) {
....
}Предупреждение PVS-Studio: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. CSharpCompletionEngine.cs 2392
Избыточный код. Выражение можно упростить:
if (hintType.Kind != TypeKind.Interface) {Я могу продолжать, но, пожалуй, хватит. Все остальные «запахи» достаточно однообразны и похожи на уже перечисленные.
Заключение
Ну что, как видите, сам по себе язык C# не защищает от бестолковых ошибок. Потому я с чистой совестью могу привести здесь эту картинку.
Да здравствует единорог, который теперь научился находить ошибки и в C# программах!
А если серьезно, то:
- При программировании мы все допускаем не только сложные, но и простые ошибки. Суммарно на поиск простых ошибок тратится много времени. А иногда очень много.
- Большое количество простых ошибок можно выявить ещё на этапе написания кода, если использовать инструменты статического анализ кода. Использование таких инструментов существенно экономит время, которое может быть потрачено на поиск и отладку многих ошибок.
- Самое главное в статическом анализе — регулярное применение. Нет смысла в разовых проверках статического анализа. Вся его суть в том, чтобы найти ошибку сразу после того, как она появилась в коде. Редкие проверки отнимают много времени и приносят мало пользы. Ведь те ошибки, которые можно было найти быстро и легко, уже к этому моменту поправлены потом и кровью.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Experimental version of PVS-Studio with C# support.
Комментарии (113)
leremin
30.11.2015 17:38+150000 строк. Всего два ворнинга на тему «It is odd that the body of 'Foo1' function is fully equivalent to the body of 'Foo2' function (344, line 356)». Так не интересно.
EvgeniyRyzhkov
30.11.2015 17:47А сообщения по делу?
leremin
30.11.2015 17:52По делу, конечно. Но про эти одинаковые функции я знал. А вот в c++ на ~40000 строк PVS-Studio нашел у меня полтора десятка ошибок, среди них были очень серьезные мои косяки.
Andrey2008
30.11.2015 17:58+7Так в C++ анализаторе на порядок больше диагностик. А C# анализатор пока ещё маленький. Но не волнуйтесь, мы его усиленно кормим.
Oxoron
30.11.2015 18:11-1Насколько я понял из статьи, вы пока мигрируете самые простые проверки. Вы дублируете проверки R#, или выделяете уникальные проверки? Можно ли будет разделить проверки на «дублирующие» и «оригинальные», и отключить все проверки одной группы?
Andrey2008
30.11.2015 19:27+1Мы не дублируем проверки R#. А если и дублируем, но не специально, а просто как следствие развитие анализатора. Делить мы ничего не будем, так как вообще не понятно зачем это нужно. Ну дублируется какая-то проверка, зачем её отключать? Более того, уверен многие проверки будут более мощные чем у R#, хотя на первый взгляд они будут выглядеть одинаково.
Oxoron
30.11.2015 20:00-5Делить мы ничего не будем, так как вообще не понятно зачем это нужно.
Скорость создания. Продукт с уникальными проверками создается быстрее, чем с уникальными и дублирующимися. Плюс, сразу исчезают сомнения вроде «а нафиг мне PVS, если 90% его функционала так или иначе покрывается R#?»
Сейчас нет возможности проверить (чуть выше я уже ошибся), но почти все проверки в статье (сомневаюсь насчет enum-ов) так или иначе покрываются R# (как минимум, R# привлекает внимание к проблемным местам).
Paull
01.12.2015 11:54В статье имелось в виду, что мы мигрируем для C# самые простые General Analysis проверки C++ версии PVS-Studio, у нас нет в планах дублировать проверки каких-либо других анализаторов. Безусловно, неизбежно пересечение наших диагностик и диагностик других анализаторов, но это не означает, что они идентичны.
a553
30.11.2015 18:13
А ваш анализатор для всех свойств такую проверку делает, или только для специально отобранных?if (!(hintType.Kind == TypeKind.Interface && hintType.Kind != TypeKind.Array)) {Andrey2008
30.11.2015 19:56+1Не для всех. Есть специальная логика, определяющая, стоит проверять или нет.
mayorovp
30.11.2015 18:31+2Рискну предположить, что правильной будет следующая строка форматирования:
String.Format ("[Line {0}:{{1},{2}}-{{3},{4}}:{5}]", File, Row, Column, EndRow, EndColumn, Offset);
Не будет. Потому что {{ — это экранированный знак {. Чтобы вывести первый параметр после символа { — надо написать {{{1}. С закрывающей скобкой еще хуже: Если написать {1}}} — то символ } окажется внутри пары скобок, а не снаружи, что приведет к ошибке форматирования. Тут приходится выносить этот символ в параметры, правильный вызов выглядит как-то так:
String.Format ("[Line {0}:{{{1},{2}{6}-{{{3},{4}{6}:{5}]", File, Row, Column, EndRow, EndColumn, Offset, '}');Andrey2008
30.11.2015 19:58+2Спасибо. Поправлю статью.
mayorovp
30.11.2015 20:13-3Что-то вы как-то статью наполовину поправили…
mayorovp
01.12.2015 08:25+1upd. Добрался до компьютера со студией и проверил. Я был не прав, закрывающая фигурная скобка прекрасно экранируется. Теперь уже и не вспомню, что за баг я с ней ловил…
Впрочем, еще одно исправление статьи, где «лишние» фигурные скобки вообще исчезли, похоже на правду.
wrewolf
30.11.2015 18:36+1№5 там асинхронный кусок, то что в условие if(){ //Вот тут должно перебрасывать в поток UI }
InvokeRequired
samodum
30.11.2015 19:36Скажите, проверка кода происходит у вас на сервере?
Andrey2008
30.11.2015 20:19Нет. Это расширение (plug-in) для Visual Studio. Проверка происходит на той машине, где запущена VS. Исходники никуда не отправляются.
Можно запускать на сервере (например, ночные запуски). Но это другое.samodum
30.11.2015 22:04+1Просто при отключенном интернете у меня ничего не сработало. Когда включил, то код был проверен. Вот я и засомневался
druss
30.11.2015 19:38К сожалению не получилось запустить даже на пустом консольном проекте.
При «Check -> Solution» получаю: «The tools version „14.0“ is unrecognized. Available tools versions are „12.0“, „2.0“, „3.5“, „4.0“. c:\users\johndoe\documents\visual studio 2013\Projects\ConsoleApplication2\ConsoleApplication2\ConsoleApplication2.csproj 0»
Visual Studio Ultimate 2013
Пробовал target framework .net 4.5 и 3.5 — результат одинаковmayorovp
30.11.2015 20:08Target Framework тут ни при чем, Tools Version — это версия MSBuild.
Вообще такая ситуация выглядит странно, обычно с 2013й студией идет 12я версия MSBuild, а не 14я. Можете попробовать поправить в файле проекта этот номер любым текстовым редактором (только не на 12.0, а на 4.0 — версии 12.0 у вас может и не быть, а 4.0 ставится с фреймворком) — если повезет, то все заработает. А если не повезет — вернуть обратно всегда можно.
Paull
01.12.2015 11:49Пришлите пожалуйста тестовый проект, на котором повторяется ошибка на support@viva64.com, мы посмотрим, что можно сделать.
Athari
30.11.2015 19:40+1Хотели выполнить некоторые действия, если 'node' наследуется от интерфейса 'ISolutionFolderNode'. Но проверили не ту переменную.
У некоторых дотнетчиков есть дурная привычка использовать безопасное приведение типа вместо обычного, будто падать с NullReferenceException лучше, чем InvalidCastException. Этот код может подразумевать, что интерфейс ISolutionFolderNode реулизуется node в любом случае, но node может быть null. Хотя, конечно, ваша версия выглядит более вероятной, но здесь прослеживается разница между плюсами и шарпом: в дотнете меньше боятся null, это не сразу убивает приложение. Может оказаться, что исправлением будет изменение способа приведения типа.
ulltor
30.11.2015 19:40Visual Studio 2008, судя по всему, не поддерживаете? Хотел проверить рабочий проект, у нас достаточно интересный кейс: софт под WinCE, пишется в VS2008.
Andrey2008
30.11.2015 20:08Да, VS2005 и VS2008 более не поддерживается. Для клиентов (С++), использующих старые Visual Studio будет некоторое время существовать параллельная версия 5.xx.
ulltor
30.11.2015 20:12+1Ваша логика понятна, но, тем не менее, жаль. VS2008 — последняя студия с поддержкой разработки под WinCE.
leotsarev
30.11.2015 23:40Пример:
public Task<bool> GetTwoFactorEnabledAsync(User user) { return Task.FromResult(false); } public Task<bool> GetLockoutEnabledAsync(User user) { return Task.FromResult(false); }
Показывается варнинг V3013, не должен, функции слишком элементарные
m08pvv
01.12.2015 09:59+3Из замеченного сразу:
Если в асинхронном коде есть периодические проверки на IsCancellationRequested с соответствующим return, то PVS-Studio выдаёт предупреждение, что все остальные проверки IsCancellationRequested бессмысленны, но это не так.
Пару странных опечаток с лёгкостью было найдено: повтор логического условия, которое зачем-то продублировали, а также присвоение значения самому себе, повторное присвоение значения…
Unconditional 'return' within a loop можно было бы сделать и high severity — уж очень подозрительная вещь
Было обнаружено две функции с одинаковым телом, но разной сигнатурой: у одной есть аргумент, но он не использовался — можно было такое показывать с severity повыше, чем lowleotsarev
01.12.2015 14:53Если в асинхронном коде есть периодические проверки на IsCancellationRequested с соответствующим return, то PVS-Studio выдаёт предупреждение, что все остальные проверки IsCancellationRequested бессмысленны, но это не так.
Тоже самое, если мы проверяем volatile bool — можно бы и догадаться, что переменная volatile может поменяться.m08pvv
01.12.2015 16:04Тогда ещё и под lock'ом переменные, все явные чтения Volatile.Read и всё, что с MemoryBarrier, но там можно много всего интересного найти из способов выстрелить в ногу лёгким движением левой пятки компилятора, ибо модель памяти такая.
lair
01.12.2015 16:36Если в асинхронном коде есть периодические проверки на IsCancellationRequested с соответствующим return, то PVS-Studio выдаёт предупреждение, что все остальные проверки IsCancellationRequested бессмысленны, но это не так.
Да это и без асинхронии «не так»: последовательные обращения к любому свойству/методу могут давать разные значения (по крайней мере, до тех пор, пока в CLR не появится явного указания на детерминированность метода).m08pvv
01.12.2015 17:07+1Не совсем так. Спецификация говорит, что для кода без volatile, синхронизаций и прочего, вполне законны любые перестановки, которые законны для однопоточного кода. Справедливости ради, стоит заметить, что проверка IsCancellationRequested в итоге сводится к проверке значения поля, которое объявлено так:
private volatile int m_statelair
01.12.2015 17:14Спецификация говорит, что для кода без volatile, синхронизаций и прочего, вполне законны любые перестановки, которые законны для однопоточного кода.
А перестановка вызовов методов законна для однопоточного кода?m08pvv
01.12.2015 17:46Понятно, что с точки зрения компилятора геттер свойства — вызов метода, про чистоту которого ничего неизвестно, но я пытаюсь сказать, что геттер вполне законно может быть заинлайнен, а значения используемых в нём переменных сохранены в регистры и не перечитываться, если они не помечены volatile или не синхронизированы иным образом. Соответственно, в случае с IsCancellationRequested имеем volatile поле, обращение к которому идёт вместе с соответствующей инструкцией процессора, которая не даёт ему ни переместить чтение, ни использовать старое значение.
lair
01.12.2015 17:58+1я пытаюсь сказать, что геттер вполне законно может быть заинлайнен, а значения используемых в нём переменных сохранены в регистры и не перечитываться, если они не помечены volatile или не синхронизированы иным образом
Эмм. Однопоточный код:
class A { int _c = 0; public int C {get {return _c;}} public void Q () {/*меняет значение _с, не инлайнится*/} } void Foo() { var a = new A(); if (a.C != 0) return; a.Q(); if (a.C != 0) return; }
Вы правда хотите сказать, что ничего не мешает сначала заинлайнить геттерC, а потом прочитать значениеa._cединожды?m08pvv
01.12.2015 22:12+1Вы не так поняли. Перестановки и преобразования разрешены только те, которые не меняют результат в случае однопоточного кода. В данном примере два чтения не могут быть объединены в одно ни на стадии компиляции, ни на стадии jit, ни во время исполнения, так как присутствует запись. Если бы там не было записи в _с, то обе операции чтения могли быть вполне законно объединены в одну.
lair
01.12.2015 22:16В данном примере два чтения не могут быть объединены в одно ни на стадии компиляции, ни на стадии jit, ни во время исполнения, так как присутствует запись.
… и это не зависит отvolatile?
(мне, конечно, отдельно интересно, как определяется, присутствует ли запись)m08pvv
01.12.2015 22:25В примере с методом компилятор не будет ничего перемещать относительно вызова метода просто потому, что для этого надо слишком много всего проверять и тогда компиляция длилась бы целую вечность. jit-компилятор вполне может заинлайнить вызов метода, но переставить операции чтения и записи одной и той же переменной он не вправе их переставлять просто потому, что это изменит поведения даже для однопоточного случая. Процессор во время исполнения в пределах одного потока тоже не будет ничего переставлять (но при этом может выполнять много чего одновременно, но на результате это не должно отражаться). В случае же если бы там не было записи, то jit легко мог бы объединить два чтения в одно, если поле не volatile.
lair
01.12.2015 22:58В примере с методом компилятор не будет ничего перемещать относительно вызова метода просто потому, что для этого надо слишком много всего проверять и тогда компиляция длилась бы целую вечность.
Вот именно поэтому в общем случае два вызова даже одного и того же метода оптимизации не подлежат. В частном случае, когда оба они инлайнятся, и в результате инлайна получается два чтения одного филда, и между ними нет операций записи или выхода за пределы скоупа — тогда, может быть, будет оптимизация (на самом деле, все равно не факт, JIT весьма консервативен).
Запихивать всю эту логику в статический анализатор? Можно, конечно, но…m08pvv
02.12.2015 12:12И не надо засовывать всю эту логику в анализатор. Я говорю о том, что для диагностики V3021 можно снизить количество false positive за счёт предварительного анализа стандартных библиотек (например, mscorlib) и создания списка исключений. Для примера с IsCancellationRequested несложно было понять, что метод не чистый, ибо там читается volatile поле.
lair
02.12.2015 13:15Для примера с IsCancellationRequested несложно было понять, что метод не чистый, ибо там читается volatile поле.
Как раз наоборот, это чистый метод (он не меняет состояние объекта), он недетерминированный.
И с моей точки зрения эта диагностика должна быть opt-in; иными словами, по умолчанию, любой метод считается возвращающим разные значения при последовательных вызовах, а те методы, которые детерминированы, аннотируются отдельно.m08pvv
02.12.2015 13:47Как раз наоборот, это чистый метод (он не меняет состояние объекта), он недетерминированный.
чистая функция, это функция, которая:
является детерминированной;
не обладает побочными эффектами.
Если функция недерминированная, то она по определению не может быть чистой.
И с моей точки зрения эта диагностика должна быть opt-in
Тут уже лучше узнать у авторов анализатора о причинах реализации её как она сейчас есть. Возможно у них есть статистика по подобным ошибкам, а может быть просто не было ресурсов, чтобы добавить поддержку аннотаций.lair
02.12.2015 13:50Если функция недерминированная, то она по определению не может быть чистой.
Ох, опять терминологическое расхождение. Ладно, я не буду спорить, это к сути дела отношения не имеет.
Возможно у них есть статистика по подобным ошибкам,
Интересно, откуда.m08pvv
02.12.2015 13:55Интересно, откуда.
Например, из проверяемых проектов. Прогнали анализатором, нашли N срабатываний V3021, поговорили с разработчиками и выяснили, что M из этих N оказалось ошибками. Далее, основываясь на критичности данных ошибок можно делать выводы о том какой severity должен быть у данной диагностики и надо ли переделывать её на opt-in.
Sioln
01.12.2015 10:28Попробовал. Работает. Что-то находит, спасибо!
Наверное, вы хотели бы знать какую-то статистику?
Чем мы (пользователи) можем быть вам полезны?Sioln
01.12.2015 10:31Не очень понятен мотив отношения ошибки V3022 к High Severity.
В выраженииif (Settings.SessionTTLMinutes == 0) { ... }
SessionTTLMinutes — константа. Ну могут же быть какие-то константы в конфигурации для разных версий продукта.
Sioln
01.12.2015 10:35Ваш анализатор понимает, что Nullable.HasValue и Nullable != null это одно и то же?
Sioln
01.12.2015 11:00+3if (item.OperationID.HasValue && item.OperationID != null)
Вот тут V3027, что мы используем значение, до его проверки на null.
В коде, конечно, есть ошибка, но она другая.
SvyatoslavMC
01.12.2015 11:16+4Некоторые диагностики поддерживают это, некоторые нет. Мы рассмотрим возможность поддержать эту ситуацию во всех диагностиках. Спасибо.
Andrey2008
01.12.2015 10:35В первую очередь интересны сообщения о падениях, глюках и т.п. Также мы будем благодарны за предложения по созданию интересных диагностик. И за явные ложные срабатывания тоже благодарны.
Sioln
01.12.2015 10:50+4Прогнал ваш анализатор по исходникам Entity Framework.
public override bool Equals(object obj) { var right = obj as MemberPath; if (obj == null)//V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'right'. MemberPath.cs 530 { return false; } return Equals(right); }
Прям реальные косяки находит! Вы молодцы.
Sioln
01.12.2015 10:57+3У меня 33 fails. Как их вам передать?
EvgeniyRyzhkov
01.12.2015 11:05+2Лучше прислать проект и способ повторить на support@viva64.com (если проект публичный), ну и .plog конечно
Sioln
01.12.2015 11:33А если проект не публичный? Забить?
Paull
01.12.2015 11:36В идеале — прислать маленький примерчик, на котором повторяется падение. Но мы будем благодарны, если вы пришлёте хотя бы стек падения\plog, если такой возможности нет.
Sioln
01.12.2015 11:40+2Скопировал Fails в буфер обмена, а там:
<Clipboard contents were truncated. A valid license is required for copying a full version of analysis log.>
На горло песне наступили…
leotsarev
01.12.2015 10:53• a team of no more than 9 developers may use the product, the analyzer being installed only on one computer of each user;
У нас в команде 20+ человек сейчас. Я могу себе одному поставить, чтобы проверить общий код в тестовых целях?EvgeniyRyzhkov
01.12.2015 11:02В случае триала — да, конечно. Мы не заставляем сразу всех членов команды пользоваться триалом.
petuhov_k
01.12.2015 11:14+1Не могу найти цены на продукт на вашем сайте. Подозреваю, что они должны быть напротив надписей вида
«10-30 разработчиков, лицензия на первый год
31-50 разработчиков, лицензия на первый год
51-70 разработчиков, лицензия на первый год»
но их там нет — таблица из одной колонки.EvgeniyRyzhkov
01.12.2015 11:21+1Для того чтобы получить цены, надо написать запрос на support@viva64.com. Это не всем нравится, но это более или менее стандартная практика для таких продуктов.
petuhov_k
01.12.2015 11:28+4На странице FAQ, ссылка «Текущие цены смотрите на странице покупки.» ведёт туда, где цен нет. Непорядок.
Greendq
01.12.2015 11:22+2Жаль, что Express Edition вы не поддерживаете. Это значит, что мелкие разработчики — мимо, хотя это и понятно — маленькие команды полную студию себе не купят, а значит, и ваш продукт — тоже. Печально…
Andrey2008
01.12.2015 11:27+6Это не мы Express Edition не поддерживаем. Это Express Edition не поддерживает плагины, которым и является PVS-Studio. Зато мы поддерживаем Community Edition.
Greendq
01.12.2015 12:04Community Edition хочет восьмёрку минимум. А у меня Виста. Десятку покупать надо, так как Виста уже Микрософтом списана де-факто, но всё откладываю до обновления проца, а то потом вопить будет, что активировали на 3 ядрах, а появилось 8 внезапно.
PsyHaSTe
03.12.2015 11:23Висту до 10 все равно не обвновить афайк, только 7/8/8.1 вроде. А коробочная версия как позволяла переустановку на любое число любых компов произвольной конфигурации, так и сейчас позволяет. Учитывая, что 10 — это судя по всему надолго, есть смысл её купить уже сейчас, и не страдать с экспрессами.
Paull
01.12.2015 11:41+2Вы также можете попробовать command line версию. Для просмотра отчёта можно использовать утилиту Standalone.
BloodJohn
01.12.2015 12:34+2Немедленно скачал и проверил Unity проект. PVS нашла кучу очепяток, но есть несколько моментов, которые могут быть вам интересны:
_isCastleInfo = (_structure as Castle) != null;
V3019, я не думаю, что тут проверка _structure на ноль. Хотя конечно _structure is Castle — более правильная.
public IEnumerator ToAnimation() { _position = Transform.position; _rotation = Transform.rotation.eulerAngles; yield return new WaitForSeconds(time); _position = Transform.position; _rotation = Transform.rotation.eulerAngles; }
V3008, Здесь речь идет про корутину, поэтому значение может поменяться в другой задаче
Bronx
01.12.2015 12:39+3На такой код:
string.Format("{0}, {2}, {3}, {4}", p0, p1, p2, p3);
ругается правильно, но сообщение об ошибке немного неточное:
Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 4. Present: 4.
Думаю правильнее «Expected: 5».SvyatoslavMC
01.12.2015 13:57+2Это уже поправлено. Спасибо. В целом о формате сообщения диагностики подумаем.
Greendq
01.12.2015 15:53Кстати, если бы оно поддерживало работу в виде модуля для Юнити (Unity3D) — то я уверен, что сотню-другую лицензий вы бы продали. Даже по 200 баксов за лицензию. Хотя, по сравнению со стоимостью подписки на полную версию — это не так уж и много.
Andrey2008
01.12.2015 16:01+2Больше никаких версий по $200. :) habrahabr.ru/company/pvs-studio/blog/256637
leotsarev
01.12.2015 16:19В общем, солюшен на 150 проектов 1 баг и тот в неиспользуемом коде, но баг такой, что решарпер бы не помог.
BOBS13
01.12.2015 16:19+1Спасибо, инструмент уже может порадовать. В решении 1 000 000 строк кода нашел 64 варнингов первого приоритета. Для даже не беты считаю уже не плохо. Особенно удивил код в одном из старых не используемых классах:
if (this.urlBox != null) { while (this.testHyperLinkControl != null) { this.testHyperLinkControl.NavigateUrl = "javascript:TestURL('" + SPHttpUtility.EcmaScriptStringLiteralEncode(this.urlBox.ClientID) + "')"; break; } }
Andy_Ion
01.12.2015 18:54Прогнал 2 проекта: сервер и клиент, в сервере нашлась V3027 в длинном куске LINQ (использовал string.StartsWith перед string.IsNullOrWhiteSpace), в клиенте же я ещё не успел нашкодить.
Спасибо за предоставленную возможность проверить проекты!
Andrey2008
01.12.2015 20:48Примечание. По аналогии с C++ принимаются «заявки» на проверку проектов. Прошу присылать названия интересных открытых C#-проектов. Будем их потихоньку проверять.
BOBS13
01.12.2015 22:27Первая мысль которя возникла это Roslyn :)
Andrey2008
01.12.2015 22:35Да, будет. Думаю проверкой этого проекта ознаменуется выход Release-версии.
Andriyan
02.12.2015 13:18Попробовал на своих проектах. Ощущения странные.
В одном проекте нашёл явно излишнее повторное присваивание, в другом — явно лишнюю проверку условия. Отлично.
Кроме этого, нашёл повторное присваивание в подобном коде:
int step = 1; try { FirstFunction (); step = 2; SecondFunction (); step = 3; ThirdFunction (); } catch (Exception exception) { Print ("Exception at step {0}: {1}", step, exception); }
Согласен, что это странный способ отладки (кто, кстати, предложит лучше?), но подобное предупреждение лично я могу и игнорировать.
По-настоящему удивило другое. Открыл другой solution, запустил проверку — увидел, что он проверяет 4 файла (уже странно, там гораздо больше файлов) и через небольшое время поздравляет меня с тем, что замечаний у него к моему коду нет. Однако в этом solution'е тоже присутствует мой метод отладки с переменной step — к нему нет замечаний. Подозреваю, что PVS-Studio просто не обрабатывал этот файл.
Запускал проверку из Visual Studio 2010, этот solution впервые был создан какой-то первой версией Visual Studio для C#, затем последовательно апргейдился последующими версиями Visual Studio — возможно, поэтому оказался несовместимым с проверкой PVS-Studio.
Или это ограничения ознакомительной версии?Paull
02.12.2015 13:48Проверьте, пожалуйста, какую платформу\конфигурацию вы проверяете. Если для какой-либо конфигурации сборка проекта отключена, то PVS-Studio не будет проверять такой проект.
Andriyan
02.12.2015 14:20Не помогло. Включил сборку всех проектов (кроме Setup-ов) во всех конфигурациях и платформах — всё равно видит только 4 файла (и не ясно — какие именно).
Прошу проверить текущий файл — тоже не проверяет.Paull
02.12.2015 14:28Что пишется, когда пытаетесь проверить один файл? А если проверить один проект (через контекстное меню в Solution Explorer)? Solution включает обычные csproj проекты?
Также можно попробовать проверить solution напрямую из командной строки с помощью PVS-Studio_Cs.exe, возможно проблема на стороне Visual Studio плагина.Andriyan
02.12.2015 20:42Один файл игнорирует — появляется окно прогресса, затем молча исчезает. Никаких сообщений не появляется.
Один проект через контекстное меню в Solution Explorer — появляется окно прогресса, затем молча исчезает. Никаких сообщений не появляется.
Solution включает в себя проекты csproj, уж не знаю — насколько они обычные, потому что создавались ранними версиями Visual Studio, затем апгрейдились более свежими версиями Visual Studio.
PVS-Studio_Cs.exe работает и даже находит какие-то ошибки, но намекает на необходимость заплатить. Так что похоже, что проблема действительно в плагине для Visual Studio.Paull
03.12.2015 09:48В появляющемся окошке (при проверке одного проекта) указывается какое-то количество файлов?
Может быть файлы в проекты включены не как цели для компиляции (можно посмотреть в свойствах файла)? Также отдельные участки кода не будут проверены, если они заключены в define'ы, не определённые для проверяемого проекта.
Если возможно, пришлите пожалуйста csproj файл на support@viva64.com, мы попробуем разобраться.
Lailore
02.12.2015 15:29Вариант получше: просто запустить в дебаг режиме, и при необработанном исключении у вас остановится программа и будет точное место и контекст.
mayorovp
02.12.2015 17:25Или при обработанном исключении, но если оно вышло за пределы пользовательского кода…
Andriyan
02.12.2015 20:30+3Это понятно.
Проблема в отладке в рабочем окружении, которое трудно воспроизвести в лаборатории. Приходится выводить отладочную печать, но так как объём кода большой, то хочется сначала приблизительно локализовать проблему, а потом ловить её точнее.
dordzhiev
Андрей, а будет ли сравнение с R#? Понимаю, на данном этапе разработки (как вы говорите это даже не beta) может быть неуместным. Но к моменту релиза такое сравнение — must have.
EvgeniyRyzhkov
Сравнения точно не будет, мы зареклись делать любые сравнения. Уверен, что Андрей разовьет эту мысль подробнее. Но смысл в том, что даже когда мы тратим месяц работы команды на подготовку самого объективного сравнения с каким-то инструментом, первый комментарий будет: «Да вы дураки и конечно сделали так, чтобы у вас было лучше!».
Andrey2008
Нет, не будет. Я зарекся делать сравнения. Почему — напишу отдельную статью. В двух словах: это крайне трудоемко и при этом все равно никто не верит, так как сравнение не является независимым.
Используете R#? Отлично! Теперь запустите PVS-Studio и найдите ошибки. :)
Oxoron
Даже сейчас R# поможет в первом случае, частично в четвертом (подсветит серым неюзаный аргумент), но не в третьем. Так что кое-какие мелочи все-равно останутся.
Можно ли прикрутить PVS к TFS? В билд-процессы, или в RM?
lair
Вы про вот это?
Oxoron
Да, проверил, R# предложит упростить до true.
Более того, даже
R# предложит заменить на
dordzhiev
На третий фрагмент мой решарпер говорит «Similar expressions comparison». Версия 9.2.
Paull
Для интеграции анализа C# кода в Continuous Integration / Build Automation и т.п. системы, вы можете использовать Command line анализатор напрямую. Подробная документация доступна тут.
Athari
Всякие unused arg in string.Format, pure function call, var assigned to self, possible NRE и прочее решарпер найдёт в большинстве случаев. Сложные паттерны с похожими ветками, неполные перечисления в switch и прочее — вряд ли. Надо тестировать. В любом случае у PVS, судя по статьям, очень хороший поиск кривой копипасты, а решарпер такое ловит плохо.
mezastel
Строго говоря, большинство примеров, приведенных тут, R# покрывает как проверками, так и контекстными действиями и фиксами для быстрой коррекции.