Microsoft hat den Quellcode von Xamarin.Forms geöffnet. Wir konnten es nicht versäumen, dies mit PVS-Studio zu überprüfen

Microsoft hat den Quellcode von Xamarin.Forms geöffnet. Wir konnten es nicht versäumen, dies mit PVS-Studio zu überprüfen

Sie wissen wahrscheinlich bereits, dass die Microsoft Corporation die Xamarin Company gekauft hat. Auch wenn Microsoft damit begonnen hat, den Quellcode einiger seiner Produkte schrittweise zu öffnen, war der Xamarin.Forms-Code eine große Überraschung. Ich konnte es nicht genehmigen und beschloss, den Code mit einem statischen Code-Analysator zu überprüfen.

Das zu analysierende Projekt

Xamarin.Forms ist eine plattformübergreifende, nativ unterstützte UI-Toolkit-Abstraktion, mit der Entwickler auf einfache Weise Benutzeroberflächen erstellen können, die von Android, iOS, Windows und Windows Phone gemeinsam genutzt werden können. Die Benutzeroberflächen werden mithilfe der nativen Steuerelemente der Zielplattform gerendert, sodass Xamarin.Forms-Anwendungen das entsprechende Erscheinungsbild für jede Plattform beibehalten können. Sie können Code oder Markup verwenden, um eine Benutzeroberfläche mit Datenbindung und Stilen zu erstellen, indem Sie entweder C#- oder XAML-Markup verwenden.

Der Code des Frameworks ist in C# geschrieben und in einem Repository auf GitHub verfügbar.

Analysetool

Das Projekt wurde mit dem statischen Codeanalysator PVS-Studio überprüft; an deren Entwicklung ich mich aktiv beteilige. Wir arbeiten ständig an seiner Verbesserung, modifizieren und fügen neue Diagnoseregeln hinzu. Aus diesem Grund können wir mit jeder neuen Projektprüfung eine größere Vielfalt an Fehlern finden.

Jede Diagnoseregel verfügt über eine Dokumentation, die eine Beschreibung des Fehlers sowie Beispiele für den falschen und richtigen Code enthält. Die Testversion des Analysators kann hier heruntergeladen werden. Außerdem schlage ich vor, einen Artikel anzusehen, der kürzlich von meinem Kollegen geschrieben wurde. Es gibt Erläuterungen zu den Gründen für die Einschränkungen einer Demoversion und was getan werden sollte, um die volle Funktionalität des Tools zu erhalten. Wer zu faul zum Lesen war, kann sich einfach bei uns melden.

P.S. Außerdem gibt es eine nette Fehlerbasis, die wir in Open-Source-Projekten gefunden haben, und eine Liste von Artikeln (Open-Source-Projektüberprüfung, technische Details usw.), die ich zum Auschecken empfehle.

Verdächtige Codefragmente

Beginnen wir mit den "klassischen" Fehlern, die von der V3001-Diagnoseregel erkannt werden:

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-Warnung: V3001 Es gibt identische Unterausdrücke 'RwWait' links und rechts vom '|' Operator. SplitOrderedList.cs 458

Wie wir im Code sehen, wird ein Ausdruckswert mithilfe von bitweisen Operationen ausgewertet. Gleichzeitig wird in einem der Unterausdrücke RwWait | RwWait , haben wir die gleichen konstanten Felder. Es ergibt keinen Sinn. Außerdem hat der Satz von Konstanten, die zuvor deklariert wurden, die Werte gleich der Zweierpotenz, folglich sollten sie als Flags verwendet werden (das sehen wir im Beispiel mit bitweisen Operationen). Ich denke, es wäre sinnvoller, sie in eine Aufzählung zu setzen, die mit dem Attribut [Flags] gekennzeichnet ist. das würde eine Reihe von Vorteilen beim Arbeiten mit dieser Aufzählung bringen (siehe die Dokumentation für V3059).

Apropos aktuelles Beispiel - wir gehen davon aus, dass RwWrite Konstante sollte hier sein. Dies ist einer der Minuspunkte von IntelliSense – obwohl dieses Tool während der Codeentwicklung sehr hilfreich ist, kann es manchmal die falsche Variable „vorschlagen“, was zu einem Fehler führen kann.

Ein weiteres Codebeispiel mit einem ähnlichen Fehler.

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-Warnung: V3001 Es gibt identische Teilausdrücke 'Left ==0' Links und rechts vom Operator '&&'. Dicke.cs 29

Der Unterausdruck Left ==0 wird im Ausdruck zweimal verwendet. Anscheinend ist das ein Fehler. Der Code Unten ==0 sollte anstelle des letzten Teilausdrucks verwendet werden, da dies die einzige Eigenschaft ist (der Logik und dem Eigenschaftssatz nach zu urteilen), die in diesem Ausdruck nicht überprüft wird.

Der folgende Fehler ist eigenartig, da er in zwei Dateien mit ähnlichen Namen und teilweise ähnlichem Code zu finden ist. So vervielfachen sich Bugs - an einer Stelle war ein Fehler, dann wurde dieser Code an eine andere Stelle kopiert - und schwupps! - Hier ist ein weiteres fehlerhaftes 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-Warnung: V3003 Die Verwendung des Musters „if (A) {...} else if (A) {...}“ wurde erkannt. Es besteht die Wahrscheinlichkeit des Vorliegens eines logischen Fehlers. Überprüfen Sie die Zeilen:28, 30. Xamarin.Forms.Platform.Android SwitchRenderer.cs 28

In diesem Codefragment sehen wir eine seltsame Logik im if Aussage. Einige Bedingungen (widthConstraint <=0 ) überprüft, und wenn das Ergebnis nicht wahr ist, wird diese Bedingung erneut überprüft. Ist es ein Fehler? Ja ist es. Es ist nicht so einfach zu sagen, wie man es repariert. Diese Aufgabe geht an den Autor des Codes.

Wie ich bereits sagte, wurde derselbe Fehler in der Datei mit demselben Namen gefunden. Hier ist die vom Analysator ausgegebene Meldung:V3003 Die Verwendung des Musters „if (A) {...} else if (A) {...}“ wurde erkannt. Es besteht die Wahrscheinlichkeit des Vorliegens eines logischen Fehlers. Überprüfen Sie die Zeilen:26, 28. Xamarin.Forms.Platform.Android SwitchRenderer.cs 26

Dank des Mechanismus der virtuellen Werte konnten wir mehrere Diagnoseregeln verbessern, einschließlich der V3022-Diagnose, die erkennt, ob der Ausdruck immer true ergibt oder falsch . Hier sind einige Beispiele, die von dieser Diagnose erkannt wurden:

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-Warnung: V3022 Ausdruck 'genericParameter.Owner.GenericParameterType ==GenericParameterType.Type' ist immer falsch. ICSharpCode.Decompiler TypesHierarchyHelpers.cs 441

Auch wenn ich einen Teil einer für uns nicht so interessanten Methode gelöscht habe, fällt der Fehler dennoch nicht sehr auf. Daher schlage ich vor, den Code zu vereinfachen und kürzere Variablennamen zu verwenden:

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

Jetzt ist alles etwas klarer geworden. Die Wurzel des Problems – die zweite Prüfung a ==enVal (genericParameter.Owner.GenericParameterType ==GenericParameterType.Type) , die sich im ternären Operator befindet. Ein ternärer Operator im else -Zweig des if -Anweisung macht keinen Sinn - in diesem Fall gibt die Methode immer d zurück Wert (UnresolvedGenericMethodParameter ).

Wenn es immer noch nicht ganz klar ist – lassen Sie mich einige Erklärungen geben. Für den Fall, dass das Programm zur Auswertung eines ternären Operators gelangt, ist bereits bekannt, dass der Ausdruck a ==enVal ist falsch , daher hat es im ternären Operator denselben Wert. Ergebnis:Das Ergebnis des ternären Operators ist immer gleich. Nun ... das ist ein Fehler.

Es ist schwierig, diese Fehler sofort zu erkennen, selbst wenn der redundante Code von der Methode abgeschnitten wird, bleibt der Fehler im anderen Teil des Codes. Wir mussten zusätzliche Vereinfachungen vornehmen, um diese „Fallgrube“ zu erkennen. Für den Analysator ist dies jedoch kein Problem, da er diese Aufgabe problemlos bewältigt.

Natürlich ist dies nicht der einzige Fall. Hier ist noch einer:

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

PVS-Studio-Warnung: V3022 Ausdruck 'forceInferChildren' ist immer wahr. ICSharpCode.Decompiler TypeAnalysis.cs 632

Um es einfacher zu machen, den Fehler zu finden, lassen Sie uns wieder den unnötigen Code herausschneiden. Und hier ist sie - die Bedingung forceInferChildren wird zweimal überprüft; außerdem wird diese Variable in keiner Weise zwischen if verwendet Aussagen. Wenn wir berücksichtigen, dass dies ein Parameter einer Methode ist, können wir schlussfolgern, dass weder andere Threads noch irgendwelche Methoden ihn ohne direkten Zugriff ändern können. Wenn also das erste if Anweisung als wahr ausgewertet wird, wird die zweite immer ebenfalls wahr sein. Seltsame Logik.

Es gibt eine ähnliche Diagnose wie bei V3022 - V3063. Diese Diagnoseregel bestimmt, ob ein Teil des bedingten Ausdrucks immer wahr ist oder falsch . Dadurch ist es uns gelungen, mehrere interessante Codefragmente zu finden:

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-Warnung: V3063 Ein Teil des Bedingungsausdrucks ist immer wahr:Ausnahme ==null. Xamarin.Forms.Xaml ApplyPropertiesVisitor.cs 280

Uns interessiert der Unterausdruck Exception ==null . Es ist offensichtlich, dass es immer wahr sein wird . Wozu brauchen wir dann diesen Check? Es ist nicht klar. Übrigens gibt es keine Kommentare, die darauf hindeuten könnten, dass der Wert beim Debuggen geändert werden kann (wie // new Exception(); )

Dies sind nicht die einzigen verdächtigen Fragmente, die von den Diagnoseregeln V3022 und V3063 gefunden werden. Aber lassen Sie uns weitermachen und sehen, was sonst noch in diesem Code gefunden wurde.

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

PVS-Studio-Warnung: V3038 Das erste Argument der Funktion „Replace“ ist gleich dem zweiten Argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

In diesem Code interessiert uns das Replace Methode, die für einen String aufgerufen wird. Anscheinend wollte der Programmierer alle einfachen Anführungszeichen durch einen Schrägstrich und Anführungszeichen ersetzen. Aber die Sache ist, dass im letzteren Fall der Schrägstrich ausgeblendet wird, deshalb ersetzt dieser Methodenaufruf auch ein einfaches Anführungszeichen durch ein einfaches Anführungszeichen. Irgendwelche Zweifel? Versuchen Sie Equals("'", "\'"). Es mag nicht wirklich offensichtlich sein, aber der Analysator ist immer wachsam. Wir können das @-Symbol vor dem String-Literal verwenden, um eine Überprüfung zu vermeiden. Dann das richtige Ersetzen Der Methodenaufruf lautet wie folgt:

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

Es gibt auch Methoden, die immer die gleichen Werte zurückgeben. Zum Beispiel:

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

PVS-Studio-Warnung: V3009 Merkwürdig ist, dass diese Methode immer ein und denselben Wert „false“ zurückgibt. Xamarin.Forms.UITest.TestCloud OptionSet.cs 239

Unabhängig von den Argumenten und was in dieser Methode ausgeführt wird, gibt sie immer false. zurück Sie würden wahrscheinlich zustimmen, dass es etwas seltsam aussieht.

Übrigens befand sich dieser Code in einem anderen Fragment - die Methode wurde kopiert und an einer anderen Stelle abgelegt. Die Analysatorwarnung:V3009. Merkwürdig ist, dass diese Methode immer ein und denselben Wert „false“ zurückgibt. Xamarin.Forms.Xaml.Xamlg Options.cs 1020

Es wurden mehrere Codefragmente mit einer wiederholten Ausnahme generiert, die möglicherweise Fehler enthalten.

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

  ....
}

PVS-Studio-Warnung: V3052 Das ursprüngliche Ausnahmeobjekt 'ex' wurde verschluckt. Stapel der ursprünglichen Ausnahme könnte verloren gehen. Xamarin.Forms.Core.UnitTests ImageTests.cs 221

Es könnte scheinen, dass die Logik einfach ist. Im Falle einer Ausnahme führen wir einige Aktionen aus und generieren sie dann erneut. Aber der Teufel steckt im Detail. Wenn in diesem Fall die Ausnahme erneut ausgelöst wird, geht der Stack der ursprünglichen Ausnahme vollständig "verloren". Um dies zu vermeiden, muss nicht dieselbe Ausnahme ausgelöst werden, es würde ausreichen, die vorhandene erneut auszulösen, indem Sie throw aufrufen Betreiber. Dann der Code des Fangs Block sieht so aus:

cancelled = true;
throw;

Ein ähnliches Beispiel:

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-Warnung: V3052 Das ursprüngliche Ausnahmeobjekt „ae“ wurde verschluckt. Stapel der ursprünglichen Ausnahme könnte verloren gehen. Xamarin.Forms.Xaml RegisterXNamesVisitor.cs 38

In beiden Fällen geht die Information über die vorherige Ausnahme verloren. Wir könnten annehmen, dass im zweiten Fall die Informationen nicht wirklich relevant sind (obwohl es immer noch seltsam ist), im ersten Fall wollte der Programmierer diese Ausnahme früher lokalisieren, aber stattdessen wurde eine neue generiert. Die Lösung ist die gleiche wie im vorherigen Beispiel - rufen Sie throw auf Operator ohne Argumente.

Apropos folgendes Fragment - es ist schwer zu sagen, ob es sich um einen Fehler handelt oder nicht, aber es sieht zumindest seltsam aus.

void UpdateTitle()
{
  if (Element?.Detail == null)
    return;

   ((ITitleProvider)this).Title = (Element.Detail as NavigationPage)
                                   ?.CurrentPage?.Title 
                                   ?? Element.Title ?? Element?.Title;
}

PVS-Studio-Warnung: V3042 Mögliche NullReferenceException. Das '?.' und '.' Operatoren werden für den Zugriff auf Elemente des Elementobjekts Xamarin.Forms.Platform.WinRT MasterDetailPageRenderer.cs 288

verwendet

Der Analysator war misstrauisch in Bezug auf die Tatsache, dass der Zugriff auf Title -Eigenschaft wird auf unterschiedliche Weise ausgeführt - Element.Title und Element?.Titel Dabei erfolgt die Adressierung zuerst direkt und dann - unter Verwendung eines nullbedingten Operators. Aber alles ist nicht so einfach.

Wie Sie vielleicht bemerkt haben, gibt es am Anfang der Methode eine Prüfung, Element?.Detail ==null , was voraussetzt, dass wenn das Element == Null, dann wird die Methode hier beendet und es gibt keine weiteren Operationen.

Gleichzeitig wird der Ausdruck Element? .Titel impliziert, dass zum Zeitpunkt seiner Ausführung das Element kann null sein . Wenn ja, dann auf der vorherigen Stufe zum Zeitpunkt des Zugriffs auf den Titel -Eigenschaft direkt, haben wir die Ausnahme von NullReferenceException generiert, und daher wird der nullbedingte Operator nicht verwendet.

Auf jeden Fall sieht dieser Code sehr seltsam aus und muss behoben werden.

Es war auch seltsam, dass ein Objekt in seinen eigenen Typ umgewandelt wurde. Hier ist ein Beispiel:

public FormsPivot Control { get; private set; }

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

PVS-Studio-Warnung: V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 73

In diesem Fall handelt es sich nicht um einen Fehler, aber dieser Code sieht zumindest verdächtig aus, wenn man bedenkt, dass Control Objekt hat bereits einen FormsPivot Typ. Übrigens ist das nicht die einzige Warnung dieser Art, es gab noch viele andere:

  • V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 78
  • V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 282
  • V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 175
  • V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 197
  • V3051 Eine übermäßige Typumwandlung. Das Objekt ist bereits vom Typ 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 205

Es gibt Bedingungen, die vereinfacht werden könnten. Ein Beispiel für eines davon:

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

PVS-Studio-Warnung: V3031 Eine übermäßige Prüfung kann vereinfacht werden. Das '||' Der Operator ist von entgegengesetzten Ausdrücken umgeben. Xamarin.Forms.Platform.iOS.Classic ContextActionCell.cs 102

Dieser Ausdruck kann vereinfacht werden, indem der Unterausdruck _scroller entfernt wird! =null. Es wird nur ausgewertet, wenn der Ausdruck links vom '||' Operator, _scroller ==null ist falsch, folglich _scroller ist nicht null, also können wir keine Angst haben, NullReferenceException. zu erhalten Dann sieht der vereinfachte Code so aus:

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

Nachteile der durchgeführten Analyse

Leider haben wir es nicht geschafft, die gesamte Lösung zu kompilieren - 6 Projekte blieben ungeprüft und die Fragmente, in denen die Klassen verwendet wurden, wurden nicht so gründlich analysiert, wie sie hätten sein können. Vielleicht haben wir etwas anderes Interessantes für uns gefunden.

Ob es Probleme mit der Analyse gibt, können Sie übrigens anhand der Level-3-Meldung V051 erkennen. Wenn Sie solche Warnungen erhalten, ist dies normalerweise ein Signal dafür, dass das C#-Projekt einige Kompilierungsfehler aufweist, aufgrund derer es nicht alle Informationen erhalten kann, die für die eingehende Analyse erforderlich sind. Trotzdem wird es versuchen, die Prüfungen durchzuführen, die keine detaillierten Informationen über die Typen und Objekte erfordern.

Es ist ratsam, während der Projektprüfung darauf zu achten, dass keine V051-Warnungen vorliegen. Wenn sie da sind - versuchen Sie, sie loszuwerden (überprüfen Sie, ob das Projekt kompiliert ist, stellen Sie sicher, dass alle Abhängigkeiten hochgeladen wurden)

Schlussfolgerung

Die Überprüfung von Xamarin.Forms war sehr lohnend - wir fanden mehrere interessante Fragmente; einige waren wirklich falsch, einige - verdächtig und seltsam. Ich hoffe, dass die Entwickler den Artikel bemerken und die Probleme beheben, die wir hier besprochen haben. Sie können alle verdächtigen Codefragmente sehen, indem Sie eine Testversion des Analyseprogramms herunterladen. Die beste Lösung wäre, PVS-Studio zu implementieren und regelmäßig zu verwenden, wodurch Fehler in den frühen Phasen der Entwicklung erkannt werden können.