# Microsoft opened the source code of Xamarin\.Forms\. We couldn't miss a chance to check it with PVS\-Studio You probably already know that the Microsoft Corporation bought the Xamarin Company\. Even though Microsoft has started gradually opening the source code of some of its products, the Xamarin\.Forms code was a big surprise\. I couldn't give it the go\-by, and decided to check the code using a static code analyzer\. ![0400_XamarinForms/image1.png](https://import.viva64.com/docx/blog/0400_XamarinForms/image1.png) ## The project to be analyzed [Xamarin\.Forms](https://dotnet.microsoft.com/apps/xamarin/xamarin-forms) is a cross\-platform natively backed UI toolkit abstraction that allows developers to easily create user interfaces that can be shared across Android, iOS, Windows, and Windows Phone\. The user interfaces are rendered using the native controls of the target platform, allowing Xamarin\.Forms applications to retain the appropriate look and feel for each platform\. You can use code or markup to build a UI with data\-binding and styles, using either C\# or XAML markup\. ![0400_XamarinForms/image2.png](https://import.viva64.com/docx/blog/0400_XamarinForms/image2.png) The code of the framework is written in C\# and is available at a [repository on GitHub](https://github.com/xamarin/xamarin.forms)\. ## Analysis tool The project was checked using the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) static code analyzer; in the development of which I take an active part\. We are constantly working on its improvement, modifying and adding new diagnostic rules\. This is why with every new project check we are able to find a larger variety of bugs\. ![0400_XamarinForms/image4.png](https://import.viva64.com/docx/blog/0400_XamarinForms/image4.png) Each diagnostic rule has [documentation](https://pvs-studio.com/en/docs/warnings/), which includes a description of the error, as well as examples of the incorrect, and correct, code\. The trial version of the analyzer can be downloaded [here](https://pvs-studio.com/en/pvs-studio/download/)\. Also, I suggest having a look at an [article](https://pvs-studio.com/en/blog/posts/0395/) that was recently written by my colleague\. It gives explanations of the reasons behind the limitations of a demo\-version, and what should be done to get the full functionality of the tool\. For those who were too lazy to read \- you can just [contact](https://pvs-studio.com/en/about-feedback/) us\. **P\.S\.** Besides that, there is a nice [error base](https://pvs-studio.com/en/blog/examples/) that we found in open source projects, and a [list of articles](https://pvs-studio.com/en/blog/posts/) \(open\-source projects check, technical details, etc\.\) I recommend checking it out\. ## Suspicious code fragments Let's start with the "classic" errors detected by V3001 diagnostic rule: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'RwWait' to the left and to the right of the '\|' operator\. SplitOrderedList\.cs 458 As we see in the code, an expression value is evaluated using bitwise operations\. At the same time, in one of the subexpressions _RwWait \| RwWait_, we have the same constant fields\. It does not make sense\. Also, the set of constants that are declared earlier have the values equal to the power of two number, consequently, they were meant to be used as flags \(this is what we see in the example with bitwise operations\)\. I think it would be more sensible to put them in an enumeration marked with \[Flags\] attribute; that would give a number of advantages when working with this enumeration \(see the [documentation for V3059](https://pvs-studio.com/en/docs/warnings/v3059/)\)\. Speaking about the current example \- we assume, that _RwWrite_ constant was supposed to be here\. This is one of the minuses of IntelliSense \- despite the fact that this tool is very helpful during the code development, at times it can "suggest" the wrong variable, which can lead to an error\. One more code example with a similar error\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'Left \=\= 0' to the left and to the right of the '&&' operator\. Thickness\.cs 29 The subexpression _Left \=\= 0 _is used twice in the expression\. Apparently, that's a mistake\. The code _Bottom \=\= 0_ should be used instead of the last subexpression, as it is the only property \(judging by the logic and the property set\) that is not checked in this expression\. The following error is peculiar due to the fact that it can be found in two files with similar names and partially similar code\. That's how bugs get multiplied \- there was an error in one place, then this code was copied to another location \- and presto\! \- Here is another buggy fragment\. ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3003/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 28, 30\. Xamarin\.Forms\.Platform\.Android SwitchRenderer\.cs 28 In this code fragment we see strange logic in the _if _statement\. Some condition \(_widthConstraint <\= 0_\) gets checked, and if its result is not true, this condition is checked again\. Is it a bug? Yep, it is\. It's not that easy to say how to fix it\. This task goes to the author of the code\. As I have said before, the same error was found in the file with the same name\. Here is the message issued by the analyzer: V3003 The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 26, 28\. Xamarin\.Forms\.Platform\.Android SwitchRenderer\.cs 26 Thanks to the mechanism of [virtual values](https://pvs-studio.com/en/blog/posts/0394/) we managed to improve several diagnostic rules, including the [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) diagnostic, which detects if the expression always evaluates to _true _or _false_\. Here are some examples that were detected by this diagnostic: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'genericParameter\.Owner\.GenericParameterType \=\= GenericParameterType\.Type' is always false\. ICSharpCode\.Decompiler TypesHierarchyHelpers\.cs 441 Even though I have deleted part of a method that is not of great interest to us, the error is still not very noticeable\. So I suggest simplifying the code, using shorter variable names: ```cpp if (a == enVal) return b; else return a == enVal ? c : d; ``` Now everything has become a little clearer\. The root of the problem \- the second check _a \=\= enVal \(genericParameter\.Owner\.GenericParameterType \=\= GenericParameterType\.Type\)_, that is located in the ternary operator\. A ternary operator in the _else_\-branch of the _if _statement makes no sense \- in this case the method will always return _d _value \(_UnresolvedGenericMethodParameter_\)\. If it's still not very clear \- let me give some explanations\. In the case where the program gets to the evaluation of a ternary operator, it's already known that the expression _a \=\= enVal_ is _false_, thus, it will have the same value in the ternary operator\. Outcome: the result of the ternary operator is always the same\. Well\.\.\. that's a bug\. It's hard to see these defects right away, even cutting off the redundant code from the method, the error remains in the other part of the code\. We had to make additional simplifications to detect this "pitfall"\. However, it's not a problem for the analyzer, as it coped quite easily with this task\. Of course, this is not the only case\. Here's another one: ```cpp TypeReference DoInferTypeForExpression(ILExpression expr, TypeReference expectedType, bool forceInferChildren = false) { .... if (forceInferChildren) { .... if (forceInferChildren) { InferTypeForExpression(expr.Arguments.Single(), lengthType); } } .... } ``` **PVS\-Studio warning:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'forceInferChildren' is always true\. ICSharpCode\.Decompiler TypeAnalysis\.cs 632 Again, to make it easier to spot the mistake, let's cut out the unnecessary code\. And here it is \- the condition _forceInferChildren_ gets checked twice; besides that, this variable isn't used in any way between the _if _statements\. If we take into account that this is a parameter of a method, we can conclude that neither other threads, nor any methods, can change it without direct accessing\. Thus, if the first _if_ statement is evaluated as true, the second will always be true as well\. Strange logic\. There is a diagnostic similar to the V3022 \- V3063\. This diagnostic rule determines if a part of the conditional expression is always _true _or _false_\. Thanks to this, we managed to find several interesting code fragments: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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_\. It is obvious that it will always be _true_\. Why do we need this check then? It's not clear\. By the way, there are no comments that could give a hint that the value can be changed during debugging \(like _// new Exception\(\);_\) These are not the only suspicious fragments found by the diagnostic rules V3022 and V3063\. But let's move on and see what else was found in this code\. ```cpp void WriteSecurityDeclarationArgument( CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } ``` **PVS\-Studio warning:** [V3038](https://pvs-studio.com/en/docs/warnings/v3038/) The first argument of 'Replace' function is equal to the second argument\. ICSharpCode\.Decompiler ReflectionDisassembler\.cs 349 In this code, we are interested in the _Replace_ method which is called for some string\. Apparently, the programmer wanted to replace all the single quote characters with a slash and quotation marks\. But the thing is, in the latter case, the slash character gets screened, that's why this method call replaces a single quote with a single quote too\. Any doubts? Try_ Equals\("'", "\\'"\)\. _It may not be really evident, but the analyzer is always alert\. We can use the @ symbol before the string literal, to avoid screening\. Then the correct _Replace_ method call will be the following: ```cpp Replace("'", @"\'") ``` There are also methods that always return the same values\. For example: ```cpp static bool Unprocessed(ICollection extra, Option def, OptionContext c, string argument) { if (def == null) { .... return false; } .... return false; } ``` **PVS\-Studio warning:** [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'false'\. Xamarin\.Forms\.UITest\.TestCloud OptionSet\.cs 239 Regardless of the arguments, and what is executed in this method, it always returns _false\._ You'd probably agree that it looks slightly weird\. By the way, this code was in another fragment \- the method was copied, and put in a different place\. The analyzer warning: V3009\. It's odd that this method always returns one and the same value of 'false'\. Xamarin\.Forms\.Xaml\.Xamlg Options\.cs 1020 There were several code fragments with a repeated exception generated, that may potentially have bugs\. ```cpp static async Task GetStreamAsync (Uri uri, CancellationToken cancellationToken) { try { await Task.Delay (5000, cancellationToken); } catch (TaskCanceledException ex) { cancelled = true; throw ex; } .... } ``` **PVS\-Studio warning:** [V3052](https://pvs-studio.com/en/docs/warnings/v3052/) The original exception object 'ex' was swallowed\. Stack of original exception could be lost\. Xamarin\.Forms\.Core\.UnitTests ImageTests\.cs 221 It could seem that the logic is simple\. In the case of an exception, we perform some actions, and then generate it again\. But the devil's in the details\. In this case, when the exception is re\-thrown, the stack of the original exception gets fully "lost"\. To avoid this, there is no need to throw the same exception, it would be enough to re\-throw the existing one, by calling the _throw_ operator_\. _Then the code of the _catch_ block will be like this: ```cpp cancelled = true; throw; ``` A similar example: ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3052/) The original exception object 'ae' was swallowed\. Stack of original exception could be lost\. Xamarin\.Forms\.Xaml RegisterXNamesVisitor\.cs 38 In both cases the information about the previous exception is lost\. We could suppose that in the second case the information won't really be relevant \(although it is still strange\), in the first case the programmer intended to locate this exception earlier, but instead, a new one was generated\. The solution is the same as in the previous example \- call the _throw_ operator without arguments\. Speaking about the following fragment \- it's hard to say for sure if it is an error or not, but it looks at least strange\. ```cpp void UpdateTitle() { if (Element?.Detail == null) return; ((ITitleProvider)this).Title = (Element.Detail as NavigationPage) ?.CurrentPage?.Title ?? Element.Title ?? Element?.Title; } ``` **PVS\-Studio warning:** [V3042](https://pvs-studio.com/en/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the Element object Xamarin\.Forms\.Platform\.WinRT MasterDetailPageRenderer\.cs 288 The analyzer was suspicious about the fact that the accessing the _Title_ property is done in different ways \- _Element\.Title_ and _Element?\.Title_ at that the addressing is first done directly, and then \- using a null\-conditional operator\. But everything is not so simple\. As you may have noticed, in the beginning of the method there is a check, _Element?\.Detail \=\= null_, which supposes that if the _Element_ _\=\=_ _null,_ then the method will exit here, and there will be no other operations\. At the same time, the expression _Element?_\._Title_ implies that at the time of its execution, the _Element_ can be _null_\. If it is so, then on the previous stage at the time of accessing the _Title_ property directly, we will have the exception of _NullReferenceException_ generated, and therefore there is no use in the null\-conditional operator\. In any case, this code looks very strange, and it needs to be fixed\. It was also strange that an object was cast to its own type\. Here is an example: ```cpp public FormsPivot Control { get; private set; } Brush ITitleProvider.BarBackgroundBrush { set { (Control as FormsPivot).ToolbarBackground = value; } } ``` **PVS\-Studio warning:** [V3051](https://pvs-studio.com/en/docs/warnings/v3051/) An excessive type cast\. The object is already of the 'FormsPivot' type\. Xamarin\.Forms\.Platform\.UAP TabbedPageRenderer\.cs 73 In this case, it's not a bug, but this code looks at least suspicious, taking into consideration that _Control _object already has a _FormsPivot_ type\. By the way, is not the only warning of this kind, there were many others: * 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: ```cpp public override void LayoutSubviews() { .... if (_scroller == null || (_scroller != null && _scroller.Frame == Bounds)) return; .... } ``` **PVS\-Studio warning:** [V3031](https://pvs-studio.com/en/docs/warnings/v3031/) An excessive check 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 subexpression _\_scroller\! \= null\._ It will be evaluated only if the expression to the left of the '\|\|' operator, _\_scroller \=\= null_ is false, consequently, _\_scroller_ isn't null, so, we can be not afraid to get _NullReferenceException\._ Then the simplified code will be like this: ```cpp if (_scroller == null || _scroller.Frame == Bounds)) ``` ## Drawbacks of the performed analysis Unfortunately, we didn't manage to compile the entire solution \- 6 projects remained unchecked and those fragments, where the classes were used, weren't analyzed as thoroughly as they could have been\. Perhaps we might have found something else of interest to us\. By the way, you can see if there are any problems with the analysis by taking a look at the level three message, [V051](https://pvs-studio.com/en/docs/warnings/v051/)\. If you have such warnings, it's usually a signal that the C\# project has some compilation bugs, because of which it cannot get the full information necessary for the in\-depth analysis\. Nevertheless, it will try to do the checks that do not require detailed information about the types and objects\. It's advisable to make sure that you don't have any V051 warnings during the project check\. If they are there \- try to get rid of them \(check if the project is compiled, make sure that all the dependencies are uploaded\) ## Conclusion ![0400_XamarinForms/image5.png](https://import.viva64.com/docx/blog/0400_XamarinForms/image5.png) The check of Xamarin\.Forms was quite rewarding \- we found several interesting fragments; some were really erroneous, some \- suspicious and strange\. My hope is that the developers will notice the article, and fix the issues that we have discussed here\. You can see all the suspicious code fragments by downloading a [trial version](https://pvs-studio.com/en/pvs-studio/download/) of the analyzer\. The best solution would be to implement PVS\-Studio and use it regularly, which will allow the detection of errors during the early stages of development\.