`M0-1-3`: Local variables that are used are reported as unused

[rak3-sh]

Affected rules

• M0-1-3

Description

Local variables that are used are reported as unused. Please refer to the example for the sample scenarios.

Example

int foo()
{
  constexpr int arrayDim=10; // 'arrayDim' reported unused
  static int array[arrayDim] {}; // 'arrayDim' is used here
  return array[4];
}

template<typename T>
static T template_function()
{
  return T();
}

template<typename T, typename First, typename... Rest>           
static T template_function(const First& first, const Rest&... rest) // 'rest' reported unused
{
  return first + template_function<T>(rest...); // 'rest' is used here
}

static int templ_fnc2() {
  return template_function<int>(1, 2, 3, 4, 5);
}

int main()
{
  return 0;
}

[rak3-sh]

Root Cause

1. The usage in array size is not considered in getUseCountConservatively function.
2. For the 'rest' variable, there are two instances of Variable objects that get generated - one of type Parameter and the other of type LocalVariable. The name and location of the definition for both these objects are same (as checked by getName() and getDefinitionLocation() functions), but the getAnAccess API returns an access for Parameter object but not for the LocalVariable object.

Fix Strategy

(1) Consider the usage in array size in the getUseCountConservatively function like the following:

int getUseCountConservatively(Variable v) {
  result =
    count(VariableAccess access | access = v.getAnAccess()) 
      +
      ... <existing logic snipped>
      +
      // In case an array type uses a constant in the same scope as the constexpr variable,
      // consider it as used.
      count(ArrayType at, LocalVariable arrayVariable |
        arrayVariable.getType().resolveTypedefs() = at and
        v.(PotentiallyUnusedLocalVariable).getFunction() = arrayVariable.getFunction() and
        at.getArraySize().toString() = getConstExprValue(v)
      )
}

(2) Consider checking the usage of Parameter objects that have an access and logically same as the PotentiallyUnusedLocalVariable in UnusedVariable.qll like the following:

class PotentiallyUnusedLocalVariable extends LocalVariable {
  PotentiallyUnusedLocalVariable() {
    // Ignore variables declared in macro expansions
    not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion()) 
    and
    ... <existing logic snipped>
    and
    not exists(Variable another |
      // Other variable has an access
      exists(another.getAnAccess()) and
      // Other variable has the same name
      another.hasName(this.getName()) and
      // Other variable is defined at the same location as this.
      another.getDefinitionLocation() = this.getDefinitionLocation()
    )
  }
}

[lcartey]

Thanks @rak3-sh!

I think 1. seems sensible to me.

For 2., I'd suggest applying some of the same restrictions as in 1. to ensure good performance:

• Use LocalVariable instead of Variable as the type.
• Restrict results to variables within the same function.

[rak3-sh]

Thanks @lcartey! Following are my observations for (2).

1. Using LocalVariable is not working because the other variant is of type Parameter which is at the same level in the type hierarchy as LocalVariable and hence the false positive on the variable rest in the example above is still reported. Instead, LocalScopeVariable seems to be a better option since both LocalVariable and Parameter are derived from LocalScopeVariable.

2. When I use LocalScopeVariable and use the getFunction to match the functions, the false positive on rest in the example above is still reported. I am guessing it uses a different Function object and hence cannot be matched (?) because the getFunction().toString() results in the same function name but the equals operator returns false.

The modified fix that works for (2) looks like the following:

class PotentiallyUnusedLocalVariable extends LocalVariable {
  PotentiallyUnusedLocalVariable() {
    // Ignore variables declared in macro expansions
    not exists(DeclStmt ds | ds.getADeclaration() = this and ds.isInMacroExpansion()) and
    ... <existing logic snipped >
    exists(Function f | f = getFunction() |
    ... <existing logic snipped >
    )
    and
    not exists(LocalScopeVariable another |
      another.getDefinitionLocation() = this.getDefinitionLocation()
      and
      another.hasName(this.getName())
      and
      exists(another.getAnAccess())
    )
  }
}

[rak3-sh]

Another observation is that if I modify getConstExprValue function to consider const variables as well, then the expected false positive in the query's test case will also be removed. I suppose its safe to do the following. Kindly let me know.

private string getConstExprValue(Variable v) {
  result = v.getInitializer().getExpr().getValue()
  and
  (
    // Check for const or constexpr. The test case has a const variable.
    v.isConst() or v.isConstexpr()
  )
}

[rak3-sh]

Candidate fix is submitted at PR #660.