У нас появилась экспериментальная версия анализатора PVS-Studio, который умеет анализировать C#-проекты и который можно показать миру. Это не Release, и даже не Beta. Это просто текущая сборка PVS-Studio. Мы хотим как можно раньше начать получать отзывы от наших пользователей или потенциальных пользователей касательно поддержки C#. Поэтому предлагаем энтузиастам попробовать новую версию PVS-Studio на своих C#-проектах и рассказать нам о результатах, недостатках и высказать свои пожелания. Ах да, и, конечно, в статье будут описаны результаты проверки очередного проекта - SharpDevelop.
Сейчас одним из важных вопросов является: "Зачем делать очередной инструмент анализа кода для C#?".
Попробуем ответить как потенциальным пользователям, так и самим себе, чтобы ясно понимать куда и зачем мы движемся.
Мы успешно создали и продолжаем развивать анализатор PVS-Studio для языка C/C++. В этом анализаторе реализовано много интересных и уникальных идей по выявлению разнообразных типов ошибок. Со временем стало понятно, что многие из реализованных диагностик никак не связаны с конкретным языком программирования. Не важно, какой язык вы используете. Всегда будут опечатки, ошибки из-за невнимательности или неудачного Copy-Paste.
И тогда мы решили попробовать применить свой опыт к другому языку программирования, к C#. Насколько это будет успешным начинанием, покажет время. Сами мы считаем, что постепенно сможем создать очень интересный инструмент, пользу от которого может получить большое количество C#-разработчиков.
Сейчас наша задача - начать как можно раньше получать отзывы от наших потенциальных пользователей. Полноценная версия анализатора PVS-Studio ещё не готова. Сейчас в ней мало диагностик (на момент написания статьи их было 36). Но эту версию можно установить и попробовать. И мы будем благодарны всем, кто это сделает. Нам важно убедиться, что мы вообще движемся в верном направлении и что анализатор в целом работоспособен. А у уж добавлять новые и новые диагностики мы будем очень быстро.
Примечание. Со временем приведенная выше ссылка станет недействительной. Поэтому, если вы читаете эту статью по пришествию месяца или более с момента публикации, то предлагаем вам установить текущую версию дистрибутива: http://www.viva64.com/ru/pvs-studio/download/
Если читатель ещё не разу не пробовал PVS-Studio, то предлагаю ознакомиться со статьёй "PVS-Studio для Visual C++". Как вы видите, она ориентирована на C++, но на самом деле никакой разницы нет. С точки зрения интерфейса вообще почти нет никакой разницы, работаете вы с C++ или с C# проектами.
Чтобы отправить свои отзывы и пожелания, вы можете воспользоваться страницей обратной связи.
Для программистов обыкновенная реклама не работает. Однако я знаю, как привлекать внимание этих серьезных и очень занятых творцов. Мы проверяем различные открытые проекты и пишем про это статьи. Нет лучшей рекламы, чем показать, что умеет наш инструмент.
Не вижу смысла изобретать велосипед. Теперь тем же методом я буду очаровывать 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 таких ошибки:
Фрагмент 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# программах!
А если серьезно, то: