Статический анализ – крайне полезный инструмент для любого разработчика, так как помогает вовремя отыскать не только ошибки, но и просто подозрительные и странные фрагменты кода, которые могут вызвать недоумение у программистов, которым пришлось бы работать с ним в будущем. Эту мысль продемонстрирует анализ открытого C# проекта TensorFlow.NET, разрабатываемый для работы с популярной библиотекой машинного обучения TensorFlow. Меня зовут Никита Липилин. Некоторое время назад я присоединился к отделу C# программистов компании PVS-Studio. Традиционно, все новички в команде пишут статьи, в которых рассматриваются результаты проверки различных открытых проектов с помощью статического анализатора PVS-Studio. Такие статьи помогают новым сотрудникам лучше познакомиться с продуктом, а заодно несут дополнительную пользу с точки зрения популяризации методологии статического анализа. Предлагаю вам для ознакомления мою первую работу по теме анализа открытых проектов.
Введение
Разнообразие возможных ошибок в программном коде поражает. Некоторые из них обнаруживают себя сразу, при поверхностном осмотре созданного приложения, другие бывает проблематично заметить даже при проведении code review командой опытных разработчиков. Однако бывает и так, что по невнимательности или какой-либо другой причине программист иногда пишет попросту странный и нелогичный код, который, тем не менее, (вроде бы) успешно выполняет свою функцию. Вот только далее, при возвращении к написанному, или при изучении этого кода другими, начинают возникать вопросы, на которые не найти ответа.
Проведение рефакторинга старого кода может обернуться проблемами, особенно если от него зависят другие части программы, поэтому при обнаружении даже каких-то откровенно кривых конструкций часто применяется позиция -Работает? Вот и не трогай! Впоследствии это затрудняет изучение исходников, а следовательно, становится сложно расширять имеющиеся возможности. Кодовая база засоряется, появляется вероятность того, что не будет вовремя исправлена какая-то небольшая и незаметная, но потенциально весьма неприятная внутренняя проблема.
В определённый момент последствия этой ошибки дадут о себе знать, но вот её поиск займёт уже много времени, ведь подозрения разработчика будут падать на огромное количество странных фрагментов кода, которые не были в своё время подвергнуты рефакторингу. Из этого следует вывод, что различные проблемы и странности в том или ином фрагменте должны исправляться непосредственно после его написания. В случае же, когда есть разумные причины оставить всё как есть (например, если код написан в качестве некоторой заготовки ?на потом?), такой фрагмент должен сопровождаться поясняющим комментарием.
Стоит также заметить, что, независимо от квалификации разработчика, некоторые проблемные и просто неудачно реализованные моменты могут ускользнуть от его взгляда, а иной раз в каком-то месте может быть применено ?временное решение?, которое вскоре забывается и становится ?постоянным?. Впоследствии на разбор такого кода (скорее всего, этим будет заниматься вообще другой разработчик) будет потрачено непозволительно много сил.
В таких случаях может помочь code review. Однако, если задача достаточно серьёзная, то этот вариант потребует большого количества времени. Кроме того, когда мелких ошибок или недочётов много, то за ними проверяющий вполне может не заметить ошибок высокого уровня. Проверка кода становится утомительной рутиной, и постепенно эффективность ревью падает.
Очевидно, что рутинные задачи гораздо лучше будет переложить с человека на компьютер. Именно такой подход используется во многих направлениях современности. Автоматизация различного рода процессов – ключ к процветанию. Что же есть автоматизация в контексте рассматриваемой темы?
Верным помощником при решении задачи написания разумного и стабильно работающего кода является статический анализ. Каждый раз перед отправкой результатов своей деятельности на ревью программист сможет провести автоматизированную проверку и не нагружать других разработчиков (да и себя самого) лишней работой. Код будет передаваться на проверку только после того, как все предупреждения анализатора учтены: ошибки исправлены, а странные моменты переписаны или хотя бы объяснены комментарием.
Конечно, необходимость code review не исчезает, однако статический анализ дополняет и значительно упрощает его проведение. Достаточно большая часть ошибок будет исправлена благодаря анализатору, а странные моменты будут уже точно не забыты и соответствующим образом отмечены. Соответственно, при code review можно будет сосредоточиться на проверке корректности реализации сложных логических взаимодействий и поиске глубинных проблем, которые, к сожалению, (пока что) не могут быть выявлены анализатором.
TensorFlow.NET
Фрагменты, привлекшие внимание при изучении отчёта анализатора
На самом деле, довольно большое количество предупреждений анализатора для этого проекта можно назвать не столько ошибками, сколько именно странным кодом. При просмотре строк, для которых выдаётся предупреждение, возникает по меньшей мере недоумение. Конечно, некоторые из представленных ниже примеров, вероятно, являются ?временными решениями?, вот только каких-либо комментариев по этому поводу не даётся, а значит, у того, кто будет работать с этим кодом в будущем, возникнут вопросы, поиск ответов на которые займёт дополнительное время.
Изощрённый обход коллекции
private static void _RemoveDefaultAttrs(....) { var producer_op_dict = new Dictionary<string, OpDef>(); producer_op_list.Op.Select(op => { producer_op_dict[op.Name] = op; return op; }).ToArray(); .... }Предупреждение анализатора: V3010 The return value of function 'ToArray' is required to be utilized. importer.cs 218 Анализатор считает вызов ToArray в этом месте подозрительным, так как значение, возвращаемое данной функцией, не присваивается переменной. Тем не менее, ошибкой такой код не является. Данная конструкция используется для заполнения словаря producer_op_dict значениями, соответствующими элементам списка producer_op_list.Op. Вызов ToArray необходим, чтобы функция, переданная в качестве аргумента метода Select, была вызвана для всех элементов коллекции.
На мой взгляд, код выглядит не лучшим образом. Заполнение словаря происходит несколько неочевидно, а у некоторых разработчиков может появиться желание убрать ?ненужный? вызов ToArray. Гораздо проще и понятнее было бы использовать здесь цикл foreach:
var producer_op_dict = new Dictionary<string, OpDef>(); foreach (var op in producer_op_list.Op) { producer_op_dict[op.Name] = op; }В этом случае код выглядит настолько простым, насколько это вообще возможно.
Ещё один подобный момент выглядит так:
public GraphDef convert_variables_to_constants(....) { .... inference_graph.Node.Select(x => map_name_to_node[x.Name] = x).ToArray(); .... }Предупреждение анализатора: V3010 The return value of function 'ToArray' is required to be utilized. graph_util_impl.cs 48 Единственное отличие состоит в том, что такая запись выглядит лаконичнее, но только соблазн убрать тут вызов ToArray никуда не пропадает, да и выглядит всё так же неочевидно.
Временное решение
public GraphDef convert_variables_to_constants(....) { .... var source_op_name = get_input_name(node); while(map_name_to_node[source_op_name].Op == "Identity") { throw new NotImplementedException); .... } .... }Предупреждение анализатора: V3020 An unconditional 'throw' within a loop. graph_util_impl.cs 73 В рассматриваемом проекте достаточно часто применяется следующий подход: если какое-то поведение должно быть реализовано позже, то в соответствующем месте выбрасывается NotImplementedException. Понятно, почему анализатор предупреждает о возможной ошибке в этом куске: использование while вместо if действительно выглядит не слишком разумно.
Это не единственное предупреждение, появляющееся по причине использования временных решений. Например, есть такой метод:
public static Tensor[] _SoftmaxCrossEntropyWithLogitsGrad( Operation op, Tensor[] grads ) { var grad_loss = grads[0]; var grad_grad = grads[1]; var softmax_grad = op.outputs[1]; var grad = _BroadcastMul(grad_loss, softmax_grad); var logits = op.inputs[0]; if(grad_grad != null && !IsZero(grad_grad)) // <= { throw new NotImplementedException("_SoftmaxCrossEntropyWithLogitsGrad"); } return new Tensor[] { grad, _BroadcastMul(grad_loss, -nn_ops.log_softmax(logits)) }; }Предупреждение анализатора: V3022 Expression 'grad_grad != null && !IsZero(grad_grad)' is always false. nn_grad.cs 93 По сути, исключение NotImplementedException("_SoftmaxCrossEntropyWithLogitsGrad") никогда не будет выброшено, так как этот код попросту недостижим. Для того, чтобы разгадать причину, необходимо перейти к коду функции IsZero:
private static bool IsZero(Tensor g) { if (new string[] { "ZerosLike", "Zeros" }.Contains(g.op.type)) return true; throw new NotImplementedException("IsZero"); }Метод либо возвращает true, либо выбрасывает исключение. Этот код не является ошибкой — очевидно, реализация здесь оставлена на потом. Главное, чтобы это ?потом? действительно настало. Что ж, весьма удачно сложилось, что PVS-Studio не позволит забыть о том, что тут присутствует такая вот недоделка :)
Is Tensor is Tensor?
private static Tensor[] _ExtractInputShapes(Tensor[] inputs) { var sizes = new Tensor[inputs.Length]; bool fully_known = true; for(int i = 0; i < inputs.Length; i++) { var x = inputs[i]; var input_shape = array_ops.shape(x); if (!(input_shape is Tensor) || input_shape.op.type != "Const") { fully_known = false; break; } sizes[i] = input_shape; } .... }Предупреждение анализатора: V3051 An excessive type check. The object is already of the 'Tensor' type. array_grad.cs 154 Тип возвращаемого значения метода shape – Tensor. Таким образом, проверка input_shape is Tensor выглядит по меньшей мере странно. Возможно, когда-то метод возвращал значение другого типа и проверка имела смысл, но также возможен и вариант, что вместо Tensor в условии должен быть указан какой-то наследник данного класса. Так или иначе, разработчику стоит обратить внимание на этот фрагмент.
Основательная проверка условия
public static Tensor[] _BaseFusedBatchNormGrad(....) { .... if (data_format == "NCHW") // <= throw new NotImplementedException(""); var results = grad_fun(new FusedBatchNormParams { YBackprop = grad_y, X = x, Scale = scale, ReserveSpace1 = pop_mean, ReserveSpace2 = pop_var, ReserveSpace3 = version == 2 ? op.outputs[5] : null, Epsilon = epsilon, DataFormat = data_format, IsTraining = is_training }); var (dx, dscale, doffset) = (results[0], results[1], results[2]); if (data_format == "NCHW") // <= throw new NotImplementedException(""); .... }Предупреждения анализатора:
- V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless nn_grad.cs 230
- V3022 Expression 'data_format == «NCHW»' is always false. nn_grad.cs 247
В отличие от некоторых из предыдущих примеров, тут с кодом явно что-то не так. Вторая проверка не имеет никакого смысла, так как если условие истинно, то до неё выполнение программы вообще не дойдёт. Возможно, здесь допущена какая-то опечатка, либо одна из проверок вообще лишняя.
Иллюзия выбора
public Tensor Activate(Tensor x, string name = null) { .... Tensor negative_part; if (Math.Abs(_threshold) > 0.000001f) { negative_part = gen_ops.relu(-x + _threshold); } else { negative_part = gen_ops.relu(-x + _threshold); } .... }Предупреждение анализатора: V3004 The 'then' statement is equivalent to the 'else' statement. gen_nn_ops.activations.cs 156 Достаточно занятная демонстрация эффективности использования статического анализа при разработке. Сложно придумать разумную причину, по которой разработчик написал именно этот код – скорее всего, это типичная copy-paste ошибка (хотя это, конечно, может быть очередной ?на потом?). Есть и другие фрагменты подобного плана, например:
private static Operation _GroupControlDeps( string dev, Operation[] deps, string name = null ) { return tf_with(ops.control_dependencies(deps), ctl => { if (dev == null) { return gen_control_flow_ops.no_op(name); } else { return gen_control_flow_ops.no_op(name); } }); }Предупреждение анализатора: V3004 The 'then' statement is equivalent to the 'else' statement. control_flow_ops.cs 135 Может быть, когда-то проверка имела смысл, но со временем он пропал, либо в будущем планируется внести какие-то дополнительные изменения. Тем не менее, ни тот, ни другой вариант не выглядит достаточным обоснованием для того, чтобы оставлять в коде нечто подобное, притом никак не объясняя данную странность. С большой долей вероятности здесь точно так же допущена ошибка copy-paste.
Запоздалая проверка
public static Tensor[] Input(int[] batch_shape = null, TF_DataType dtype = TF_DataType.DtInvalid, string name = null, bool sparse = false, Tensor tensor = null) { var batch_size = batch_shape[0]; var shape = batch_shape.Skip(1).ToArray(); // <= InputLayer input_layer = null; if (batch_shape != null) // <= .... else .... .... }Предупреждение анализатора: V3095 The 'batch_shape' object was used before it was verified against null. Check lines: 39, 42. keras.layers.cs 39 Классическая и достаточно опасная ошибка потенциального использования переменной, которая является ссылкой в никуда. При этом код явно подразумевает возможность, что в batch_shape будет null – это ясно и по списку аргументов, и по последующей проверке этой же переменной. Таким образом, анализатор здесь указывает на явную ошибку.
Очередной ?на потом??
public MnistDataSet( NDArray images, NDArray labels, Type dataType, bool reshape // <= ) { EpochsCompleted = 0; IndexInEpoch = 0; NumOfExamples = images.shape[0]; images = images.reshape( images.shape[0], images.shape[1] * images.shape[2] ); images = images.astype(dataType); // for debug np.multiply performance var sw = new Stopwatch(); sw.Start(); images = np.multiply(images, 1.0f / 255.0f); sw.Stop(); Console.WriteLine($"{sw.ElapsedMilliseconds}ms"); Data = images; labels = labels.astype(dataType); Labels = labels; }Предупреждение анализатора: V3117 Constructor parameter 'reshape' is not used. MnistDataSet.cs 15 Как и некоторые другие странности, эта, скорее всего, связана с тем, что функционал реализован далеко не полностью и вполне возможно, что параметр reshape в будущем будет как-то использоваться в этом конструкторе. Но пока что создаётся впечатление, будто его сюда пробрасывают просто так. Если это сделано действительно ?на потом?, то было бы разумно пометить это каким-нибудь комментарием. Если же нет, то получается, что коду, который конструирует объект, придётся пробрасывать в конструктор лишний параметр, и вполне возможна ситуация, когда удобнее было бы это не делать.
Неуловимый possible null dereference
public static Tensor[] _GatherV2Grad(Operation op, Tensor[] grads) { .... if((int)axis_static == 0) { var params_tail_shape = params_shape.slice(new NumSharp.Slice(start:1)); var values_shape = array_ops.concat( new[] { indices_size, params_tail_shape }, 0 ); var values = array_ops.reshape(grad, values_shape); indices = array_ops.reshape(indices, indices_size); return new Tensor[] { new IndexedSlices(values, indices, params_shape), // <= null, null }; } .... }Предупреждение анализатора: V3146 Possible null dereference of the 1st argument 'values' inside method. The '_outputs.FirstOrDefault()' can return default null value. array_grad.cs 199 Чтобы понять, в чём же тут проблема, стоит сначала обратиться к коду конструктора IndexedSlices:
public IndexedSlices( Tensor values, Tensor indices, Tensor dense_shape = null ) { _values = values; _indices = indices; _dense_shape = dense_shape; _values.Tag = this; // <= }Очевидно, что передача null в этот конструктор приведёт к выбрасыванию исключения. Однако почему анализатор считает, что в переменной values может содержаться null?
PVS-Studio использует технику Data-Flow Analysis (анализ потока данных), которая позволяет находить множества возможных значений переменных в различных частях кода. Предупреждение подсказывает, что null в указанную переменную может быть возвращён следующей строкой: _outputs.FirstOrDefault(). Однако взглянув на код выше, можно обнаружить, что значение переменной values получено вызовом array_ops.reshape(grad, values_shape). Так причём же тут тогда _outputs.FirstOrDefault()?
Дело в том, что при анализе потока данных рассматривается не только текущая функция, но и все вызываемые; при этом PVS-Studio получает информацию о множестве возможных значений любой переменной в любом месте. Таким образом, предупреждение означает, что реализация array_ops.reshape(grad, values_shape) содержит в себе вызов _outputs.FirstOrDefault(), результат работы которого в итоге и возвращается.
Чтобы проверить это, необходимо перейти к реализации reshape:
public static Tensor reshape<T1, T2>(T1 tensor, T2 shape, string name = null) => gen_array_ops.reshape(tensor, shape, null);Затем переходим к методу reshape, вызванному внутри:
public static Tensor reshape<T1, T2>(T1 tensor, T2 shape, string name = null) { var _op = _op_def_lib._apply_op_helper( "Reshape", name, new { tensor, shape } ); return _op.output; }Функция _apply_op_helper возвращает объект класса Operation, который содержит свойство output. Именно при получении его значения и происходит вызов кода, описанного в предупреждении:
public Tensor output => _outputs.FirstOrDefault();Tensor это, конечно же, ссылочный тип, поэтому default значением для него будет null. Из всего этого видно, что PVS-Studio дотошно анализирует логическую структуру кода, проникая глубоко внутрь структуры вызовов.
Анализатор выполнил свою работу, указав на потенциально проблемное место. Программисту остаётся проверить, возможно ли возникновение ситуации, когда элементы в _outputs будут отсутствовать.
Таким образом, статический анализ по крайней мере заставит разработчика обратить внимание на подозрительный фрагмент, чтобы оценить, действительно ли там может возникнуть ошибка. При таком подходе количество ошибок, остающихся незамеченными, будет стремительно сокращаться.
Ненадёжный waiting?
private (LoopVar<TItem>, Tensor[]) _BuildLoop<TItem>( .... ) where .... { .... // Finds the closest enclosing non-None control pivot. var outer_context = _outer_context; object control_pivot = null; while (outer_context != null && control_pivot == null) // <= { } if (control_pivot != null) { } .... }Предупреждение анализатора: V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. WhileContext.cs 212 Анализатор указывает, что такая реализация ожидания может быть подвергнута оптимизации компилятором, но я сомневаюсь, что тут действительно пытались реализовать waiting – скорее всего, код просто не дописан и в будущем планируется его доработка. Возможно, здесь стоит выбрасывать NotImplementedException, учитывая, что в других местах проекта используется данная практика. Так или иначе, я считаю, что тут явно не помешал бы какой-нибудь поясняющий комментарий.
Нарушение границ
public TensorShape(int[][] dims) { if(dims.Length == 1) { switch (dims[0].Length) { case 0: shape = new Shape(new int[0]); break; case 1: shape = Shape.Vector((int)dims[0][0]); break; case 2: shape = Shape.Matrix(dims[0][0], dims[1][2]); break; // <= default: shape = new Shape(dims[0]); break; } } else { throw new NotImplementedException("TensorShape int[][] dims"); } }Предупреждение анализатора: V3106 Possibly index is out of bound. The '1' index is pointing beyond 'dims' bound. TensorShape.cs 107 Среди просмотренных странных кусочков кода затесалась самая настоящая ошибка, которую весьма непросто заметить. Ошибочным здесь является следующий фрагмент: dims[1][2]. Получение элемента с индексом 1 из массива, состоящего из одного элемента, очевидно, является ошибкой. В то же время, если поменять фрагмент на dims[0][2], то появляется другая ошибка – получение элемента с индексом 2 из массива dims[0], длина которого в этой case-ветви равна 2. Таким образом, эта проблема оказалась как бы с «двойным дном».
В любом случае, данный фрагмент кода должен быть изучен и исправлен разработчиком. На мой взгляд, этот пример является отличной иллюстрацией эффективности работы Data Flow Analysis в PVS-Studio.
Очепятка?
private void _init_from_args(object initial_value = null, ....) // <= { var init_from_fn = initial_value.GetType().Name == "Func`1"; // <= .... tf_with(...., scope => { .... tf_with(...., delegate { initial_value = ops.convert_to_tensor( // <= init_from_fn ? (initial_value as Func<Tensor>)():initial_value, name: "initial_value", dtype: dtype ); }); _shape = shape ?? (initial_value as Tensor).TensorShape; _initial_value = initial_value as Tensor; // <= .... _dtype = _initial_value.dtype.as_base_dtype(); // <= if (_in_graph_mode) { .... if (initial_value != null) // <= { .... } .... } .... }); }Для понимания кода выше стоит также привести реализацию функции tf_with:
[DebuggerStepThrough] // with "Just My Code" enabled this lets the [DebuggerNonUserCode()] //debugger break at the origin of the exception public static void tf_with<T>( T py, Action<T> action ) where T : ITensorFlowObject { try { py.__enter__(); action(py); } finally { py.__exit__(); py.Dispose(); } }Предупреждение анализатора: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'initial_value', '_initial_value'. ResourceVariable.cs 137
_init_from_args — достаточно объёмная функция, поэтому многие фрагменты были опущены. Полностью её можно посмотреть, перейдя по ссылке. Хотя предупреждение поначалу не показалось мне действительно серьёзным, после изучения я осознал, что с кодом определённо что-то не так. Во-первых, нужно отметить, что метод может быть вызван без передачи параметров и по умолчанию в initial_value будет null. В таком случае исключение будет выброшено на первой же строке.
Во-вторых, проверка initial_value на неравенство null выглядит странно: если initial_value действительно стал равен null после вызова ops.convert_to_tensor, то и _initial_value был бы null, а значит, вызов _initial_value.dtype.as_base_dtype() тоже вызвал бы исключение.
Анализатор намекает, что возможно, нужно проверять на null именно _initial_value, но как было замечено выше, обращения к данной переменной происходят ещё до этой проверки, поэтому такой вариант тоже был бы некорректным.
Была бы замечена эта малюсенькая ошибка в такой гигантской функции без PVS-Studio? Очень сомневаюсь.
Заключение
В проекте с большим количеством примеров странного кода может скрываться немало проблем. Программист, привыкая видеть непонятное, перестаёт вместе с тем замечать и ошибки. Последствия могут быть весьма печальны. Действительно, среди предупреждений анализатора встречаются и ложные, однако в большинстве случаев предупреждения как минимум указывают на фрагменты кода, которые могут вызвать вопросы и при просмотре человеком. В случае, когда странный код написан намеренно, стоит оставлять пояснения, чтобы фрагмент был понятен разработчику, который будет работать с этим кодом в будущем (даже если это означает оставлять комментарии для самого себя).
В то же время, инструменты статического анализа, такие как PVS-Studio, могут стать отличными помощниками при поиске потенциальных ошибок и странностей, чтобы они были на виду и не забывались, а все временные решения впоследствии дорабатывались и превращались в чистый, структурированный и стабильно работающий код.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. How does strange code hide errors? TensorFlow.NET Analysis.