`M3-4-1`: Incorrectly computes scope for template variables and constexpr variables

[rak3-sh]

Affected rules

• M3-4-1

Description

M3-4-1 raises a false positive while computing the usage or the scope of variables that are template instantiations or constexpr.

Example

namespace a_namespace {

// array of unsigned int
constexpr static unsigned int a_constexpr_var{10U}; // constexpr variable in namespace 
static unsigned int a_namespace_var[a_constexpr_var]{}; // a_constexpr_var used in a variable definition

// uses a_namespace_var
constexpr static unsigned int a_namespace_function(void) noexcept {
  unsigned int a_return_value{0U};

  for (auto loop_var : a_namespace_var) {
    a_return_value += loop_var;
  }
  return a_return_value;
}

// uses a_constexpr_var and a_namespace_var
constexpr static unsigned int another_namespace_function(void) noexcept {
  unsigned int a_return_value{0U};

  for (unsigned int i{0U}; i < a_constexpr_var; i++) { // M3-4-1 reports a_constexpr_var
                                                       // can be moved to this scope but it cannot because
                                                       // it is also used in the definition of
                                                       // a_namespace_var at the
                                                       // namespace scope.
    a_return_value += a_namespace_var[i];
  }
  return a_return_value;
}
} // namespace a_namespace

int main() { return 0; }

[rak3-sh]

Fix Strategy (proposed)

In UnnecessaryExposedIdentifierDeclarationShared.qll, can we exclude variables coming from template instantiations and constexpr because their usages in the code can be folded away and CodeQL can miss its usage in its analysis. Following modification is proposed.

class CandidateDeclaration extends Declaration {
  CandidateDeclaration() {
    this instanceof LocalVariable
    or
    this instanceof GlobalOrNamespaceVariable
    and
    // <Fix starts>: Dont consider it as a candidate if its from a template instantiation or constexpr 
    not this.isFromTemplateInstantiation(_) and
    not this.(GlobalOrNamespaceVariable).isConstexpr()
    // <Fix ends>
    or
    this instanceof Type and
    not this instanceof ClassTemplateInstantiation and
    not this instanceof TemplateParameter
  }
}

It works on the offending example. However, should this be added to LocalVariable too?

[rak3-sh]

@lcartey : Kindly let me know your valuable comments.

[lcartey]

Thanks @rak3-sh!

I think it's reasonable to exclude constexpr variables from this query (both global/namespace and local), given that we cannot accurately determine whether those declaration are referred to in array size expressions, or as template arguments.

Can you provide a bit more explanation on the issues with template instantiations? I don't think the proposed change currently impacts results, because GlobalOrNamespaceVariables cannot be from instantiations in the first place.

[rak3-sh]

Thanks @lcartey! Basically the query shows the alerts on template variables inside Namespaces. In the alert the "from scope" is not computed well so it is not known but the "to scope" points to the specifc scope where it wants to move. But the problem is that the same template variable in a Namespace scope is being asked to be moved to multiple scopes which looks wrong. I will try to post a minimal example soon for that case.

[rak3-sh]

@lcartey : Thanks for your help and discussion on this. I'm adding an example where (from my understanding) NamespaceVariable could arise from template instantiations. Please let me know in case I've misunderstood.

namespace parent_namespace
{
  namespace child_namespace
  {
     template <typename From>
     class a_class_in_child_namespace {
       public:
       template <typename To>
       constexpr auto&& operator()(To&& val)const noexcept {
         return static_cast<To>(val);
       }
     }; // a_class_in_child_namespace end

     template <typename From>
     extern constexpr a_class_in_child_namespace<From> a_class_in_child_namespace_impl{};

  } // child_namespace end

  // M3-4-1 says to move the below declaration at multiple locations.
  template <typename From>
  static constexpr auto const& a_parent_namespace_variable = 
    child_namespace::a_class_in_child_namespace_impl<From>;

  namespace child_namespace2
  {
    class a_class
    {
      public:
      int func(...)
      {
        return 0;
      }
      void foo(int x)
      {
        x++;
      }
      template <typename F>
      constexpr auto bar(F (* func), int b)
      {
        // M3-4-1 says to move a_parent_namespace_variable here
        foo(func(a_parent_namespace_variable<F>(b)));
      }
    }; // a_class end
  } // child_namespace2 end

  class another_class
  {
    int a;
    int b;
    void bar(int param)
    {
    }

    bool has_value()
    {
      return a == b;
    }
    public:
    template <typename F>
    int foo(F (* func), int b)
    {
        if (has_value())
        {
          // M3-4-1 says to move a_parent_namespace_variable here as well
          bar(func(a_parent_namespace_variable<F>(b)));
        }
        return 0;
    } // foo end
  }; // another_class end
}// parent_namespace end

template <typename T>
T a_func(T v)
{
  return v++;
}

int main()
{
  parent_namespace::child_namespace2::a_class a_class_obj;
  a_class_obj.bar(a_func<int>,10);
  parent_namespace::another_class another_class_obj;
  another_class_obj.foo(a_func<int>,10);
}

In the above, M3-4-1 shows 2 alerts. One suggests to move a_parent_namespace_variable from parent_namespace to child_namespace2::a_class::bar scope and another alert to move it to parent_namespace::another_class::foo (inside the if block). This doesn't seem expected and can be fixed if we add the predicate isFromTemplateInstantiation(_) for GlobalOrNamespaceVariable and LocalVariable. Kindly let me know.

[lcartey]

Oh, I see, it's for variable templates - sorry, I missed that part!

I agree the query currently doesn't handle template variables correctly.

Variable templates and their instantiations should be considered together, rather than separately. So we should:

1. Exclude template instantiations from the candidate declarations, as you suggest.
2. Consider any accesses of the template instantiations to be accesses of the variable template itself.

We can achieve 2. by modifying the queries definition of declaration accesses:

newtype TDeclarationAccess =
  ObjectAccess(Variable v, VariableAccess va) {
    va = v.getAnAccess() or v.(TemplateVariable).getAnInstantiation().getAnAccess() = va
  } or
....

There's one further unique aspect to the variable templates, which is that they cannot be declared in local scope. We therefore need to modify the query to not report local block scopes as viable scopes for variable templates.

We can do this by modifying predicate possibleScopesForDeclaration to no longer report BlockStmts as viable scopes for variable templates:

  ...
  // Limit the best scope to block statements and namespaces or control structures
  (
    result instanceof BlockStmt and
    // Template variables cannot be in block scope
    not d instanceof TemplateVariable
    or
    result instanceof Namespace
  )
}

As template variables cannot appear as local variables, we don't need to worry about applying the isFromTemplateInstantiation(_) exclusion to them in the candidate declarations.

[rak3-sh]

@lcartey - thanks for your valuable insights. These comments are incorporated in PR #699