Análisis de IronPython y IronRuby con PVS-Studio

Análisis de IronPython y IronRuby con PVS-Studio

Hace un tiempo, lanzamos una nueva versión de nuestro analizador PVS-Studio con soporte para análisis de código C#. Con el desarrollo detenido por el momento del lanzamiento, aproveché esta oportunidad para probar el analizador. Para mis experimentos, elegí los proyectos IronPython y IronRuby. Después de escanearlos, pensé que podría informarle sobre los resultados del análisis en este pequeño artículo.

IronPython y IronRuby

IronPython y IronRuby son implementaciones de los lenguajes de programación Python y Ruby en la plataforma .NET. Los códigos fuente de los proyectos se pueden descargar desde GitHub aquí. El paquete también incluye el código fuente de DLR. A partir de .NET Framework 4.0, DLR se incluye como parte integral y IronPython y IronRuby lo utilizan. Pero dado que la versión anterior de DLR estaba en el paquete, también la analicé.

Detalles del análisis

Entonces, todo el código se compone de tres grandes partes:DLR, IronPython y IronRuby, y contiene 1630 archivos *.cs. El análisis se realizó con PVS-Studio 6.00, que se puede descargar desde nuestro sitio web. Me tomó un poco más de un minuto analizar la solución. El analizador emite 34 advertencias del primer nivel, 15 advertencias del segundo nivel y 280 advertencias del tercer nivel.

De las 34 advertencias de primer nivel, 19 resultaron ser errores genuinos (lo cual es un buen resultado) y 6 advertencias se refieren a fragmentos sospechosos que deben revisarse. Las 9 advertencias restantes son falsos positivos, la mitad de los cuales pueden eliminarse mediante algunas mejoras en el propio analizador, que realizaremos pronto.

Entre las advertencias de segundo y tercer nivel, hubo muchos menos errores y fragmentos sospechosos.

Errores encontrados

Ahora analicemos ejemplos de errores reales encontrados por PVS-Studio en los proyectos:

Muestras 1 y 2. Descuido.

private bool Enter(RangeExpression/*!*/ node, bool isCondition) {
    ....
    if (!isCondition && litBegin != null && litEnd != null
        && litBegin.Value is int && litBegin.Value is int) {
        _result = MakeNode(NodeKind.lit, new Range(
            (int)litBegin.Value, (int)litEnd.Value,
            node.IsExclusive));
    } else {
    ....
    }
....
}

Mensaje de diagnóstico de PVS-Studio:V3001 Hay subexpresiones idénticas 'litBegin.Value is int' a la izquierda ya la derecha del operador '&&'. IronRubyParseTreeOps.cs 277

En la condición, litBegin.Value se verifica dos veces por ser del tipo 'int' en lugar de verificar también litEnd.Value.

Se pueden encontrar cheques duplicados similares en dos lugares más, por ejemplo:

private static PythonTuple ReduceProtocol2(
    CodeContext/*!*/ context, object self) {
    ....
    if (self is PythonDictionary || self is PythonDictionary) {
        dictIterator = PythonOps.Invoke(context, self,
            "iteritems", ArrayUtils.EmptyObjects);
    }
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3001 Hay subexpresiones idénticas 'self is PythonDictionary' a la izquierda y a la derecha de '||' operador. IronPython ObjectOps.cs 452

Muestra 3. Expresiones idénticas.

protected override MSAst.Expression VisitTry(
    MSAst.TryExpression node) {
    ....
    if (newHandlers != null || newFinally != null) {
        node = Ast.MakeTry(node.Type, node.Body,
            newFinally != null ? newFinally : node.Finally,
            node.Fault,
            newHandlers != null ? newHandlers : newHandlers
        );
    }
    return node;
}

Mensaje de diagnóstico de PVS-Studio:V3012 El operador '?:', independientemente de su expresión condicional, siempre devuelve el mismo valor:newHandlers. DebugInfoRewriter.cs 252

En este ejemplo, newHandlers se usa en ambas partes de la declaración condicional. En realidad, es node.Handlers el que estaba destinado a usarse cuando newHandlers es nulo.

Muestras 4 y 5. Descuido.

public static bool HasValue(RubyContext/*!*/ context,
    object/*!*/ self, object value) {
    var strValue = value as MutableString;
    if (value == null) {
        return false;
    }
    var clrStrValue = strValue.ConvertToString();
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3019 Posiblemente una variable incorrecta se compara con nula después de la conversión de tipo usando la palabra clave 'as'. Compruebe las variables 'valor', 'strValue'. EntornoSingletonOps.cs 189

Al convertir el tipo de una variable con el operador 'as', el error común de los programadores es verificar el objeto de origen, en lugar del resultante, para ver si es nulo y luego usar una referencia no verificada.

Otro caso similar:

private static RubyRegex/*!*/ ConstructRubyRegexp(
    RubyConstructor/*!*/ ctor, Node/*!*/ node) {
    ScalarNode scalar = node as ScalarNode;
    if (node == null) {
        throw RubyExceptions.CreateTypeError(
            "Can only create regex from scalar node");
    }
    Match match = _regexPattern.Match(scalar.Value);
    ....
}

Ejemplo 6. Copiar y pegar.

private void LoadNewObj(CodeContext/*!*/ context) {
    PythonTuple args = PopStack() as PythonTuple;
    if (args == null) {
        throw PythonOps.TypeError("expected second argument, got {0}",
            DynamicHelpers.GetPythonType(args));
    }
    PythonType cls = PopStack() as PythonType;
    if (args == null) {
        throw PythonOps.TypeError("expected first argument, got {0}",
            DynamicHelpers.GetPythonType(args));
    }
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3021 Hay dos declaraciones 'si' con expresiones condicionales idénticas. La primera instrucción 'if' contiene el retorno del método. Esto significa que la segunda declaración 'si' no tiene sentido. cPickle.cs 2194

En este fragmento de código, dos condiciones y llamadas a la función GetPythonType() son totalmente iguales. La segunda condición obviamente se escribió copiando la primera, pero el programador olvidó cambiar el nombre de la variable en el fragmento copiado. Hubo dos errores más de este tipo en el proyecto.

Muestra 7. Condiciones idénticas.

public static int Compare(SourceLocation left, SourceLocation right) {
    if (left < right) return -1;
    if (right > left) return 1;
    return 0;
}

Este método parece demasiado simple para cometer un error en él, ¿no es así? Sin embargo, el programador intercambió parámetros de izquierda a derecha en la segunda condición por alguna razón. Como resultado, ambas condiciones verifican lo mismo, y esto es lo que no le gustó al analizador.

La versión fija del código:

public static int Compare(SourceLocation left, SourceLocation right) {
    if (left < right) return -1;
    if (left > right) return 1;
    return 0;
}

Muestra 8. Condición extra.

private void WriteSingleQuoted(string text, bool split) {
    ....
    while (ending <= text.Length) {
        c = '\0';
        if (ending < text.Length) {
            c = text[ending];
        }
        if (spaces) {
            if (c == 0 || c != 32) {
            ....
}

Mensaje de diagnóstico de PVS-Studio:V3023 Considere inspeccionar el 'c ==0 || c !=32' expresión. La expresión es excesiva o contiene un error tipográfico. Emisor.cs 308

A la variable 'c' se le asigna primero un valor predeterminado, '\0'. Luego, en caso de que toda la cadena no haya sido procesada todavía, se asigna 'c' al siguiente carácter de la cadena. Al final, se comprueba si 'c' aún contiene el valor predeterminado o cualquier otro carácter excepto el espacio. La comprobación de cero no es necesaria aquí, en realidad, ya que cero no puede ser igual a 32 (el código de espacio) de todos modos. Este defecto no causa ningún error, pero hace que el código sea menos claro, por lo que se debe omitir la verificación nula. El analizador encontró algunas comprobaciones adicionales similares más en este proyecto.

Muestras 9 y 10. Cadena de formato incorrecto.

El problema general sobre el uso de la función String.Format es que el compilador no verifica si la cantidad y los números de los parámetros de una cadena de formato se corresponden con los números de parámetros pasados a String.Format. Puede dar como resultado la formación de una cadena incorrecta o generar una FormatException. Vea los siguientes ejemplos.

public T Current {
    get {
        try {
            return (T)enumerable.Current;
        }
        catch (InvalidCastException iex) {
            throw new InvalidCastException(string.Format(
"Error in IEnumeratorOfTWrapper.Current. Could not cast: {0} in {0}",
typeof(T).ToString(), enumerable.Current.GetType().ToString()), iex);
        }
    }
}

Mensaje de diagnóstico de PVS-Studio:V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'Formato'. Esperado:1. Presente:2. ConversionWrappers.cs 235

En este ejemplo, el último parámetro no se utiliza. En su lugar, el valor typeof(T).ToString() se imprimirá dos veces.

private static void DumpGenericParameters(
    MetadataTableView genericParams,
    MetadataRecord owner) {
    foreach (GenericParamDef gp in genericParams) {
        _output.WriteLine("  generic parameter #{0}: {1}",
        gp.Index, gp.Name, gp.Attributes);
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3025 Formato incorrecto. Se espera un número diferente de elementos de formato al llamar a la función 'WriteLine'. Esperado:2. Presente:3. Program.cs 268

Y aquí, la función WriteLine recibe un parámetro más de lo sugerido por la cadena de formato.

Ejemplo 11. Verificación nula después de direccionar.

public static MutableString ChompInPlace(....) {
    MutableString result = InternalChomp(self, separator);
    if (result.Equals(self) || result == null) {
        self.RequireNotFrozen();
        return null;
    }
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3027 La variable 'resultado' se utilizó en la expresión lógica antes de verificarse contra nulo en la misma expresión lógica. MutableStringOps.cs 1097

En esta condición, la verificación nula y la llamada al método Equals deben intercambiarse. De la forma en que está escrito originalmente, la aplicación puede bloquearse y generar una NullReferenceException.

Ejemplo 12. Problemas con la sincronización.

class DictThreadGlobalState {
    public int DoneCount;
    ....
}

private static void RunThreadTest(DictThreadGlobalState globalState) {
    ....
    globalState.DoneEvent.Reset();
    globalState.Event.Set();
    while (globalState.DoneCount != 0) {
        // wait for threads to get back to finish
    }
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3032 Esperar esta expresión no es confiable, ya que el compilador puede optimizar algunas de las variables. Utilice variables volátiles o primitivas de sincronización para evitar esto. PruebaDeMotor.cs 2558

Este código contiene un error que solo aparecerá en algunas ocasiones, según el entorno de ejecución, la versión de .NET Framework, la cantidad de procesadores en la computadora y otras especificaciones de implementación. Dichos errores son muy difíciles de atrapar. En este caso, la variable DoneCount no se declara como volátil; por lo tanto, el compilador asume que solo lo usa un subproceso y su valor se puede almacenar en caché y luego restaurar desde el caché todo el tiempo, ya que esta variable no cambia dentro del ciclo. En nuestro caso, sin embargo, sí cambia en otro hilo. Es por eso que las variables deben declararse como volátiles cuando se usan para sincronizar hilos. Consulte MSDN para obtener más detalles.

Ejemplo 13. Doble asignación

private static Dictionary<string, EncodingInfoWrapper>
    MakeCodecsDict() {
    ....
    switch (normalizedName) {
        case "iso_8859_1":
            d["8859"] = d["latin_1"] = d["latin1"] =
            d["iso 8859_1"] = d["iso8859_1"] = d["cp819"] = d["819"] =
            d["latin"] = d["latin1"] = d["l1"] = encs[i];
            break;
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3005 La variable 'd["latin1"]' está asignada a sí misma. StringOps.cs 1905

En este código, a la variable d["latin1"] se le asignan valores dos veces. La segunda asignación parece ser solo un código superfluo, no un error. Pero también es posible que este código estuviera destinado a manejar alguna página de códigos. De todos modos, debería comprobarse.

Ejemplo 14. Comprobar si una variable sin signo es nula

public static int __hash__(UInt64 x) {
    int total = unchecked((int) (((uint)x) + (uint)(x >> 32)));
    if (x < 0) {
        return unchecked(-total);
    }
    return total;
}

Mensaje de diagnóstico de PVS-Studio:V3022 La expresión 'x <0' siempre es falsa. El valor de tipo sin firmar siempre es>=0. IntOps.Generated.cs 1967

Estoy casi seguro de que es 'total', no 'x', que debería compararse con nulo porque no parece correcto hacer algo a 'x' todo el tiempo y luego verificar un caso especial. Además, 'total' está firmado, por lo que la verificación "total <0" parece tener más sentido.

Muestra 15. Cheques idénticos.

public void ReflectTypes(Type[]/*!*/ allTypes) {
    ....
    def.Super = null;
    if (cls != null && def.Extends != typeof(BasicObject)
        && !def.Extends.IsInterface) {
        if (cls != null && cls.Inherits != null) {
            def.Super = new TypeRef(cls.Inherits);
    ....
}

Mensaje de diagnóstico de PVS-Studio:V3030 Verificación recurrente. La condición 'cls !=null' ya se verificó en la línea 373. LibraryDef.cs 374

En ambas condiciones, se comprueba si la variable 'cls' es nula. El programador probablemente quería verificar 'def' para nulo en la primera condición, ya que abordan su propiedad Extends justo después de la verificación. Pero tampoco es realmente necesario, porque a 'def.Super' se le asigna un valor nulo justo antes de la condición, lo que significa que 'def' ya no es nulo. Entonces, es solo un cheque adicional.

Ejemplo 16. Copiar y pegar.

Ahora llegamos a las advertencias de tercer nivel, que hacen un total de 280. La mayoría de ellas tratan sobre pares de funciones con cuerpos idénticos y comparación de números de punto flotante. No esperaba encontrar nada serio aquí, así que comencé a hojear las advertencias, pero finalmente me topé con algo interesante.

public static bool IsPositiveOne(BigDecimal x) {
    return IsOne(x) && IsPositive(x);
}
public static bool IsNegativeOne(BigDecimal x) {
    return IsOne(x) && IsPositive(x);
}

Mensaje de diagnóstico de PVS-Studio:V3013 Es extraño que el cuerpo de la función 'IsPositiveOne' sea totalmente equivalente al cuerpo de la función 'IsNegativeOne' (351, línea 355). BigDecimal.cs 351

Este es un error real que resulta de copiar código de una función a otra. La versión corregida del código debería verse así:

public static bool IsNegativeOne(BigDecimal x) {
    return IsOne(x) && IsNegative(x);
}

Muestra 17. Comprobación extraña de NaN.

public static bool Equals(float x, float y) {
    if (x == y) {
        return !Single.IsNaN(x);
    }
    return x == y;
}

Mensaje de diagnóstico de PVS-Studio:V3024 Una comparación extraña y precisa:x ==y. Considere usar una comparación con precisión definida:Math.Abs(A - B)

No estoy seguro de por qué uno necesitaría un cheque especial para NaN aquí. Si la condición (x ==y) es verdadera, entonces ni 'x' ni 'y' son NaN porque NaN no es igual a ningún otro valor, incluido él mismo. Es decir, la primera declaración de retorno siempre devolverá verdadero. Parece que esta comprobación de NaN es simplemente superflua.

Conclusión

Creo que el analizador había hecho bien con el análisis de estos proyectos. En primer lugar, detectó un par de docenas de errores interesantes, cuya corrección mejorará el código del proyecto; en segundo lugar, encontré algunos falsos positivos que pueden eliminarse realizando algunas mejoras en nuestro producto. Por lo tanto, animo a todos a que descarguen la versión de demostración de PVS-Studio y la ejecuten en su código.