Probabilmente sai già che Microsoft Corporation ha acquistato la Xamarin Company. Anche se Microsoft ha iniziato ad aprire gradualmente il codice sorgente di alcuni dei suoi prodotti, il codice Xamarin.Forms è stata una grande sorpresa. Non ho potuto dargli il via libera e ho deciso di controllare il codice utilizzando un analizzatore di codice statico.
Il progetto da analizzare
Xamarin.Forms è un'astrazione del toolkit dell'interfaccia utente supportata in modo nativo multipiattaforma che consente agli sviluppatori di creare facilmente interfacce utente che possono essere condivise su Android, iOS, Windows e Windows Phone. Il rendering delle interfacce utente viene eseguito utilizzando i controlli nativi della piattaforma di destinazione, consentendo alle applicazioni Xamarin.Forms di mantenere l'aspetto appropriato per ogni piattaforma. Puoi utilizzare codice o markup per creare un'interfaccia utente con associazione dati e stili, utilizzando il markup C# o XAML.
Il codice del framework è scritto in C# ed è disponibile in un repository su GitHub.
Strumento di analisi
Il progetto è stato verificato utilizzando l'analizzatore di codice statico PVS-Studio; nel cui sviluppo prendo parte attiva. Lavoriamo costantemente al suo miglioramento, modificando e aggiungendo nuove regole diagnostiche. Questo è il motivo per cui con ogni nuovo controllo del progetto siamo in grado di trovare una più ampia varietà di bug.
Ciascuna regola diagnostica dispone di documentazione, che include una descrizione dell'errore, nonché esempi del codice errato e corretto. La versione di prova dell'analizzatore può essere scaricata qui. Inoltre, suggerisco di dare un'occhiata a un articolo che è stato scritto di recente dal mio collega. Fornisce spiegazioni sui motivi alla base dei limiti di una versione demo e su cosa dovrebbe essere fatto per ottenere la piena funzionalità dello strumento. Per coloro che erano troppo pigri per leggere, puoi semplicemente contattarci.
PS Oltre a ciò, c'è una bella base di errori che abbiamo trovato nei progetti open source e un elenco di articoli (controllo dei progetti open source, dettagli tecnici, ecc.) Consiglio di verificarlo.
Frammenti di codice sospetti
Cominciamo con gli errori "classici" rilevati dalla regola diagnostica 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;
....
}Avviso di PVS-Studio: V3001 Sono presenti sottoespressioni identiche 'RwWait' a sinistra ea destra di '|' operatore. SplitOrderedList.cs 458
Come si vede nel codice, un valore di espressione viene valutato utilizzando operazioni bit per bit. Allo stesso tempo, in una delle sottoespressioni RwWait | RwAspetta , abbiamo gli stessi campi costanti. Non ha senso. Inoltre, l'insieme di costanti dichiarate in precedenza ha i valori uguali alla potenza di due numeri, di conseguenza, dovevano essere usate come flag (questo è ciò che vediamo nell'esempio con operazioni bit per bit). Penso che sarebbe più sensato inserirli in un'enumerazione contrassegnata con l'attributo [Flags]; ciò darebbe una serie di vantaggi quando si lavora con questa enumerazione (consultare la documentazione per V3059).
Parlando dell'esempio attuale, assumiamo che RwWrite costante doveva essere qui. Questo è uno degli svantaggi di IntelliSense:nonostante questo strumento sia molto utile durante lo sviluppo del codice, a volte può "suggerire" la variabile sbagliata, che può portare a un errore.
Un altro esempio di codice con un errore simile.
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; }
}Avviso di PVS-Studio: V3001 Esistono sottoespressioni identiche 'Left ==0' a sinistra ea destra dell'operatore '&&'. Spessore.cs 29
La sottoespressione Sinistra ==0 è usato due volte nell'espressione. A quanto pare, è un errore. Il codice Bottom ==0 dovrebbe essere utilizzata al posto dell'ultima sottoespressione, poiché è l'unica proprietà (a giudicare dalla logica e dall'insieme di proprietà) che non è selezionata in questa espressione.
Il seguente errore è peculiare per il fatto che può essere trovato in due file con nomi simili e codice parzialmente simile. È così che i bug vengono moltiplicati:si è verificato un errore in un punto, quindi questo codice è stato copiato in un'altra posizione e presto! - Ecco un altro frammento di bug.
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;
....
}Avviso di PVS-Studio: V3003 È stato rilevato l'utilizzo del pattern 'if (A) {...} else if (A) {...}'. C'è una probabilità di presenza di un errore logico. Righe di controllo:28, 30. Xamarin.Forms.Platform.Android SwitchRenderer.cs 28
In questo frammento di codice vediamo una strana logica nel if dichiarazione. Alcune condizioni (widthConstraint <=0 ) viene verificata e, se il risultato non è true, questa condizione viene nuovamente verificata. E 'un errore? Sì, lo è. Non è così facile dire come risolverlo. Questa attività va all'autore del codice.
Come ho detto prima, lo stesso errore è stato trovato nel file con lo stesso nome. Ecco il messaggio emesso dall'analizzatore:V3003 È stato rilevato l'uso del pattern 'if (A) {...} else if (A) {...}'. C'è una probabilità di presenza di un errore logico. Righe di controllo:26, 28. Xamarin.Forms.Platform.Android SwitchRenderer.cs 26
Grazie al meccanismo dei valori virtuali siamo riusciti a migliorare diverse regole diagnostiche, inclusa la diagnostica V3022, che rileva se l'espressione restituisce sempre true o falso . Ecco alcuni esempi che sono stati rilevati da questa diagnostica:
public TypeReference ResolveWithContext(TypeReference type)
{
....
if (genericParameter.Owner.GenericParameterType ==
GenericParameterType.Type)
return TypeArguments[genericParameter.Position];
else
return genericParameter.Owner.GenericParameterType
== GenericParameterType.Type
? UnresolvedGenericTypeParameter :
UnresolvedGenericMethodParameter;
....
}Avviso di PVS-Studio: V3022 L'espressione 'genericParameter.Owner.GenericParameterType ==GenericParameterType.Type' è sempre falsa. ICSharpCode.Decompiler TypesHierarchyHelpers.cs 441
Anche se ho cancellato parte di un metodo che non ci interessa molto, l'errore non è ancora molto evidente. Quindi suggerisco di semplificare il codice, utilizzando nomi di variabili più brevi:
if (a == enVal)
return b;
else
return a == enVal ? c : d;Ora tutto è diventato un po' più chiaro. La radice del problema:il secondo controllo a ==enVal (genericParameter.Owner.GenericParameterType ==GenericParameterType.Type) , che si trova nell'operatore ternario. Un operatore ternario in else -ramo dell'se istruzione non ha senso - in questo caso il metodo restituirà sempre d valore (UnresolvedGenericMethodParameter ).
Se non è ancora molto chiaro, lascia che ti dia alcune spiegazioni. Nel caso in cui il programma arrivi alla valutazione di un operatore ternario, è già noto che l'espressione a ==enVal è falso , quindi, avrà lo stesso valore nell'operatore ternario. Risultato:il risultato dell'operatore ternario è sempre lo stesso. Beh... questo è un bug.
È difficile vedere subito questi difetti, anche tagliando il codice ridondante dal metodo, l'errore rimane nell'altra parte del codice. Abbiamo dovuto fare ulteriori semplificazioni per rilevare questa "trappola". Tuttavia, non è un problema per l'analizzatore, poiché ha affrontato abbastanza facilmente questo compito.
Naturalmente, questo non è l'unico caso. Eccone un altro:
TypeReference DoInferTypeForExpression(ILExpression expr,
TypeReference expectedType,
bool forceInferChildren =
false)
{
....
if (forceInferChildren) {
....
if (forceInferChildren) {
InferTypeForExpression(expr.Arguments.Single(), lengthType);
}
}
....
}Avviso di PVS-Studio: V3022 L'espressione 'forceInferChildren' è sempre vera. ICSharpCode.Decompiler TypeAnalysis.cs 632
Ancora una volta, per rendere più facile individuare l'errore, tagliamo il codice non necessario. Ed eccola qui:la condizione forceInferChildren viene controllato due volte; oltre a ciò, questa variabile non viene utilizzata in alcun modo tra if dichiarazioni. Se prendiamo in considerazione che questo è un parametro di un metodo, possiamo concludere che né altri thread, né alcun metodo, può modificarlo senza accedervi direttamente. Quindi, se il primo se affermazione è valutata come vera, anche la seconda sarà sempre vera. Strana logica.
Esiste una diagnostica simile a V3022 - V3063. Questa regola diagnostica determina se una parte dell'espressione condizionale è sempre vera o falso . Grazie a ciò, siamo riusciti a trovare diversi frammenti di codice interessanti:
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);
}
....
}Avviso di PVS-Studio: V3063 Una parte dell'espressione condizionale è sempre vera:eccezione ==null. Xamarin.Forms.Xaml ApplyPropertiesVisitor.cs 280
Siamo interessati alla sottoespressione exception ==null . È ovvio che sarà sempre vero . Perché abbiamo bisogno di questo controllo allora? Non è chiaro. A proposito, non ci sono commenti che potrebbero suggerire che il valore può essere modificato durante il debug (come // new Exception(); )
Questi non sono gli unici frammenti sospetti trovati dalle regole diagnostiche V3022 e V3063. Ma andiamo avanti e vediamo cos'altro è stato trovato in questo codice.
void WriteSecurityDeclarationArgument(
CustomAttributeNamedArgument na)
{
....
output.Write("string('{0}')",
NRefactory.CSharp
.TextWriterTokenWriter
.ConvertString(
(string)na.Argument.Value).Replace("'", "\'"));
....
}Avviso di PVS-Studio: V3038 Il primo argomento della funzione 'Sostituisci' è uguale al secondo argomento. ICSharpCode.Decompiler ReflectionDisassembler.cs 349
In questo codice, siamo interessati a Sostituisci metodo che viene chiamato per una stringa. Apparentemente, il programmatore voleva sostituire tutte le virgolette singole con una barra e le virgolette. Ma il fatto è che, in quest'ultimo caso, il carattere slash viene schermato, ecco perché questa chiamata al metodo sostituisce anche una singola virgoletta con una singola virgoletta. Nessun dubbio? Prova Uguale("'", "\'"). Potrebbe non essere molto evidente, ma l'analizzatore è sempre all'erta. Possiamo usare il simbolo @ prima della stringa letterale, per evitare lo screening. Quindi il corretto Sostituisci la chiamata al metodo sarà la seguente:
Replace("'", @"\'")Esistono anche metodi che restituiscono sempre gli stessi valori. Ad esempio:
static bool Unprocessed(ICollection<string> extra, Option def,
OptionContext c, string argument)
{
if (def == null)
{
....
return false;
}
....
return false;
}Avviso di PVS-Studio: V3009 È strano che questo metodo restituisca sempre lo stesso valore di 'false'. Xamarin.Forms.UITest.TestCloud OptionSet.cs 239
Indipendentemente dagli argomenti e da ciò che viene eseguito in questo metodo, restituisce sempre false. Probabilmente saresti d'accordo sul fatto che sembra leggermente strano.
A proposito, questo codice era in un altro frammento:il metodo è stato copiato e messo in un posto diverso. L'avviso dell'analizzatore:V3009. È strano che questo metodo restituisca sempre lo stesso valore di 'false'. Xamarin.Forms.Xaml.Xamlg Options.cs 1020
C'erano diversi frammenti di codice con un'eccezione ripetuta generata, che potevano potenzialmente avere dei bug.
static async Task<Stream>
GetStreamAsync (Uri uri, CancellationToken cancellationToken)
{
try {
await Task.Delay (5000, cancellationToken);
} catch (TaskCanceledException ex) {
cancelled = true;
throw ex;
}
....
}Avviso di PVS-Studio: V3052 L'oggetto eccezione originale 'ex' è stato ingoiato. Lo stack dell'eccezione originale potrebbe essere perso. Xamarin.Forms.Core.UnitTests ImageTests.cs 221
Potrebbe sembrare che la logica sia semplice. Nel caso di un'eccezione, eseguiamo alcune azioni, quindi la generiamo di nuovo. Ma il diavolo è nei dettagli. In questo caso, quando l'eccezione viene generata nuovamente, lo stack dell'eccezione originale viene completamente "perso". Per evitare ciò, non è necessario lanciare la stessa eccezione, basterebbe rilanciare quella esistente, chiamando il throw operatore . Quindi il codice della cattura il blocco sarà così:
cancelled = true;
throw;Un esempio simile:
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);
}
}Avviso di PVS-Studio: V3052 L'oggetto eccezione originale 'ae' è stato ingoiato. Lo stack dell'eccezione originale potrebbe essere perso. Xamarin.Forms.Xaml RegisterXNamesVisitor.cs 38
In entrambi i casi le informazioni sull'eccezione precedente vengono perse. Si potrebbe supporre che nel secondo caso l'informazione non sarà realmente rilevante (sebbene sia ancora strano), nel primo caso il programmatore intendeva localizzare questa eccezione prima, ma invece ne è stata generata una nuova. La soluzione è la stessa dell'esempio precedente:chiama il lancio operatore senza argomenti.
Parlando del seguente frammento, è difficile dire con certezza se si tratti di un errore o meno, ma sembra almeno strano.
void UpdateTitle()
{
if (Element?.Detail == null)
return;
((ITitleProvider)this).Title = (Element.Detail as NavigationPage)
?.CurrentPage?.Title
?? Element.Title ?? Element?.Title;
}Avviso di PVS-Studio: V3042 Possibile NullReferenceException. Il '?.' e '.' gli operatori vengono usati per accedere ai membri dell'oggetto Element Xamarin.Forms.Platform.WinRT MasterDetailPageRenderer.cs 288
L'analizzatore era sospettoso del fatto che l'accesso al Titolo la proprietà viene eseguita in diversi modi:Element.Title e Elemento?.Titolo a questo punto l'indirizzamento viene prima eseguito direttamente, quindi - utilizzando un operatore condizionale nullo. Ma non tutto è così semplice.
Come avrai notato, all'inizio del metodo c'è un segno di spunta, Element?.Detail ==null , il che presuppone che se l'Elemento == nulla, quindi il metodo uscirà da qui e non ci saranno altre operazioni.
Allo stesso tempo, l'espressione Elemento? .Titolo implica che, al momento della sua esecuzione, l'Elemento può essere nullo . In tal caso, nella fase precedente al momento dell'accesso al Titolo proprietà direttamente, avremo l'eccezione di NullReferenceException generato, e quindi non vi è alcun uso nell'operatore condizionale nullo.
In ogni caso, questo codice sembra molto strano e deve essere corretto.
Era anche strano che un oggetto fosse fuso nel suo stesso tipo. Ecco un esempio:
public FormsPivot Control { get; private set; }
Brush ITitleProvider.BarBackgroundBrush
{
set { (Control as FormsPivot).ToolbarBackground = value; }
}Avviso di PVS-Studio: V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 73
In questo caso, non è un bug, ma questo codice sembra quantomeno sospetto, tenendo conto che Control l'oggetto ha già un FormsPivot genere. A proposito, non è l'unico avviso di questo tipo, ce ne sono stati molti altri:
- V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 78
- V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.UAP TabbedPageRenderer.cs 282
- V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 175
- V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 197
- V3051 Un cast di tipo eccessivo. L'oggetto è già del tipo 'FormsPivot'. Xamarin.Forms.Platform.WinRT.Phone TabbedPageRenderer.cs 205
Ci sono condizioni che potrebbero essere semplificate. Un esempio di uno di questi:
public override void LayoutSubviews()
{
....
if (_scroller == null || (_scroller != null &&
_scroller.Frame == Bounds))
return;
....
}Avviso di PVS-Studio: V3031 Un controllo eccessivo può essere semplificato. Il '||' operatore è circondato da espressioni opposte. Xamarin.Forms.Platform.iOS.Classic ContextActionCell.cs 102
Questa espressione può essere semplificata rimuovendo la sottoespressione _scroller! =nullo. Verrà valutato solo se l'espressione a sinistra di '||' operatore, _scroller ==null è falso, di conseguenza, _scroller non è null, quindi non possiamo aver paura di ottenere NullReferenceException. Quindi il codice semplificato sarà così:
if (_scroller == null || _scroller.Frame == Bounds))Svantaggi dell'analisi eseguita
Sfortunatamente, non siamo riusciti a compilare l'intera soluzione:6 progetti sono rimasti deselezionati e quei frammenti, in cui sono state utilizzate le classi, non sono stati analizzati a fondo come avrebbero potuto essere. Forse potremmo aver trovato qualcos'altro di nostro interesse.
A proposito, puoi vedere se ci sono problemi con l'analisi dando un'occhiata al messaggio di livello tre, V051. Se sono presenti avvisi di questo tipo, di solito è un segnale che il progetto C# ha alcuni bug di compilazione, a causa dei quali non può ottenere tutte le informazioni necessarie per l'analisi approfondita. Tuttavia, proverà a eseguire i controlli che non richiedono informazioni dettagliate sui tipi e sugli oggetti.
È consigliabile assicurarsi di non avere alcun avviso V051 durante il controllo del progetto. Se ci sono, prova a sbarazzartene (controlla se il progetto è compilato, assicurati che tutte le dipendenze siano state caricate)
Conclusione
Il controllo di Xamarin.Forms è stato piuttosto gratificante:abbiamo trovato diversi frammenti interessanti; alcuni erano davvero errati, altri - sospettosi e strani. La mia speranza è che gli sviluppatori notino l'articolo e risolvano i problemi di cui abbiamo discusso qui. Puoi vedere tutti i frammenti di codice sospetti scaricando una versione di prova dell'analizzatore. La soluzione migliore sarebbe implementare PVS-Studio e utilizzarlo regolarmente, il che consentirà il rilevamento degli errori durante le prime fasi di sviluppo.