Microsoft opened the source code of Xamarin.Forms. We couldn't miss a chance to check it with PVS-Studio

• The project to be analyzed
• Analysis tool
• Suspicious code fragments
• Drawbacks of the performed analysis
• Conclusion

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.

The project to be analyzed

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.

The code of the framework is written in C# and is available at a repository on GitHub.

Analysis tool

The project was checked using the 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.

Each diagnostic rule has documentation, 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. Also, I suggest having a look at an article 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 us.

P.S. Besides that, there is a nice error base that we found in open source projects, and a list of articles (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:

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' 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).

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.

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 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.

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 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 we managed to improve several diagnostic rules, including the V3022 diagnostic, which detects if the expression always evaluates to true or false. Here are some examples that were detected by this diagnostic:

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

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:

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:

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, 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:

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. 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.

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

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:

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

There are also methods that always return the same values. 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 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.

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 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:

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 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.

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 '.' 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:

public FormsPivot Control { get; private set; }

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

PVS-Studio warning: 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:

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

PVS-Studio warning: 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:

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. 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

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 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.