Our team is working on an experimental version of the PVS-Studio analyzer that from now on can analyze C# projects. This is neither a Release, nor even a Beta version. It's just a current build of PVS-Studio. We would like to start getting feedback from our users or potential users regarding C# support as soon as possible. Therefore we offer C# enthusiasts to try running a new version of PVS-Studio on your C# projects, and share with us the results. Your opinion on advantages/faults and recommendations about PVS-Studio for C++/C# will be highly appreciated. And of course in this article we are going to tell about another project check - this time SharpDevelop.
Now one of the most important questions for us is: "Why should we make another analysis tool for C# in general?"
I'll try to come up with a decent answer for us and our potential clients, as we need to have a clear understanding of where and why we are going.
We have successfully created PVS-Studio analyzer for C/C++ languages and continue developing it. We've implemented a great deal of interesting and unique ideas on bug detection. In the course of time we realized that a big number of diagnostics is not related to a particular programming language, i.e. it doesn't matter which language you use, as there will always be typos and errors due to carelessness or Copy-Paste usage.
So we decided to try applying our experience to another programming language, namely C#. Time will tell if it is going to be a successful affair. To our own humble opinion, we'll be able to create a very useful tool that many C# developers can benefit from.
And now our main task is to start getting feedback from our potential users as soon as possible. I should warn that the full version of PVS-Studio analyzer is not ready yet. At this point there are few diagnostics in it (at the moment of writing of this article, there were 36 of them). But you can already install and try this version now. We'll be really grateful to anyone who would do that. It is important for us to make sure that we are moving in the right direction and that the analyzer is runnable in general. Adding new diagnostics is a quick process.
Note. With time this link will become a dead one. That's why if you happen to read this article in a month or more from the moment of its publication, I advise installing a current version of the distribution: http://www.viva64.com/en/pvs-studio/download/
If our dear reader hasn't tried PVS-Studio before, I suggest having a look at this article: PVS-Studio for Visual C++. As you see, it is about C++, but in reality, there is no big difference. Interface-wise there is almost no difference, whether you work with C++ or C# projects.
So, in case if you want to contribute to C# analyzer development, you can send your feedback and recommendations, using our feedback page.
What we have also realized is that usual ways of advertising don't work for programmers. But I think I know how to attract attention of these serious and very busy creators. We check various open-source projects and write articles about it. There is no better marketing than showing, what the tool is capable of.
So I don't see the point in reinventing the wheel. I'll try to use the same method to get attention of C# programmers. And here is another article about checking an open-source project SharpDevelop.
SharpDevelop is a free IDE for C#, VisualBasic .NET, Boo, IronPython, IronRuby, F# and C++. Typically it is used as an alternative to Visual Studio .NET.
For us it is mostly important that the project is written entirely in C#, which means that we can check it with our experimental version of PVS-Studio. In this project there are 8522 files with the "cs" extension, the total size of which is 45 megabytes.
Fragment N1
public override string ToString()
{
return String.Format("Thread Name = {1} Suspended = {2}",
ID, Name, Suspended);
}PVS-Studio warning V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. Thread.cs 235
ID variable isn't used in any way. May be there is no actual bug here. However, this fragment is clearly worth checking out. Perhaps a completely different string was meant here.
Fragment N2
public override string ToString ()
{
return
String.Format ("[Line {0}:{1,2}-{3,4}:{5}]",
File, Row, Column, EndRow, EndColumn, Offset);
}PVS-Studio warning: V3025 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 4. Present: 6. MonoSymbolTable.cs 235
This is a more interesting case. It's not very clear what the programmer wanted to say. Probably he wanted the message to be like this:
[Line file.cs:10,20-30,40:7]
But apparently he missed some curly brackets. So it turns out that the ",2" and ",4" specify field alignment, rather than display the values of EndRow and EndColumn variables.
I'll dare suggesting that it would be correct to use the following formatting string:
String.Format ("[Line {0}:{1},{2}-{3},{4}:{5}]",
File, Row, Column, EndRow, EndColumn, Offset);Fragment 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 warning: V3001 There are identical sub-expressions 'mc_a.Location.File' to the left and to the right of the '!=' operator. membercache.cs 1306
We have a typo here. I think a correct option will be the following comparison:
if (mc_a.Location.File != mc_b.Location.File)Fragment N4
public WhitespaceNode(string whiteSpaceText,
TextLocation startLocation)
{
this.WhiteSpaceText = WhiteSpaceText;
this.startLocation = startLocation;
}PVS-Studio warning V3005 The 'this.WhiteSpaceText' variable is assigned to itself. WhitespaceNode.cs 65
Quite a nice bug. And the static analyzer fully revealed its capabilities. It is still being attentive and unlike a human being does not get tired. That's why it noticed a typo. Do you see it? We have to admit, it's not easy to find a bug here.
Just one letter was mistyped. "=whiteSpaceText" should have been written instead of "=WhiteSpaceText". As a result, the value of 'WhiteSpaceText' in the class remains unchanged.
In general, it is a good example of how not to name the variables. It's a bad idea to differentiate the names only by one lowercase or uppercase letter. But arguments about a coding style may go beyond the topic of the article. Moreover, it can lead to a holy debating war.
Fragment 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 warning V3004 The 'then' statement is equivalent to the 'else' statement. Editor.cs 225
It is very suspicious that the same actions will be carried out despite of the 'this.InvokeRequired' value. I am almost convinced that the string "base. Enabled = ... " has been copied. And then something was left unchanged.
Fragment 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 warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'node', 'solutionFolderNode'. SolutionNodeCommands.cs 127
We see that some actions were intended to be carried out, if 'node' is inherited from 'ISolutionFolderNode' interface. But a wrong variable got checked. Correct variant:
ISolutionFolderNode solutionFolderNode =
node as ISolutionFolderNode;
if (solutionFolderNode != null)
{By the way, this is a fairly common error pattern in C# programs. The analyzer detected 3 more similar errors in SharpDevelop:
Fragment N10
public override void VisitInvocationExpression(....)
{
....
foundInvocations = (idExpression.Identifier == _varName);
foundInvocations = true;
....
}PVS-Studio warning: V3008 The 'foundInvocations' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 211, 209. RedundantAssignmentIssue.cs 211
A very suspicious repeating assignment. Perhaps the second assignment was written during the code debugging, and then the programmer just forgot about it.
Fragment 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 warning: V3008 The 'pos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 151, 148. CodeSnippet.cs 151
Another repeating assignment. Here we have either a bug or the "pos = m.Index;" is unnecessary here.
Fragment N12
....
public string Text { get; set; }
....
protected override void OnKeyUp(KeyEventArgs e)
{
....
editor.Text.Insert(editor.CaretIndex, Environment.NewLine);
....
}PVS-Studio warning: V3010 The return value of function 'Insert' is required to be utilized. InPlaceEditor.cs 166
In C# strings are unalterable. Therefore, if we do something with the string, the result should be saved somewhere else. However, it is easy to forget about it, as it happened here for example. The developer decided that by calling the Insert() method, he will add something to the string. But this is not the case. Correct code variant:
editor.Text =
editor.Text.Insert(editor.CaretIndex, Environment.NewLine);Fragment 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 warning: V3010 The return value of function 'Union' is required to be utilized. MappingBase.cs 274
All in all, I get the feeling that in C# projects, we'll see plenty of errors related to the fact that the programmer expects some changes in the object but they don't happen.
Extension method 'Union' defined for collections that implement the IEnumerable interface, allows you to get the intersection of two multitudes. However, the 'value' container doesn't get changed. Correct variant:
value = value.Union(baseMapping.GetMappingForTable(table));Another similar situation can be found here: V3010 The return value of function 'OrderBy' is required to be utilized. CodeCoverageMethodElement.cs 124
Fragment N15
PVS-Studio analyzer tries to detect situations where a programmer might have forgotten to do something in switch(). The logics of making a decision, whether to issue a warning or not is rather complicated. Sometimes you get false positives, sometimes these appear to be real bugs. Let's have a look at one of such false positives.
So we have such enumeration in the code:
public enum TargetArchitecture {
I386,
AMD64,
IA64,
ARMv7,
}Here and there you can see all variants of such enumeration:
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 ();
}However, there are suspicious fragments as well. For example, the analyzer drew my attention to the following code fragment:
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 warning: V3002 The switch statement does not cover all values of the 'TargetArchitecture' enum: ARMv7. ImageWriter.cs 209
As you see, we don't take into consideration the case with the the ARMv7 architecture. I'm not sure if it is an error or not. But it seems to me that there is a bug here. ARMv7 name is at the end of the enumeration, which means that it was added last. As a result, the programmer could have forgotten to fix the GetMachine() function and take this architecture into account.
Fragment 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 warning: V3005 The 'linearGradientBrush.GradientStops' variable is assigned to itself. BrushEditor.cs 120
This code fragment is rather difficult to read. Obviously that is the reason of bug being here. Most likely the code was written with Copy-Paste method and was incorrectly changed in one fragment.
Apparently, instead of:
linearGradientBrush.GradientStops =
linearGradientBrush.GradientStops;There should have been written this code:
linearGradientBrush.GradientStops =
radialGradientBrush.GradientStops;On the one hand, some fragments that analyzer points to are not actual bugs. On the other hand, the messages issued in such code, can't be called false positives as well. Usually we say that this code smells.
We have reviewed many code fragments that most likely contain bugs. Now let me give you a few examples of the smelling code. Of course I won't be looking at all cases, it's not very interesting. I will limit myself to 3 examples. You can have a look at the rest of the "smells" by running the analyzer on the SharpDevelop project yourself.
Smelling code snippet 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 warning: 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
As you can see, the program contains two identical blocks. But the condition of the lower 'if' block will never be true. To my mind it's not a bug, as to me it looks like the block was unintentionally duplicated and it is unnecessary here. Nevertheless, it is a place that is worth reviewing and fixing.
Smelling code snippet N2
void PropertyExpandButton_Click(object sender, RoutedEventArgs e)
{
....
ContentPropertyNode clickedNode =
clickedButton.DataContext as ContentPropertyNode;
clickedNode = clickedButton.DataContext as ContentPropertyNode;
if (clickedNode == null)
....
}PVS-Studio warning: V3008 The 'clickedNode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 105, 104. PositionedGraphNodeControl.xaml.cs 105
The code is redundant code and can be simplified to:
ContentPropertyNode clickedNode =
clickedButton.DataContext as ContentPropertyNode;
if (clickedNode == null)Smelling code snippet N3
IEnumerable<ICompletionData>
CreateConstructorCompletionData(IType hintType)
{
....
if (!(hintType.Kind == TypeKind.Interface &&
hintType.Kind != TypeKind.Array)) {
....
}PVS-Studio warning: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. CSharpCompletionEngine.cs 2392
Redundant code. The expression can be simplified:
if (hintType.Kind != TypeKind.Interface) {I can go on, but perhaps that's enough. All other "smells" are too boring and will look like a dull list of quasi errors.
Well, as you can see, C# doesn't guarantee a total protection from some stupid errors. That's why with clear conscience I can put such a picture here.
Long live the Unicorn that now can find errors in C #programs!
Speaking seriously: