La publicación de Microsoft del código fuente para sus proyectos es una buena razón para verificarlos. Esta vez no fue la excepción, y hoy miramos los lugares sospechosos que se encuentran en el código Infer.NET. Abajo la anotación: ¡vaya al grano!
Un poco sobre el proyecto y el analizador.
Infer.NET es un sistema de aprendizaje automático desarrollado por especialistas de Microsoft. El código fuente del proyecto recientemente estuvo disponible en
GitHub , que fue el motivo de su verificación. Se pueden encontrar más detalles sobre el proyecto, por ejemplo,
aquí .
El proyecto se verificó utilizando el analizador estático PVS-Studio versión 6.26. Permítame recordarle que PVS-Studio está buscando errores en el código en C \ C ++ \ C # (y pronto en Java) en Windows, Linux, macOS. C # código hasta ahora estamos analizando solo en Windows. El analizador se puede
descargar y probar en su proyecto.
El cheque en sí mismo fue extremadamente simple y sin problemas. Anteriormente, descargué el proyecto de GitHub, restauré los paquetes necesarios (dependencias) y me aseguré de que el proyecto se haya creado correctamente. Esto es necesario para que el analizador tenga acceso a toda la información necesaria para un análisis completo. Después de ensamblar en un par de clics, inicié el análisis de la solución a través del complemento PVS-Studio para Visual Studio.
Por cierto, este no es el primer proyecto de Microsoft probado usando PVS-Studio; hubo otros:
Roslyn ,
MSBuild ,
PowerShell ,
CoreFX y
otros .
Nota Si usted o sus conocidos están interesados en analizar el código Java, puede escribirnos en
soporte seleccionando "Quiero un analizador para Java". No hay una versión beta pública del analizador, pero debería estar disponible pronto. En algún lugar de un laboratorio secreto (a través del muro), los muchachos están trabajando activamente en él.
Pero suficiente conversación abstracta: veamos los problemas en el código.
¿Es esto un error o una característica?
Sugiero tratar de encontrar el error usted mismo, una tarea completamente solucionable. No hay bromas en el espíritu de lo que estaba en el artículo "
Los 10 errores principales en proyectos de C ++ para 2017 ", sinceramente. Por lo tanto, no se apresure a leer la advertencia del analizador proporcionada después del fragmento de código.
private void MergeParallelTransitions() { .... if ( transition1.DestinationStateIndex == transition2.DestinationStateIndex && transition1.Group == transition2.Group) { if (transition1.IsEpsilon && transition2.IsEpsilon) { .... } else if (!transition1.IsEpsilon && !transition2.IsEpsilon) { .... if (double.IsInfinity(transition1.Weight.Value) && double.IsInfinity(transition1.Weight.Value)) { newElementDistribution.SetToSum( 1.0, transition1.ElementDistribution, 1.0, transition2.ElementDistribution); } else { newElementDistribution.SetToSum( transition1.Weight.Value, transition1.ElementDistribution, transition2.Weight.Value, transition2.ElementDistribution); } .... }
Advertencia de PVS-Studio :
V3001 Hay
subexpresiones idénticas 'double.IsInfinity (transition1.Weight.Value)' a la izquierda y a la derecha del operador '&&'. Runtime Automaton.Simplification.cs 479
Como puede ver en el fragmento de código, el método está trabajando con un par de variables:
transición1 y
transición2 . El uso de nombres similares a veces está bastante justificado, pero vale la pena recordar que en este caso aumenta la probabilidad de cometer un error accidentalmente en algún lugar con el nombre.
Esto es
lo que sucedió al verificar los números para el infinito (
double.IsInfinity ). Debido a un error, verificamos el valor de la misma variable 2 veces:
transition1.Weight.Value . El valor marcado en la segunda subexpresión debía ser la variable
transición2.Peso.Valor .
Otro código sospechoso similar.
internal MethodBase ToMethodInternal(IMethodReference imr) { .... bf |= BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance; .... }
Advertencia de PVS-Studio :
V3001 Hay
subexpresiones idénticas 'BindingFlags.Public' a la izquierda y a la derecha de '|' operador Compilador CodeBuilder.cs 194
Al formar el valor de la variable
bf , el elemento de enumeración
BindingFlags.Public se usa dos veces. O este código contiene una operación de marcado adicional, o en lugar del segundo uso de
BindingFlags.Public, debería haber un valor de enumeración diferente.
Por cierto, en el código fuente este código está escrito en una línea. Me parece que si está formateado en un estilo de tabla (como aquí), el problema es más fácil de detectar.
Sigamos adelante. Traigo todo el cuerpo del método y nuevamente le sugiero que encuentre el error (o tal vez el error) usted mismo.
private void ForEachPrefix(IExpression expr, Action<IExpression> action) {
Has encontrado Estamos comprobando!
Advertencias de PVS-Studio :
- V3003 Se usó el patrón 'if (A) {...} else if (A) {...}'. Hay una probabilidad de presencia de error lógico. Verificación de líneas: 1719, 1727. Código del compiladorRecognizer.cs 1719
- V3003 Se usó el patrón 'if (A) {...} else if (A) {...}'. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 1721, 1729. Código del compiladorRecognizer.cs 1721
Simplifique un poco el código para que los problemas sean más obvios.
private void ForEachPrefix(IExpression expr, Action<IExpression> action) { if (....) .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action) .... }
Expresiones condicionales y
luego ramas de varios
si las declaraciones están duplicadas. Quizás este código se escribió utilizando el método de copiar y pegar, por lo que surgió el problema. Ahora resulta que las ramas de los duplicados nunca se ejecutarán, ya que:
- si la expresión condicional es verdadera, se ejecuta el cuerpo de la primera instrucción if del par correspondiente;
- Si la expresión condicional es falsa en el primer caso, será falsa en el segundo.
Desde
entonces, las ramas contienen las mismas acciones, ahora parece un código redundante que es confuso. Es posible que este sea un tipo diferente de problema: en lugar de duplicados, se deberían haber realizado otras verificaciones.
Continuamos
public int Compare(Pair<int, int> x, Pair<int, int> y) { if (x.First < y.First) { if (x.Second >= y.Second) {
Advertencias de PVS-Studio :
- V3004 La declaración 'then' es equivalente a la declaración 'else'. Runtime RegexpTreeBuilder.cs 1080
- V3004 La declaración 'then' es equivalente a la declaración 'else'. Runtime RegexpTreeBuilder.cs 1093
El código parece extremadamente sospechoso, ya que contiene dos declaraciones condicionales con cuerpos idénticos de
entonces y de
otras ramas. En ambos casos, probablemente valga la pena devolver valores diferentes. O, si este es un comportamiento concebido, será útil eliminar las declaraciones condicionales redundantes.
Hubo ciclos interesantes. Ejemplo a continuación:
private static Set<StochasticityPattern> IntersectPatterns(IEnumerable<StochasticityPattern> patterns) { Set<StochasticityPattern> result = new Set<StochasticityPattern>(); result.AddRange(patterns); bool changed; do { int count = result.Count; AddIntersections(result); changed = (result.Count != count); break; } while (changed); return result; }
Advertencia de PVS-Studio :
V3020 Una 'interrupción' incondicional dentro de un bucle. Compilador DefaultFactorManager.cs 474
Debido a la declaración de
interrupción incondicional, se realiza exactamente una iteración del bucle, y la variable de control
modificada ni siquiera se usa. En general, el código parece extraño y sospechoso.
El mismo método (copia exacta) se encontró en otra clase. Advertencia del analizador correspondiente:
V3020 Una 'interrupción' incondicional dentro de un bucle. Visualizers.Windows FactorManagerView.cs 350
Por cierto, un método se encontró con una declaración de
continuación incondicional en un bucle (fue encontrado por el analizador con el mismo diagnóstico), pero hubo un comentario arriba que confirma que esta es una solución temporal especial:
Recuerdo que no hubo tales comentarios cerca de la declaración de
ruptura incondicional.
Sigamos adelante.
internal static DependencyInformation GetDependencyInfo(....) { .... IExpression resultIndex = null; .... if (resultIndex != null) { if (parameter.IsDefined( typeof(SkipIfMatchingIndexIsUniformAttribute), false)) { if (resultIndex == null) throw new InferCompilerException( parameter.Name + " has SkipIfMatchingIndexIsUniformAttribute but " + StringUtil.MethodNameToString(method) + " has no resultIndex parameter"); .... } .... } .... }
Advertencia de PVS-Studio :
V3022 La expresión 'resultIndex == null' siempre es falsa. Compilador FactorManager.cs 382
Inmediatamente, observo que entre la declaración y la verificación anterior, el valor de la variable
resultIndex puede cambiar. Sin embargo, entre las comprobaciones
resultIndex! = Null y
resultIndex == null, el valor ya no se puede cambiar. Por lo tanto, el resultado de la expresión
resultIndex == null siempre será
falso , lo que significa que nunca se lanzará una excepción.
Espero que tenga interés en encontrar errores usted mismo, sin mis sugerencias, para encontrar un problema, pero por si acaso, le propondré hacerlo nuevamente. El código del método es pequeño, lo daré en su totalidad.
public static Tuple<int, string> ComputeMovieGenre(int offset, string feature) { string[] genres = feature.Split('|'); if (genres.Length < 1 && genres.Length > 3) { throw new ArgumentException(string.Format( "Movies should have between 1 and 3 genres; given {0}.", genres.Length)); } double value = 1.0 / genres.Length; var result = new StringBuilder( string.Format( "{0}:{1}", offset + MovieGenreBuckets[genres[0]], value)); for (int i = 1; i < genres.Length; ++i) { result.Append( string.Format( "|{0}:{1}", offset + MovieGenreBuckets[genres[i].Trim()], value)); } return new Tuple<int, string>(MovieGenreBucketCount, result.ToString()); }
Veamos que pasa aquí. La cadena de entrada se analiza con el carácter '|'. Si la longitud de la matriz no es la esperada, se debe lanzar una excepción.
Espera un segundo ...
géneros .
Longitud <1 & & géneros. Longitud> 3 ? Como no hay un número que se encuentre inmediatamente en el rango de valores requeridos por la expresión (
[int.MinValue..1) y
(3..int.MaxValue] ), el resultado de la expresión siempre será
falso . Por lo tanto, esta verificación no protege contra nada y no se lanzará la excepción esperada.
Esto es exactamente lo que advierte el analizador:
V3022 Expresión 'géneros. Longitud <1 && géneros. Longitud> 3' siempre es falso. Probablemente el '||' El operador debe usarse aquí. Características del evaluador.cs 242
Operación de fisión sospechosa se reunió.
public static void CreateTrueThetaAndPhi(....) { .... double expectedRepeatOfTopicInDoc = averageDocLength / numUniqueTopicsPerDoc; .... int cnt = Poisson.Sample(expectedRepeatOfTopicInDoc); .... }
Advertencia de PVS-Studio :
V3041 La expresión se
convirtió implícitamente de tipo 'int' a tipo 'doble'. Considere utilizar un molde de tipo explícito para evitar la pérdida de una parte fraccional. Un ejemplo: doble A = (doble) (X) / Y;. LDA Utilities.cs 74
Aquí es sospechoso: se realiza una división entera (las variables
averageDocLength y
numUniqueTopicsPerDoc son de tipo
int ), y el resultado se escribe en una variable de tipo
double . Se plantea la pregunta: ¿se hace especialmente o todavía se implica la división de números reales? Si la variable
esperadaRepeatOfTopicInDoc fuera del tipo
int , esto aclararía las posibles preguntas.
En otros lugares, se
usa el método
Poisson.Sample , cuyo argumento es la variable sospechosa
esperadaRepeatOfTopicInDoc , por ejemplo, como se describe a continuación.
int numUniqueWordsPerTopic = Poisson.Sample((double)averageWordsPerTopic);
averageWordsPerTopic es de tipo
int , que ya está convertido a
doble en el lugar de uso.
Y aquí hay otro lugar de uso:
double expectedRepeatOfWordInTopic = ((double)numDocs) * averageDocLength / numUniqueWordsPerTopic; .... int cnt = Poisson.Sample(expectedRepeatOfWordInTopic);
Tenga en cuenta que las variables tienen los mismos nombres que en el ejemplo original, solo la división de números reales se usa para inicializar el
esperado RepeatOfWordInTopic (debido a la conversión explícita de
numDocs al
doble ).
En general, vale la pena mirar el punto de partida donde el analizador emitió una advertencia.
Pero reflexiones sobre si vale la pena editar y cómo, dejemos a los autores del código (lo saben mejor), pero vamos más allá. A la siguiente división sospechosa.
public static NonconjugateGaussian BAverageLogarithm(....) { .... double v_opt = 2 / 3 * (Math.Log(mx * mz / Ex2 / 2) - m); if (v_opt != v) { .... } .... }
Advertencia de PVS-Studio :
V3041 La expresión se
convirtió implícitamente de tipo 'int' a tipo 'doble'. Considere utilizar un molde de tipo explícito para evitar la pérdida de una parte fraccional. Un ejemplo: doble A = (doble) (X) / Y;. Tiempo de ejecución ProductExp.cs 137
El analizador volvió a detectar la operación sospechosa de la división de enteros, ya que
2 y
3 son literales numéricos enteros, y el resultado de la expresión 2/3 será
0 . Como resultado, toda la expresión toma la forma:
double v_opt = 0 * expr;
De acuerdo, un poco extraño. Varias veces volví a esta advertencia, tratando de encontrar algún tipo de captura, y no tratando de agregarla al artículo. El método está lleno de matemáticas y varias fórmulas (que, francamente, realmente no quería desmontar), nunca se sabe qué esperar. Además, trato de ser lo más escéptico posible sobre las advertencias que escribo en el artículo, y las describo solo después de haberlas estudiado mejor.
Pero luego me di cuenta: ¿por qué necesito un factor de
0 , escrito como
2/3 ? Así que vale la pena ver este lugar de todos modos.
public static void WriteAttribute(TextWriter writer, string name, object defaultValue, object value, Func<object, string> converter = null) { if ( defaultValue == null && value == null || value.Equals(defaultValue)) { return; } string stringValue = converter == null ? value.ToString() : converter(value); writer.Write($"{name}=\"{stringValue}\" "); }
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula. Considere inspeccionar el 'valor'. Compilador WriteHelpers.cs 78
Afirmación bastante justa del analizador basada en la condición. La desreferenciación de una referencia nula puede ocurrir en el valor de la expresión.
Igual ( valor predeterminado) si
valor == nulo . Dado que esta expresión es el operando derecho del operador ||, para calcularlo, el operando izquierdo debe ser
falso , y para esto es suficiente que al menos una de las variables
defaultValue \
value no sea
nula . Como resultado, si
defaultValue! = Null , y
value == null :
- defaultValue == null -> false ;
- defaultValue == null && value == null -> false ; (no se verificó el valor)
- value.Equals (defaultValue) -> NullReferenceException , ya que el valor es nulo .
Veamos un caso similar:
public FeatureParameterDistribution( GaussianMatrix traitFeatureWeightDistribution, GaussianArray biasFeatureWeightDistribution) { Debug.Assert( (traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count), "The provided distributions should be valid and consistent in the number of features."); .... }
Advertencia de PVS-Studio :
V3080 Posible desreferencia nula. Considere la posibilidad de inspeccionar 'traitFeatureWeightDistribution'. Recomendador FeatureParameterDistribution.cs 65
Desechamos el exceso, dejando solo la lógica para calcular el valor booleano, de modo que sea más fácil de entender:
(traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count)
De nuevo, el operando correcto de || Se calcula solo si el resultado del cálculo de la izquierda es
falso . El operando izquierdo puede ser
falso , incluso cuando
traitFeatureWeightDistribution == null y
biasFeatureWeightDistribution! = Null . Luego, se calculará el operando correcto del operador ||, y una llamada a
traitFeatureWeightDistribution.All generará una
excepción ArgumentNullException .
Otra pieza interesante de código:
public static double GetQuantile(double probability, double[] quantiles) { .... int n = quantiles.Length; if (quantiles == null) throw new ArgumentNullException(nameof(quantiles)); if (n == 0) throw new ArgumentException("quantiles array is empty", nameof(quantiles)); .... }
PVS-Studio Advertencia :
V3095 El objeto 'cuantiles' se usó antes de que se verificara como nulo. Líneas de verificación: 91, 92. Runtime OuterQuantiles.cs 91
Tenga en cuenta que
primero se accede a la propiedad
quantiles.Length y luego
se verifica que los
cuantiles sean nulos . Como resultado, si los
cuantiles == nulo , el método arrojará una excepción, solo un poco mal, y un poco no donde se necesitaba. Aparentemente, desordenaron las líneas en algunos lugares.
Si usted mismo se ha ocupado de la detección de los errores anteriores, le sugiero que prepare una taza de café e intente repetir la hazaña, encontrando el error en el siguiente método. Para hacerlo un poco más interesante, cito el código completo del método.
(
Enlace a tamaño completo )
Está bien, está bien, eso fue una broma (¿o tuviste éxito?!). Simplifiquemos un poco la tarea:
if (sample.Precision < 0) { precisionIsBetween = true; lowerBound = -1.0 / v; upperBound = -mean.Precision; } else if (sample.Precision < -mean.Precision) { precisionIsBetween = true; lowerBound = 0; upperBound = -mean.Precision; } else {
¿Ha mejorado? El analizador emitió la siguiente advertencia para este código:
V3008 A la variable 'lowerBound' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 324, 323. Runtime GaussianOp.cs 324
De hecho, en la última rama
más , el valor de la variable
lowerBound se asigna dos veces seguidas. Aparentemente (y a juzgar por el código anterior), la variable
upperBound debe estar involucrada en una de las asignaciones.
Seguimos más allá.
private void WriteAucMatrix(....) { .... for (int c = 0; c < classLabelCount; c++) { int labelWidth = labels[c].Length; columnWidths[c + 1] = labelWidth > MaxLabelWidth ? MaxLabelWidth : labelWidth; for (int r = 0; r < classLabelCount; r++) { int countWidth = MaxValueWidth; if (countWidth > columnWidths[c + 1]) { columnWidths[c + 1] = countWidth; } } .... }
Advertencia de PVS-Studio :
V3081 El contador 'r' no se usa dentro de un bucle anidado. Considere inspeccionar el uso del contador 'c'. CommandLine ClassifierEvaluationModule.cs 459
Tenga en cuenta que el contador del bucle interno -
r - no se usa en el cuerpo de este bucle. Debido a esto, resulta que durante todas las iteraciones del bucle interno se realizan las mismas operaciones en los mismos elementos; después de todo, los índices también usan el contador del bucle externo (
c ), y no el interno (
r ).
Veamos qué más se encontró interesante.
public RegexpFormattingSettings( bool putOptionalInSquareBrackets, bool showAnyElementAsQuestionMark, bool ignoreElementDistributionDetails, int truncationLength, bool escapeCharacters, bool useLazyQuantifier) { this.PutOptionalInSquareBrackets = putOptionalInSquareBrackets; this.ShowAnyElementAsQuestionMark = showAnyElementAsQuestionMark; this.IgnoreElementDistributionDetails = ignoreElementDistributionDetails; this.TruncationLength = truncationLength; this.EscapeCharacters = escapeCharacters; }
Advertencia de PVS-Studio : no se
usa el parámetro
V3117 Constructor 'useLazyQuantifier'. Runtime RegexpFormattingSettings.cs 38
El constructor no usa un parámetro:
useLazyQuantifier . Esto parece particularmente sospechoso en el contexto del hecho de que una propiedad con el nombre y el tipo correspondiente se define en la clase:
UseLazyQuantifier . Aparentemente, olvidaron inicializarlo a través del parámetro correspondiente.
Conocí a varios controladores de eventos potencialmente peligrosos. A continuación se muestra un ejemplo de uno de ellos:
public class RecommenderRun { .... public event EventHandler Started; .... public void Execute() {
Advertencia de PVS-Studio :
V3083 Invocación insegura del evento 'Iniciado', es posible NullReferenceException. Considere asignar un evento a una variable local antes de invocarlo. Evaluador recomendado: Run.cs 115
El hecho es que entre la verificación de la desigualdad
nula y la llamada del controlador, se puede cancelar la suscripción de un evento, y si en el momento entre la verificación de
nulo y la llamada de los controladores el evento no tiene suscriptores, se lanzará una
NullReferenceException . Para evitar tales problemas, por ejemplo, puede guardar el enlace a la cadena de delegados en una variable local o usar el operador '?.' para llamar a los manejadores.
Además del fragmento de código anterior, había 35 de esos lugares.
Por cierto, también se cumplieron
785 advertencias de
V3024 . Se
emite la advertencia
V3024 al comparar números reales utilizando los operadores '! =' O '=='. No me detendré aquí sobre por qué tales comparaciones no siempre son correctas: más sobre esto está escrito en la documentación, también hay un enlace a
StackOverflow (esto es todo).
Teniendo en cuenta que las fórmulas y los cálculos a menudo se cumplieron, estas advertencias también pueden ser importantes, aunque se llevan al nivel 3 (ya que están lejos de ser relevantes en todos los proyectos).
Si está seguro de que estas advertencias son irrelevantes, puede eliminarlas con
casi un clic , reduciendo el número total de operaciones del analizador.
Conclusión
De alguna manera resultó que durante mucho tiempo no había escrito artículos sobre la verificación de proyectos, y tocar este proceso nuevamente fue bastante agradable. Espero que también hayas aprendido algo nuevo / útil de este artículo, o al menos lo leas con interés.
Les deseo a los desarrolladores una corrección temprana de las áreas problemáticas y les recuerdo que cometer errores es normal, sin embargo, somos personas. Para esto, se necesitan herramientas adicionales como analizadores estáticos para encontrar lo que una persona se perdió, ¿verdad? De todos modos, ¡buena suerte con el proyecto y gracias por el trabajo!
Y recuerde que el beneficio máximo de un analizador estático se logra con su
uso regular .
Todo lo mejor!
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Vasiliev.
¿Qué errores acechan en el código Infer.NET?