La calidad es importante para nosotros. Y escuchamos sobre PVS-Studio. Todo esto llevó al deseo de consultar Docotic.Pdf y descubrir qué más se puede mejorar.
Docotic.Pdf es una biblioteca de uso general para trabajar con archivos PDF. Está escrito en C #, no hay código inseguro, ni dependencias externas que no sean .NET runtime. Funciona tanto con .NET 4+ como con .NET Standard 2+.
La biblioteca ha estado en desarrollo durante poco más de 10 años y tiene 110 mil líneas de código sin tener en cuenta pruebas, ejemplos y otras cosas. Para el análisis estático, utilizamos constantemente Code Analysis y StyleCop. Varios miles de pruebas automatizadas nos protegen de las regresiones. Nuestros clientes de diferentes países y diferentes industrias confían en la calidad de la biblioteca.
¿Qué problemas detectará PVS-Studio?
Instalación y primera impresión.
Descargué la versión de prueba del sitio web PVS-Studio. Gratamente sorprendido por el pequeño tamaño del instalador. Instalado con la configuración predeterminada: motores de análisis, un entorno separado de PVS-Studio, integración en Visual Studio 2017.
Después de la instalación, no se inició nada y se agregaron dos accesos directos con los mismos íconos al menú Inicio: Independiente y PVS-Studio. Por un momento, pensé en qué comenzar. Lancé Standalone y la interfaz me sorprendió desagradablemente. La escala del 200% establecida para mi Windows es compatible de forma torcida. Parte del texto es demasiado pequeño, parte del texto no cabe en el espacio provisto para ello. El nombre, el Unicornio y la lista de Acciones se recortan para cualquier tamaño de ventana. Incluso con pantalla completa.
Bueno, está bien, decidí abrir mi archivo de proyecto. De repente, el menú Archivo no encontró esa oportunidad. Allí solo me ofrecieron abrir archivos individuales. Gracias, pensé, prefiero probar otra opción. Lancé PVS-Studio: me mostraron una ventana con texto borroso. La escala del 200% nuevamente se hizo sentir. El texto informaba:
búscame en Three Crowns, busca el menú PVS-Studio en Visual Studio. Ok, abrí el estudio.
Solución abierta. De hecho, hay un menú PVS-Studio, y tiene la capacidad de verificar el "Proyecto actual". Hizo el proyecto que necesito actual y lanzó un cheque. En el estudio apareció una ventana con los resultados del análisis. En el fondo apareció una ventana con el progreso del escaneo, pero no la encontré de inmediato. Al principio, se tenía la sensación de que el cheque no comenzó o terminó inmediatamente.
Primer resultado de verificación
El analizador verificó todos los archivos de proyecto 1253 en aproximadamente 9 minutos y 30 segundos. Al final de la verificación, el contador de archivos no cambió tan rápido como al principio. Quizás exista una dependencia no lineal de la duración del escaneo con respecto al número de archivos escaneados.
En la ventana de resultados apareció información sobre 81 advertencias altas, 109 medias y 175 bajas. Si calcula la frecuencia, obtiene 0.06 Advertencias altas / archivo, 0.09 Advertencias medias / archivo y 0.14 Advertencias bajas / archivo. O
0.74 Advertencias altas por cada mil líneas de código, 0.99 Advertencias medias por cada mil líneas de código, y 1.59 Advertencias bajas por cada mil líneas de código.
Aquí
en este artículo , se
indica que en CruiseControl.NET, con sus 256 mil líneas de código, el analizador encontró 15 advertencias Alta, 151 Media y 32 Baja.
Resulta que en términos porcentuales para Docotic.Pdf se emitieron más advertencias en cada uno de los grupos.
¿Qué se encuentra?
Decidí ignorar las advertencias bajas en esta etapa.
Ordene las advertencias por la columna Código y resultó que el poseedor absoluto del registro de frecuencia era
V3022 “La expresión siempre es verdadera / falsa” y
V3063 “Una parte de la expresión condicional siempre es verdadera / falsa si se evalúa”. En mi opinión, se trata de una cosa. En total, estas dos advertencias dan 92 de 190 casos. Frecuencia relativa = 48%.
La lógica de dividir en Alto y Medio no está del todo clara. Esperaba
V3072 "La clase 'A' que contiene miembros IDisposable no implementa por sí misma IDisposable" y
V3073 "No todos los miembros IDisposable se eliminan correctamente. Llame 'Eliminar' cuando elimine la clase 'A' ”en el grupo Alto, por ejemplo. Pero esto es gusto, por supuesto.
Sorprende que
V3095 “El objeto se usó antes de que se verificara contra nulo. Verifique las líneas: N1, N2 ”está marcado dos veces como Alto y una vez como Medio. Bicho?
Confía pero verifica
Es hora de verificar qué tan razonables son las advertencias. ¿Se han encontrado errores reales? ¿Hay alguna advertencia incorrecta?
Dividí las advertencias encontradas en los siguientes grupos.
Advertencias importantes
Su corrección aumentó la estabilidad, resolvió problemas con pérdidas de memoria, etc. Errores reales / imperfecciones.
Se emitieron 16 de estos, que es aproximadamente el 8% de todas las advertencias.
Daré algunos ejemplos.
V3019 "Posiblemente, una variable incorrecta se compara con nulo después de la conversión de tipo usando la palabra clave 'as'. Verifique las variables 'color', 'indexado' »
public override bool IsCompatible(ColorImpl color) { IndexedColorImpl indexed = color as IndexedColorImpl; if (color == null) return false; return indexed.ColorSpace.Equals(this); }
Como puede ver, en lugar de indexado, el color variable se compara con nulo. Esto es incorrecto y puede conducir a NRE.
V3080 “Posible desreferencia nula. Considere la posibilidad de inspeccionar 'cstr_index.tile_index' »
Un pequeño fragmento para ilustrar:
if (cstr_index.tile_index == null) { if (cstr_index.tile_index[0].tp_index == null) {
Obviamente, la primera condición implicada! = Nulo. En la forma actual, el código arrojará NRE con cada llamada.
V3083 “Invocación insegura del evento 'OnProgress', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo ".
public void Updated() { if (OnProgress != null) OnProgress(this, new EventArgs()); }
Una advertencia ayudó a solucionar una posible excepción. ¿Por qué puede surgir? Stackoverflow tiene una
buena explicación .
V3106 “Posiblemente el índice está fuera de los límites. El índice '0' apunta más allá del límite 'v' »
var result = new List<FontStringPair>(); for (int i = 0; i < text.Length; ++i) { var v = new List<FontStringPair>(); createPairs(text[i].ToString(CultureInfo.InvariantCulture)); result.Add(v[0]); }
El error es que se ignora el resultado de createPairs y, en su lugar, se accede a una lista vacía. Aparentemente, createPairs inicialmente aceptó la lista como parámetro, pero se cometió un error en el proceso de cambiar el método.
V3117 'No se utiliza el parámetro' constructor 'validateType'
Se emitió una advertencia para un código similar a este
public SomeClass(IDocument document, bool validateType = true) : base(document, true) { m_provider = document; }
La advertencia en sí no parece importante. Pero el problema es más grave de lo que parece a primera vista. En el proceso de agregar el parámetro opcional validateType, se olvidaron de arreglar la llamada al constructor de la clase base.
V3127 „Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y la variable 'rango' debería usarse en lugar de 'dominio' "
private void fillTransferFunction(PdfStreamImpl function) {
Quizás no se emitirá una advertencia si las partes del código son ligeramente diferentes. Pero en este caso, se detectó un error al usar copiar y pegar.
Advertencias teóricas / formales
Son correctos, pero su corrección no corrige ningún error específico y no afecta la legibilidad del código. O señalan lugares donde podría haber un error, pero no existe. Por ejemplo, el orden de los parámetros se cambia intencionalmente. Para tales advertencias, no necesita cambiar nada en el programa.
De estos, se emitieron 57, que es aproximadamente el 30% de todas las advertencias. Daré ejemplos de casos que merecen atención.
V3013 "Es extraño que el cuerpo de la función 'BeginText' sea totalmente equivalente al cuerpo de la función 'EndText' (166, línea 171)"
public override void BeginText() { m_state.ResetTextParameters(); } public override void EndText() { m_state.ResetTextParameters(); }
Ambas funciones corporales son en realidad las mismas. Pero es correcto. ¿Y es realmente tan extraño si los cuerpos de funciones de una línea coinciden?
V3106 „Posible valor de índice negativo. El valor del índice 'c1' podría alcanzar -1 "
freq[256] = 1;
Estoy de acuerdo, le di una parte del algoritmo no tan claro. Pero, en mi opinión, en este caso, el analizador se preocupa en vano.
V3107 "Expresión idéntica 'neighsum' a la izquierda y a la derecha de la asignación compuesta"
La advertencia es causada por un código bastante común:
neighsum += neighsum;
Sí, se puede reescribir mediante multiplicación. Pero no hay error.
V3109 „La sub-expresión 'l_cblk.data_current_size' está presente en ambos lados del operador. La expresión es incorrecta o se puede simplificar ".
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size) {
Los comentarios en el código aclaran la intención. Nuevamente falsa alarma.
Advertencias Justificadas
Su corrección afectó positivamente la legibilidad del código. Es decir, redujo condiciones innecesarias, controles, etc. El efecto sobre cómo funciona el código no es obvio.
De estos, se emitieron 103, que es aproximadamente el 54% de todas las advertencias.
V3008 „A la variable 'l_mct_deco_data' se le asignan valores dos veces seguidas. Quizás esto sea un error "
if (m_nb_mct_records == m_nb_max_mct_records) { ResizeMctRecords(); l_mct_deco_data = (OPJ_INT32)m_nb_mct_records; } l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
Analizador de derechos: asignación dentro si no es necesario.
V3009 "Es extraño que este método siempre devuelva el mismo valor"
private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres) { if (numres == 1U) return true;
Siguiendo el consejo del analizador, el método ha cambiado y no devuelve nada más.
V3022 "La expresión 'Agregar' siempre es verdadera"
private void addToFields(PdfDictionaryImpl controlDict, bool add) {
De hecho, no tiene sentido el segundo si. La condición siempre será verdadera.
V3029 "La expresión condicional de las declaraciones 'if' ubicadas una junto a la otra son idénticas"
if (stroke) extGState.OpacityStroke = opacity; if (stroke) state.AddReal(Field.CA, opacity);
No está claro cómo se originó dicho código. Pero ahora lo arreglamos.
V3031 „Se puede simplificar una verificación excesiva. El '||' el operador está rodeado de expresiones opuestas "
Esta es una condición de pesadilla:
if (!(cp.m_enc.m_tp_on != 0 && ((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz)))) {
Después de los cambios se hizo mucho mejor
if (!(cp.m_enc.m_tp_on != 0 && (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS))) {
V3063 "Una parte de la expresión condicional siempre es verdadera si se evalúa: x! = Nulo"
V3022 "Expresión 'x! = Nulo' siempre es cierto"
Aquí incluí advertencias de que verificar nulo no tiene sentido. Si es correcto hacerlo es una pregunta controvertida. A continuación describí con más detalle la esencia del problema.
Advertencias sin fundamento
Falsos positivos Debido a errores en la implementación de una prueba en particular o algún tipo de falla del analizador.
De estos, se emitieron 14, que es aproximadamente el 7% de todas las advertencias.
V3081 “El contador 'i' no se usa dentro de un bucle anidado. Considere la posibilidad de inspeccionar el uso del contador "j"
Una versión ligeramente simplificada del código para el que se emitió esta advertencia:
for (uint i = 0; i < initialGlyphsCount - 1; ++i) { for (int j = 0; j < initialGlyphsCount - i - 1; ++j) {
Obviamente, se usa en un bucle anidado.
V3125 „El objeto se usó después de que se verificó contra nulo“
Código para el que se emite una advertencia:
private static int Compare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2) { if (er1 == er2) return 0; if (er1 != null && er2 == null) return 1; if (er1 == null && er2 != null) return -1; return er1.CompareTo(er2); }
er1 no puede ser nulo cuando se llama a CompareTo ().
Otro código para el cual se emite esta advertencia:
private static void realloc(ref int[][] table, int newSize) { int[][] newTable = new int[newSize][]; int existingSize = 0; if (table != null) existingSize = table.Length; for (int i = 0; i < existingSize; i++) newTable[i] = table[i]; for (int i = existingSize; i < newSize; i++) newTable[i] = new int[4]; table = newTable; }
La tabla no puede ser nula en un bucle.
V3134 „El desplazamiento por [32..255] bits es mayor que el tamaño del tipo de expresión 'UInt32' '(uint) 1'“
Un código para el que se emite esta advertencia:
byte bitPos = (byte)(numBits & 0x1F); uint mask = (uint)1 << bitPos;
Se puede ver que bitPos puede tener un valor del rango [0..31], pero el analizador cree que puede tener un valor del rango [0..31], que es incorrecto.
No daré otros casos similares, ya que son equivalentes.
Reflexiones adicionales sobre algunos cheques
Me pareció indeseable advertir que 'x! = Null' siempre es cierto en los casos en que x es el resultado de llamar a algún método. Un ejemplo:
private X GetX(int y) { if (y > 0) return new X(...); if (y == 0) return new X(...); throw new ArgumentOutOfRangeException(nameof(x)); } private Method() {
Sí, formalmente, el analizador tiene razón: x no siempre será nulo, porque GetX devolverá una instancia completa o lanzará una excepción. ¿Pero el código mejorará la eliminación del cheque por nulo? ¿Qué pasa si GetX cambia más tarde? ¿Method tiene que conocer la implementación de GetX?
Dentro del equipo, las opiniones estaban divididas. Se ha sugerido que el método actual tiene un contrato por el cual no debe devolver nulo. Y no tiene sentido escribir código redundante "para el futuro" con cada llamada. Y si el contrato cambia, el código de llamada debe actualizarse.
En apoyo de esta opinión, se hizo el siguiente juicio: verificar nulo es como ajustar cada llamada en try / catch en caso de que el método comience a arrojar excepciones en el futuro.
Como resultado, de acuerdo con el principio
YAGNI , decidieron no conservar los cheques y los eliminaron. Todas las advertencias fueron transferidas de teóricas / formales a justificadas.
Estaré encantado de leer tu opinión en los comentarios.
Conclusiones
El análisis estático es algo bueno. PVS-Studio le permite encontrar errores reales.
Sí, hay advertencias irrazonables / incorrectas. Pero aún así PVS-Studio encontró errores reales en un proyecto que ya usa el Análisis de Código. Nuestro producto está bastante bien cubierto por las pruebas, nuestros clientes lo prueban de una forma u otra, pero los
robots lo hacen mejor. El análisis estático sigue siendo beneficioso.
Por último, algunas estadísticas.
Las 3 advertencias más razonables
V3081 „El contador 'X' no se usa dentro de un bucle anidado. Considere inspeccionar el uso del contador "Y"
1 de 1 encontrado irrazonable
V3125 „El objeto se usó después de que se verificó contra nulo. Verifique las líneas: N1, N2 "
9 de cada 10 se declaran infundadas
V3134 "El desplazamiento en N bits es mayor que el tamaño del tipo"
4 de cada 5 son infundadas
Las 3 principales advertencias importantes
V3083 “Invocación de evento insegura, NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo "
5 de 5 fueron considerados importantes.
V3020 „Un 'descanso / continuación / retorno / goto' incondicional dentro de un ciclo“
V3080 "Posible desreferencia nula"
2 de 2 fueron reconocidos como importantes.
V3019 "Es posible que una variable incorrecta se compare con nulo después de la conversión de tipo usando la palabra clave 'como'"
V3127 „Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'X' en lugar de 'Y' "
1 de 1 se consideró importante.