Как странный код скрывает ошибки? Анализ TensorFlow.NET

    TensorFlow.NET и PVS-Studio

    Статический анализ – крайне полезный инструмент для любого разработчика, так как помогает вовремя отыскать не только ошибки, но и просто подозрительные и странные фрагменты кода, которые могут вызвать недоумение у программистов, которым пришлось бы работать с ним в будущем. Эту мысль продемонстрирует анализ открытого C# проекта TensorFlow.NET, разрабатываемый для работы с популярной библиотекой машинного обучения TensorFlow.

    Меня зовут Никита Липилин. Некоторое время назад я присоединился к отделу C# программистов компании PVS-Studio. Традиционно, все новички в команде пишут статьи, в которых рассматриваются результаты проверки различных открытых проектов с помощью статического анализатора PVS-Studio. Такие статьи помогают новым сотрудникам лучше познакомиться с продуктом, а заодно несут дополнительную пользу с точки зрения популяризации методологии статического анализа. Предлагаю вам для ознакомления мою первую работу по теме анализа открытых проектов.

    Введение


    Разнообразие возможных ошибок в программном коде поражает. Некоторые из них обнаруживают себя сразу, при поверхностном осмотре созданного приложения, другие бывает проблематично заметить даже при проведении code review командой опытных разработчиков. Однако бывает и так, что по невнимательности или какой-либо другой причине программист иногда пишет попросту странный и нелогичный код, который, тем не менее, (вроде бы) успешно выполняет свою функцию. Вот только далее, при возвращении к написанному, или при изучении этого кода другими, начинают возникать вопросы, на которые не найти ответа.

    Проведение рефакторинга старого кода может обернуться проблемами, особенно если от него зависят другие части программы, поэтому при обнаружении даже каких-то откровенно кривых конструкций часто применяется позиция ″Работает? Вот и не трогай! ″. Впоследствии это затрудняет изучение исходников, а следовательно, становится сложно расширять имеющиеся возможности. Кодовая база засоряется, появляется вероятность того, что не будет вовремя исправлена какая-то небольшая и незаметная, но потенциально весьма неприятная внутренняя проблема.

    В определённый момент последствия этой ошибки дадут о себе знать, но вот её поиск займёт уже много времени, ведь подозрения разработчика будут падать на огромное количество странных фрагментов кода, которые не были в своё время подвергнуты рефакторингу. Из этого следует вывод, что различные проблемы и странности в том или ином фрагменте должны исправляться непосредственно после его написания. В случае же, когда есть разумные причины оставить всё как есть (например, если код написан в качестве некоторой заготовки ″на потом″), такой фрагмент должен сопровождаться поясняющим комментарием.

    Стоит также заметить, что, независимо от квалификации разработчика, некоторые проблемные и просто неудачно реализованные моменты могут ускользнуть от его взгляда, а иной раз в каком-то месте может быть применено ″временное решение″, которое вскоре забывается и становится ″постоянным″. Впоследствии на разбор такого кода (скорее всего, этим будет заниматься вообще другой разработчик) будет потрачено непозволительно много сил.

    В таких случаях может помочь code review. Однако, если задача достаточно серьёзная, то этот вариант потребует большого количества времени. Кроме того, когда мелких ошибок или недочётов много, то за ними проверяющий вполне может не заметить ошибок высокого уровня. Проверка кода становится утомительной рутиной, и постепенно эффективность ревью падает.

    Очевидно, что рутинные задачи гораздо лучше будет переложить с человека на компьютер. Именно такой подход используется во многих направлениях современности. Автоматизация различного рода процессов – ключ к процветанию. Что же есть автоматизация в контексте рассматриваемой темы?

    Верным помощником при решении задачи написания разумного и стабильно работающего кода является статический анализ. Каждый раз перед отправкой результатов своей деятельности на ревью программист сможет провести автоматизированную проверку и не нагружать других разработчиков (да и себя самого) лишней работой. Код будет передаваться на проверку только после того, как все предупреждения анализатора учтены: ошибки исправлены, а странные моменты переписаны или хотя бы объяснены комментарием.

    Конечно, необходимость code review не исчезает, однако статический анализ дополняет и значительно упрощает его проведение. Достаточно большая часть ошибок будет исправлена благодаря анализатору, а странные моменты будут уже точно не забыты и соответствующим образом отмечены. Соответственно, при code review можно будет сосредоточиться на проверке корректности реализации сложных логических взаимодействий и поиске глубинных проблем, которые, к сожалению, (пока что) не могут быть выявлены анализатором.

    TensorFlow.NET




    Написание данной статьи вдохновлено проектом TensorFlow.NET. Он представляет собой реализацию возможности работы с функционалом популярной библиотеки машинного обучения TensorFlow через код C# (к слову, мы её тоже проверяли). Данная идея показалась достаточно интересной, ведь на момент написания статьи работа с библиотекой доступна только на Python, Java и Go.

    Исходный код, доступный на GitHub, постоянно дорабатывается и сейчас его объём составляет чуть больше ста тысяч строк. После поверхностного изучения возникло желание провести проверку с использованием статического анализа. В качестве конкретного инструмента был использован PVS-Studio, доказавший свою эффективность на достаточно большом количестве различных проектов.


    Для TensorFlow.NET анализатор отобразил ряд предупреждений: 39 уровня High, 227 уровня Medium и 154 уровня Low (об уровнях предупреждений можно прочитать здесь в подразделе «Уровни предупреждений и наборы диагностических правил»). Детальный разбор каждого из них сделал бы данную статью гигантской, поэтому ниже описаны лишь те из них, что показались мне наиболее интересными. Стоит также отметить, что некоторые найденные проблемы встречаются в проекте по несколько раз, и разбор каждого такого куска кода выходит за рамки данной статьи.

    Проект ставит перед собой достаточно серьёзную задачу, и, к сожалению, появление различного рода странных фрагментов кода неизбежно. В данной статье я постараюсь показать, что использование статического анализа может значительно упрощать работу программистам, указывая на участки, которые могут вызвать вопросы. Не всегда предупреждение свидетельствует об ошибке, но достаточно часто оно указывает на код, который бы вызвал вопросы и у человека. Соответственно, значительно повышается вероятность того, что странный код будет либо переработан, либо во всяком случае прокомментирован соответствующим образом.

    Фрагменты, привлекшие внимание при изучении отчёта анализатора


    На самом деле, довольно большое количество предупреждений анализатора для этого проекта можно назвать не столько ошибками, сколько именно странным кодом. При просмотре строк, для которых выдаётся предупреждение, возникает по меньшей мере недоумение. Конечно, некоторые из представленных ниже примеров, вероятно, являются ″временными решениями″, вот только каких-либо комментариев по этому поводу не даётся, а значит, у того, кто будет работать с этим кодом в будущем, возникнут вопросы, поиск ответов на которые займёт дополнительное время.


    В то же время, некоторые предупреждения указывают на код, который очевидно является не просто странным, а именно ошибочным. В этом и состоит главная опасность ″странного кода″ — крайне сложно заметить реальную ошибку, если тут и там видишь непонятные решения и постепенно привыкаешь к тому, что код кажется неправильным.

    Изощрённый обход коллекции


    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.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Похожие публикации

    Комментарии 11

      +5
      Многие случаи «странного» кода в этом проекте поясняются тем, что код пишется девелоперами оригинального Tensorflow, и многие методы буквально «переводятся» с Python.
      Отсюда заполнения словарей через LINQ — автор кода явно пытался эмулироваться питоновские генераторы, хотя на C# это выглядит крайне неинтуитивно.
      Отсюда же и проверка на is Tensor — в аналогичном методе в питоновской реализации return type неизвестен, потому что в коде Tensorflow нет статических тайп хинтов.

      Пожалуй, в подобных проектах (где разработчикам приходится писать не на своём «естественном» языке программирования) статический анализ кода особенно важен, потому что помимо контроля качества кода он ещё и помогает разработчику выучить новую для него экосистему.
        0

        Ха. Если бы это были девелоперы оригинального TensorFlow. Это просто несколько человек, копипастящих код из TensorFlow и переводящих на C#. Когда они бросают NotImplementedException — это ещё по-божески. Хуже когда оставляют пустой метод или пропускают override. Там безопасно можно пользоваться только частью, напрямую сгенерированной их TF C API, к которой tf.keras, к примеру, не относится.

        +2

        Это какая-то жесть. Как-будто пригнали питонистов писать на шарпе без подготовки.
        Мало того что для словарей есть специальный инструмент LINQ:


        var producer_op_dict = producer_op_list.Op.ToDictionary(op => op.Name, op => op);

        Вот эта запись просто убила:


        var init_from_fn = initial_value.GetType().Name == "Func`1"; // <=

        Кажется я первый раз вижу проверку типа по имени в не учебном, не тестовом коде. Непонятно что мешало написать:


        var init_from_fn = initial_value?.GetType() is Type t 
             && t.IsGenericType 
             && t.GetGenericTypeDefinition() == typeof(Func<>);

        Можно проверить initial_value на is Delegate сначала, чтобы различить ицнициализацию по значению и с помощью функции.

          +2

          Уточнение. При создании словаря второй аргумент не нужен:


          var producer_op_dict = producer_op_list.Op.ToDictionary(op => op.Name);
            +2

            Я пропустил вот эту строчку


            init_from_fn ? (initial_value as Func<Tensor>)():initial_value,

            По всей видимости, ожидается либо Func<Tensor>, либо Tensor. В таком случае подошел бы стандартный:


            var initialValue = initial_value switch 
            {
                Tensor tensorVal => tensorVal,
                Func<Tensor> initializer => initializer(),
                _ => throw new ArgumentException(nameof(initialValue))
            };

            На самом же деле нужен перегруженный метод, принимающиу Func<Tensor>, вычисляющий его, и вызывающий метод, принимающий просто Tensor, где и просходит вся работа.


            Сложно представить сколько еще всего там просто так упаковывается/распаковывается и таскается в object-ах. Расчитывать на какой-то перформанс в .NET-части либы вообще не стоит.


            А потом нам говорят что "статическая типизация не нужна", "с динамической типизацией проще, раз-раз и проект готов".

              +1
              А потом нам говорят что «статическая типизация не нужна», «с динамической типизацией проще, раз-раз и проект готов».
              Динамическая типизация — мечта менеджера. Таски закрываются в три раза быстрее, а что их общее количество раз в десять больше — так зато можно вышестоящему менеджменту показывать, что вы делом заняты и красивые графики рисовать…
                +1

                Не, я не против динамической типизации, сам пишу на R, и это очень удобно для коротких проектов/высокоуровневых задач, когда вам нужно условно CSV.LoadData, TensorFlow.MakeEverythingWork. Но когда пишется большая либа/обертка к performance-critical бэкенду — тут хочется нормальной доменной модели, а не вот этих вот гаданий по строке, какой же тип прилетел к нам в object.

                  –1
                  Не, я не против динамической типизации
                  Извините, но вы как раз против динамической типизации. Более того, подавляющее большинство людей, которые «топят» за динамическую типизацию — хотят вовсе не её.

                  тут хочется нормальной доменной модели, а не вот этих вот гаданий по строке, какой же тип прилетел к нам в object.
                  Но ведь в этом — вся суть динамической типизации! Весь её смысл! Когда у вас в одной переменной может быть и число и строка и, я извиняюсь, Жопа-С-Ручкой!

                  То, чего вы на самом деле хотите — это как раз статическая типизация… только без писанины. Ну вот как в Go:
                    s := "Hello, world"
                  
                  Статическая тут типизация или динамическая? Статическая, конечно. Ничего, кроме строки вы в s уже никогда не положите.

                  Но объявлять тип перемнной тут не нужно — если его можно вычислить.

                  Так многие языки умеют. Даже C++ современный.

                  Просто динамически типизированные языки предоставляют такую возможность всегда… а языках со статической типизацией — есть ньюансы.

                  Но это — именно причина из-за которой Роб Пайк, внезапно, обнаружил, что люди активно переходят с Python на Go, а с C++… не очень.

                  Забавно, кстати, что современный Python обзавёлся-таки типами — что и показывает, что даже большинству людей, использующих его хочется-таки статической типизации.
                    0

                    За всех не скажу, но я хочу не "статическую типизацию без писанины", а нечто другое. Я и сейчас могу написать var s = "Hello, world!";, другое дело что в текущем скоупе (области видимости) s может содержать инстанс типа string или наследуемого типа (если таковые имеются), а не, внезапно, (int)42.


                    Для меня статическая типизация это в первую очередь контракт. Если у меня есть int GetStrLength(string @string), и я его вызову используя переменную s (var len = GetStrLength(s);), я асболютно уверен, что внутрь функции я передам либо строку, либо null, а из функции вернется знаковое целое размером 4 байта; или все это упадет с исключением. Более того, не ломая CLR, без грязных трюков это ограничение обойти невозможно (или мне неизвестно как). Именно ради этих контрактов и строятся все эти слоищи абстракций и тратится столько времени по сравнению с разработкой на языке с динамической типизацией.


                    тут хочется нормальной доменной модели, а не вот этих вот гаданий по строке, какой же тип прилетел к нам в object.

                    Но ведь в этом — вся суть динамической типизации! Весь её смысл! Когда у вас в одной переменной может быть и число и строка и, я извиняюсь, Жопа-С-Ручкой!


                    И это снова контракт, который всего лишь означает, что в compile-time допускается вызов этого метода с аргументом любого типа. И такой контракт используется повсеместно, например, в обработке событий. Легко представить типичный обработчик вида void EventFired(object sender, EventArgs e). И если sender у вас может быть любой, то подписаться на событие, где вторым аргументом передается не каноничный EventArgs, а, например, строка — уже не получится. И об этом вы узнаете в момент написания строчки кода в IDE или в момент компиляции — благодаря статической проверке типов. В случае же с динамической, узнаете вы об этом печальном type mismatch в продакшене, когда вам в метод прилетит строка, и вы попытаетесь вызвать на ней метод, доступный только на EventArgs.


                    Справедливости ради, в C# есть "настоящая" динамическая типизация, и это dynamic, который на самом деле object, но с дополнительными инструментами.

                0
                Перегрузка плачет по этому коду…
              0
              Предупреждение анализатора: V3051 An excessive type check. The object is already of the 'Tensor' type. array_grad.cs 154

              Если он null, то он не Tensor (если это не структура).

              Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

              Самое читаемое