Checking the source code of the C # /. NET component kit from Sony

As some of you remember, we recently released a version of the analyzer that supports checking C # code . With the advent of analyzing the verification of projects written in C #, a new space for creativity opens up. On the analysis of one of these projects, developed by Sony Computer Entertainment (SCEI), and will be discussed in this article.

What was checked?

Sony Computer Entertainment is a video game company, one of the divisions of Sony Corporation that specializes in video games and game consoles. This company develops video games, hardware and software for the PlayStation consoles.


')
The Authoring Tools Framework (ATF) is a set of C # /. NET components for developing tools for Windows. ATF is used by most of the advanced studios from SCEI that develop video games for making customized tools. This set of components is used by such studios as Naughty Dog, Guerrilla Games, Quantic Dream. In particular, the tools developed using these software components were used to create well-known games such as The Last of Us and Killzone. ATF is an open source project that is available in the repository on GitHub .

Analysis tool

For analyzing the source code, the PVS-Studio static analyzer was used. This tool can validate source code written in C / C ++ / C # languages. Each diagnostic message is described in detail in the documentation, which provides examples of incorrect code and ways to correct it. Many diagnostic descriptions have a link to the corresponding section of the error database , which contains information about what errors in real projects could be detected with the help of various diagnostics.



Download and try the analyzer on your (or someone else's) project, you can link .

Error examples

public static void DefaultGiveFeedback(IComDataObject data, GiveFeedbackEventArgs e) { .... if (setDefaultDropDesc && (DropImageType)e.Effect != currentType) { if (e.Effect != DragDropEffects.None) { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } else { SetDropDescription(data, (DropImageType)e.Effect, e.Effect.ToString(), null); } .... } } 

Analyzer Warning: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199

As can be seen from the code, regardless of the truth of the condition 'e.Effect! = DragDropEffects.None', the same method will be called with the same arguments. Not being a programmer who wrote this code, it is sometimes quite difficult to say how to fix this place correctly, but I think no one doubts that something needs to be fixed. What exactly - to decide the author of the code.

Consider the following code snippet:

 public ProgressCompleteEventArgs(Exception progressError, object progressResult, bool cancelled) { ProgressError = ProgressError; ProgressResult = progressResult; Cancelled = cancelled; } 

Analyzer Warning: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24

The implication was that when the method was called, the properties would be assigned the values ​​passed as arguments, and the names of the properties and parameters differ only in the case of the first letter. As a result, the 'ProgressError' property is written to itself instead of getting the value of the 'progressError' parameter.

What is interesting is not an isolated case of confusion between upper and lower case letters. Some proven projects encountered errors of exactly the same type. There is a suspicion that we will soon reveal a new sample error pattern, typical of programs written in C #. There is a tendency to initialize properties in a method whose parameter names differ from the name of the properties being initialized only by the first letter register. As a result, errors like this one appear. In the following code example, if it is not erroneous, then it looks at least strange:

 public double Left { get; set; } public double Top { get; set; } public void ApplyLayout(XmlReader reader) { .... FloatingWindow window = new FloatingWindow( this, reader.ReadSubtree()); .... window.Left = window.Left; window.Top = window.Top; .... } 

Analyzer Warnings:

• V3005 The 'window.Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706
• V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707

From the code and warnings of the analyzer you can see that the properties of the 'Left' and 'Top' of the 'window' object are written into themselves. For some cases, this option is acceptable, for example, when special logic is 'hung' on the property access method. However, there is no additional logic for these properties, therefore the reasons for writing such code remain unclear.

The following example:

 private static void OnBoundPasswordChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) { PasswordBox box = d as PasswordBox; if (d == null || !GetBindPassword(d)) { return; } // avoid recursive updating by ignoring the box's changed event box.PasswordChanged -= HandlePasswordChanged; .... } 

Analyzer warning: V3019 Possibly incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38

This type of error has also been repeatedly encountered in proven C # projects. As a result of casting an object to a compatible type using the 'as' operator, a new object is obtained; however, further in the code, the original object is checked for equality to 'null'. This code can work quite correctly if it is known that the object 'd' will always be compatible with the type 'PasswordBox'. But if this is not the case (now, or later something will change in the program), then you will easily get a 'NullReferenceException' on the code that used to work normally. So in any case, this code is worth fixing.

In the following example, the programmer, on the contrary, tried to protect himself by all available means, only a little wondering why:

 public Rect Extent { get { return _extent; } set { if (value.Top < -1.7976931348623157E+308 || value.Top > 1.7976931348623157E+308 || value.Left < -1.7976931348623157E+308 || value.Left > 1.7976931348623157E+308 || value.Width > 1.7976931348623157E+308 || value.Height > 1.7976931348623157E+308) { throw new ArgumentOutOfRangeException("value"); } _extent = value; ReIndex(); } } 

Analyzer Warning: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575

This condition will always be false. If it is not clear - let's understand why.

This is an implementation of a property of type 'Rect', therefore, 'value' is also of type 'Rect'. 'Top', 'Left', 'Width', 'Height' are properties of this type that are of type 'double'. In this code, it is checked whether the values ​​of these properties are outside the range of values ​​that the 'double' type can take. Moreover, for comparison, 'magic numbers' are used, rather than constants defined in the 'double' type. Since values ​​of type 'double' are always in this range, this condition will always be false.

Apparently the programmer wanted to protect his program from a non-standard implementation of the 'double' type in the compiler. Nevertheless, it looks strange, so the analyzer quite reasonably issued a warning, prompting the programmer to double-check the code.

Go ahead:

 public DispatcherOperationStatus Status { get; } public enum DispatcherOperationStatus { Pending, Aborted, Completed, Executing } public object EndInvoke(IAsyncResult result) { DispatcherAsyncResultAdapter res = result as DispatcherAsyncResultAdapter; if (res == null) throw new InvalidCastException(); while (res.Operation.Status != DispatcherOperationStatus.Completed || res.Operation.Status == DispatcherOperationStatus.Aborted) { Thread.Sleep(50); } return res.Operation.Result; } 

Analyzer Warning: Consider inspecting this expression V3023 . The expression is misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74

The condition for executing the 'while' loop is redundant, it could be simplified by removing the second subexpression. Then the cycle could be simplified to the following form:

 while (res.Operation.Status != DispatcherOperationStatus.Completed) Thread.Sleep(50); 

The following interesting example:

 private Vec3F ProjectToArcball(Point point) { float x = (float)point.X / (m_width / 2); // Scale so bounds map // to [0,0] - [2,2] float y = (float)point.Y / (m_height / 2); x = x - 1; // Translate 0,0 to the center y = 1 - y; // Flip so +Y is up if (x < -1) x = -1; else if (x > 1) x = 1; if (y < -1) y = -1; else if (y > 1) y = 1; .... } 

Analyzer Warnings:

• V3041 The expression was an implicitly cast from 'int' type to 'float' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216
• V3041 The expression was an implicitly cast from 'int' type to 'float' type. Consider using a fractional part. An example: double A = (double) (X) / Y ;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217

This is again one of those cases where it is very difficult for a third-party developer to say for sure whether the code contains an error or not. On the one hand, the implementation of integer division followed by an implicit reduction to the real type looks strange. On the other hand, sometimes they do it intentionally, neglecting lost accuracy.

What was meant here is hard to say. Probably, they didn’t want to lose accuracy at all, but as a result of the operation 'm_width / 2' this loss will still occur. Then the code would need to be fixed to the following:

 float x = point.X / ((float)m_width / 2); 

On the other hand, it is possible that they wanted to write an integer number in 'x', since further comparison operations with integer values ​​are performed. In this case, it was not necessary to perform an explicit conversion of the code to the type 'float':

 float x = point.X / (m_width / 2); 

Our analyzer grows and develops, respectively, updated with new interesting diagnostics. The next error was just found one of these new diagnostics. Since this diagnosis is not included in the release at the time of this writing, I can’t provide a link to the documentation. I hope, here and so everything is clear:

 public static QuatF Slerp(QuatF q1, QuatF q2, float t) { double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W; if (dot < 0) q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; .... } 

Analyzer Warning: V3043 does not correspond with its formatting. The statement is indented. It is possible that curly brackets are missing. Atf.Core.vs2010 QuatF.cs 282

From the code you can see that the sum of several products is calculated and the result of this operation is written to the variable 'dot', after which, if the value of 'dot' is negative, the inversion of all values ​​involved in the operation is performed. More precisely, it was meant inversion, judging by the formatting of the code. In fact, in this case only the property 'X' of the object 'q1' will be inverted, and all other properties will be inverted regardless of the value of the variable 'dot'. The solution to the problem is braces:

 if (dot < 0) { q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W; } 

Go ahead:

 public float X; public float Y; public float Z; public void Set(Matrix4F m) { .... ww = -0.5 * (m.M22 + m.M33); if (ww >= 0) { if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); X = (float)wwSqrt; ww = 0.5 / wwSqrt; Y = (float)(m.M21 * ww); Z = (float)(m.M31 * ww); return; } } else { X = 0; Y = 0; Z = 1; return; } X = 0; ww = 0.5 * (1.0f - m.M33); if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); Y = (float)wwSqrt; //<= Z = (float)(m.M32 / (2.0 * wwSqrt)); //<= } Y = 0; //<= Z = 1; //<= } 

Analyzer Warnings:

• V3008 The 'Y' variable is assigned twice successively. Perhaps this is a mistake. Check lines: 221, 217. Atf.Core.vs2010 QuatF.cs 221
• V3008 The 'Z' variable is assigned twice twice successively. Perhaps this is a mistake. Check lines: 222, 218. Atf.Core.vs2010 QuatF.cs 222

I deliberately cited an additional piece of code to make the error more visual. 'Y' and 'Z' are instance fields. Depending on certain conditions, these or those values ​​are written to these fields, after which the method is terminated. But in the body of the last 'if' operator, they forgot to write the 'return' operator, which is why the fields will not be assigned the same values ​​that were meant. Then the correct code might look like this:

 X = 0; ww = 0.5 * (1.0f - m.M33); if (ww >= EPS2) { double wwSqrt = Math.Sqrt(ww); Y = (float)wwSqrt; Z = (float)(m.M32 / (2.0 * wwSqrt)); return; } Y = 0; Z = 1; 

On this, perhaps, we will stop. These places seemed the most interesting, so I decided to write them out and make out. Other errors were found, in particular - I didn’t consider warnings with a low level of importance at all, I wrote out a few warnings from medium and high levels.

Conclusion

As you can see, no one is insured against errors, and due to carelessness it is easy to assign the object to oneself or to miss some operator. In a large amount of code, such errors are often difficult to detect visually, moreover, not all of them manifest themselves immediately - some of them will shoot you in the foot a couple of years later. To prevent this, a static analyzer is needed to detect errors at an early stage, reduce the cost of project development, protect your nerves and keep your feet intact.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Sony C # /. NET component set analysis .