Microsoft opened the source Xamarin.Forms. We could not miss the chance to check them with PVS-Studio

Not so long ago, as you probably know, Microsoft bought Xamarin. Even though Microsoft recently began to gradually open the source codes of its products, the opening of the Xamarin.Forms code was a big surprise. I could not get past such an event, and decided to check the source code of this project using a static code analyzer.

Analyzed project

Xamarin.Forms is a cross-platform toolkit that allows you to create user interfaces that are common to different platforms: Windows, Windows Phone, iOS, Android. User interfaces are drawn using the native components of the target platform, which allows applications created using Xamarin.Forms to save the overall look for each platform. To create user interfaces with data bindings and different styles, you can use C # or XAML markup.

')
The code for the framework itself is also written in C # and is available in the repository on GitHub .

Analysis tool

The project was tested with the help of the PVS-Studio static code analyzer, in the development of which I take an active part. We are constantly working to improve it, including modifying the existing ones and adding new diagnostic rules. Therefore, with each test of a new project, we manage to identify more and more varieties of errors.

Each diagnostic rule contains documentation that includes a description of the error, as well as examples of incorrect and correct code. You can download a trial version of the analyzer by the link . I also suggest to get acquainted with the note that my colleague recently wrote. She explains why these are just the limitations of the demo version and what should be done to try all the functionality. For those who are too lazy to read, I will prompt you right away - just write to us.

PS In addition, the site has a database of errors found in open source projects, and a catalog of articles (on checking open source projects, technical issues, and others). I recommend to get acquainted.

Suspicious code snippets

Let's start with the "classic" errors detected by the diagnostic rule V3001:

const int RwWait = 1; const int RwWrite = 2; const int RwRead = 4; .... public void EnterReadLock() { .... if ((Interlocked.Add(ref _rwlock, RwRead) & (RwWait | RwWait)) == 0) return; .... } 

PVS-Studio warning : V3001 There are identical sub-expressions "RwWait" operator. SplitOrderedList.cs 458

As can be seen from the code, the value of an expression is calculated using bit operations. In this case, in one of the subexpressions - RwWait | RwWait share the same constant fields. This makes no sense. At the same time, it is clear that the set of constants declared above has values ​​equal to powers of two, therefore, they were meant to be used as flags (which we see in the example of using bit operations). I think it would be more practical to put them in the enumeration marked with the attribute [Flags], which would give a number of advantages when working with this enumeration (see documentation V3059 ).

As for the current example, the use of the RwWrite constant was most likely implied. This can be attributed to one of the cons of IntelliSense - despite the fact that this tool helps a lot in writing code, it can sometimes “prompt” the wrong variable, which can make a mistake by inattention.

The following code example where a similar error was made:

 public double Left { get; set; } public double Top { get; set; } public double Right { get; set; } public double Bottom { get; set; } internal bool IsDefault { get { return Left == 0 && Top == 0 && Right == 0 && Left == 0; } } 

PVS-Studio warning : V3001 There are the "Left & Down" operator. Thickness.cs 29

In the expression 2 times the subexpression Left == 0 is found . Obviously, this is a mistake. In place of the last subexpression, the following code should be placed - Bottom == 0 , since this is the only property (following logic and based on a set of properties) that is not checked in this expression.

The following error is interesting in that it is in two files with the same name and partially similar code. And so it turns out that errors multiply - made a mistake in one place, copied this code to another - op! - here is another wrong place for you.

 public override SizeRequest GetDesiredSize(int widthConstraint, int heightConstraint) { .... int width = widthConstraint; if (widthConstraint <= 0) width = (int)Context.GetThemeAttributeDp(global::Android .Resource .Attribute .SwitchMinWidth); else if (widthConstraint <= 0) width = 100; .... } 

PVS-Studio warning : V3003 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 28, 30. Xamarin.Forms.Platform.Android SwitchRenderer.cs 28

In this code snippet, there is strange logic in the if statement . Some condition is checked ( widthConstraint <= 0 ) and, if it is not met, the same condition is checked again. Mistake? Mistake. But how to fix, is more difficult to say. This task already falls on the shoulders of the programmer who wrote the code.

As I said, there was exactly the same error in the file with the same name. Here is the corresponding analyzer message: V3003 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 26, 28. Xamarin.Forms.Platform.Android SwitchRenderer.cs 26

Thanks to the mechanism of virtual values, it was possible to significantly improve a number of diagnostic rules, including the diagnosis V3022 , which determines that the expression is always true or false . I propose to look at a few examples that were found with its help:

 public TypeReference ResolveWithContext(TypeReference type) { .... if (genericParameter.Owner.GenericParameterType == GenericParameterType.Type) return TypeArguments[genericParameter.Position]; else return genericParameter.Owner.GenericParameterType == GenericParameterType.Type ? UnresolvedGenericTypeParameter : UnresolvedGenericMethodParameter; .... } 

PVS-Studio warning : V3022 Expression 'genericParameter.Owner.GenericParameterType == GenericParameterType.Type' is always false. ICSharpCode.Decompiler TypesHierarchyHelpers.cs 441

Despite the fact that I deleted that part of the method that does not interest us, even now the error may not be too noticeable. To remedy this situation, I propose to simplify the code a little more by rewriting it with shorter variable names:

 if (a == enVal) return b; else return a == enVal ? c : d; 

Now everything has become somewhat clearer. The source of the problem is the second check a == enVal (genericParameter.Owner.GenericParameterType == GenericParameterType.Type) located in the ternary operator. The ternary operator in the else- branch of the if operator is meaningless - in this case, the method will always return the value d ( UnresolvedGenericMethodParameter ).

If you have not guessed what the problem is - I explain. If the program reaches the calculation of the value of the ternary operator, it is already known that the expression a == enVal is false , therefore, in the ternary operator it will have the same value. The bottom line: the result of the ternary operator is always the same. A mistake.

Immediately it is difficult to detect such defects, because even cutting out the extra code from the method, the error is cleverly concealed in the rest. I had to resort to additional simplifications in order to reveal this “pitfall”. However, there is no such problem for the analyzer, and he easily coped with the task.

Of course, this is not the only such case. Here is another one:

 TypeReference DoInferTypeForExpression(ILExpression expr, TypeReference expectedType, bool forceInferChildren = false) { .... if (forceInferChildren) { .... if (forceInferChildren) { InferTypeForExpression(expr.Arguments.Single(), lengthType); } } .... } 

PVS-Studio warning : V3022 Expression 'forceInferChildren' is always true. ICSharpCode.Decompiler TypeAnalysis.cs 632

Again, in order to more easily notice the catch, cut out all the extra code. And here it is - the forceInferChildren condition is checked 2 times, while this variable is not used between if statements . If we consider that this is a parameter of the method, we can conclude that neither other threads nor any methods can change it without direct reference. Therefore, if the first if statement is executed, the second one will always be executed. Strange logic.

There is a diagnosis similar to the V3022 - V3063. This diagnostic rule determines that a part of a conditional expression is always true or false. Thanks to her, she managed to find some interesting code fragments:

 static BindableProperty GetBindableProperty(Type elementType, string localName, IXmlLineInfo lineInfo, bool throwOnError = false) { .... Exception exception = null; if (exception == null && bindableFieldInfo == null) { exception = new XamlParseException( string.Format("BindableProperty {0} not found on {1}", localName + "Property", elementType.Name), lineInfo); } .... } 

PVS-Studio warning : V3063 A part of conditional expression is always true: exception == null. Xamarin.Forms.Xaml ApplyPropertiesVisitor.cs 280

We are interested in the subexpression exception == null . Obviously, it will always be true . Why then this check? It is not clear. By the way, there are no comments that signal in some way that the value can be changed during the debugging process (like // new Exception (); ).

These are not the only suspicious sites found by diagnostic rules V3022 and V3063. But let's not dwell on them, but let's see what else we managed to find interesting.

 void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } 

PVS-Studio warning : V3038 The first argument of 'Replace' function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

From this code, we are interested in the Replace method, called for some string. Apparently, the programmer wanted to replace all single quote characters with the slash and quotation marks . But the fact is that in the second case, the slash character is escaped, so the call to this method replaces the single quote with it. Do not believe? Equals ("'", "\'") to help. For some, this may not be obvious, but not for the analyzer. To avoid escaping, you can use the @ character in front of a string literal. Then the correct call to the Replace method would look like this:

 Replace("'", @"\'") 

There were methods that always return the same value. For example:

 static bool Unprocessed(ICollection<string> extra, Option def, OptionContext c, string argument) { if (def == null) { .... return false; } .... return false; } 

PVS-Studio Warning: V3009 This is the same as the value of 'false'. Xamarin.Forms.UITest.TestCloud OptionSet.cs 239

No matter what arguments come in and what is executed in this method, it always returns false. Agree, it looks like something strange.

By the way, this code met again - the method was copied completely and moved to another place. Analyzer Message: V3009 This is the same as the value of 'false'. Xamarin.Forms.Xaml.Xamlg Options.cs 1020

We met several code fragments with the repeated generation of an exception, potentially containing errors.

 static async Task<Stream> GetStreamAsync (Uri uri, CancellationToken cancellationToken) { try { await Task.Delay (5000, cancellationToken); } catch (TaskCanceledException ex) { cancelled = true; throw ex; } .... } 

PVS-Studio warning : V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. Xamarin.Forms.Core.UnitTests ImageTests.cs 221

It would seem the logic is simple. In case of an exception, we perform some actions and re-generate it. But the devil is in the details. In this case, when re-generating an exception, the original exception stack is completely “overwritten”. To avoid this, you do not need to re-generate the same exception; it is enough to “flip” an existing one by calling the throw operator. Then the catch block code might look like this:

 cancelled = true; throw; 

A similar example:

 public void Visit(ValueNode node, INode parentNode) { .... try { .... } catch (ArgumentException ae) { if (ae.ParamName != "name") throw ae; throw new XamlParseException( string.Format("An element with the name \"{0}\" already exists in this NameScope", (string)node.Value), node); } } 

PVS-Studio warning : V3052 The original exception object 'ae' was swallowed. Stack of original exception could be lost. Xamarin.Forms.Xaml RegisterXNamesVisitor.cs 38

In both cases, information about the previous exception is lost. And if in the second case, it can still be assumed that this information will not be relevant (although it is still strange), then in the first case it is unlikely that they most likely wanted to throw the exception up, but instead generated a new one. The solution is the same as in the previous example - calling the throw operator without any arguments.

At the expense of the next fragment - I will not undertake to say for sure whether it is a mistake or not, but it looks strange.

 void UpdateTitle() { if (Element?.Detail == null) return; ((ITitleProvider)this).Title = (Element.Detail as NavigationPage) ?.CurrentPage?.Title ?? Element.Title ?? Element?.Title; } 

PVS-Studio warning : V3042 Possible NullReferenceException. The '?.' and '.' Xamarin.Forms.Platform.WinRT MasterDetailPageRenderer.cs 288

The analyzer was alarmed by the fact that the Title property is accessed in several ways - Element.Title and Element? .Title , and first are accessed directly and then using a null-conditional operator. But everything is not so simple.

As you can see, at the beginning of the method, the Element is checked ? .Detail == null , assuming that if Element == null , then the output will be realized here and it will not get to further operations.

At the same time, the expression Element? .Title assumes that at the time of its execution the Element may be null . If this is the case, then at the previous stage, when the Title property is directly accessed , an exception of the NullReferenceException type will be generated, and therefore there is no point in using the null-conditional operator.

In any case, this code looks very strange and needs to be fixed.

It looked weird when the object was cast to its own type. Here is an example of such a code:

 public FormsPivot Control { get; private set; } Brush ITitleProvider.BarBackgroundBrush { set { (Control as FormsPivot).ToolbarBackground = value; } } 

PVS-Studio warning : V3051 An type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 73

In this case, this is not an error, but the code looks at least suspicious, given that the Control object already has the type FormsPivot. By the way, this is not the only warning of this kind; others have also met:

• V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 78
• V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 282
• V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 175
• V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 197
• V3051 An excessive type cast. The object is already of the 'FormsPivot' type. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 205

There are conditions that could be simplified. An example of one of them:

 public override void LayoutSubviews() { .... if (_scroller == null || (_scroller != null && _scroller.Frame == Bounds)) return; .... } 

PVS-Studio Warning: V3031 Alert can be simplified. The '||' operator is surrounded by opposite expressions. Xamarin.Forms.Platform.iOS.Classic ContextActionCell.cs 102

This expression can be simplified by removing the _scroller! = Null subexpression. It will be calculated only in the case when the expression to the left of the operator '||' is false. - _scroller == null , therefore - _scroller is not null and you can be safe from getting a NullReferenceException exception. Then the simplified code will look like this:

 if (_scroller == null || _scroller.Frame == Bounds)) 

A spoon of tar

Unfortunately, it was not possible to assemble the solution completely - about 6 projects remained untested, and those places where classes of them were somehow used were not subjected to such careful analysis as they could. Perhaps they could have found something else interesting, but, alas.

By the way, you can learn about the problems with the analysis of the message V051 , located at level 3. The presence of such warnings, as a rule, is a signal that the C # project contains some kind of compilation errors, which is why the analyzer cannot get all the information necessary for conducting a complex analysis. However, he will try to perform checks for which detailed information on types and objects is not needed.

When checking the project, it is advisable to make sure that you do not have warnings V051. And if they are still there - try to get rid of them (check that the project is compiled, make sure that all dependencies are loaded).

Conclusion

Checking Xamarin.Forms proved itself - there were various interesting places, both clearly erroneous, and very suspicious or strange. I hope the developers will not bypass the article and correct the fragments of code written here. All suspicious places that could be detected can be viewed by downloading a trial version of the analyzer . The introduction of PVS-Studio on an ongoing basis will be even better and more correct, which will allow detecting and correcting errors immediately after they appear.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Microsoft opened the source code of Xamarin.Forms. We couldn’t miss a chance to check it with PVS-Studio .