CodeQL queries 1.25
Skip to end of metadata
Go to start of metadata

Name: Useless assignment to local variable

Description: An assignment to a local variable that is not used later on, or whose value is always overwritten, has no effect.

ID: cs/useless-assignment-to-local

Kind: problem

Severity: warning

Precision: very-high

Query: DeadStoreOfLocal.ql
/**
 * @name Useless assignment to local variable
 * @description An assignment to a local variable that is not used later on, or whose value is always
 *              overwritten, has no effect.
 * @kind problem
 * @problem.severity warning
 * @id cs/useless-assignment-to-local
 * @tags maintainability
 *       external/cwe/cwe-563
 * @precision very-high
 */

import csharp

/**
 * Gets a callable that either directly captures local variable `v`, or which
 * is enclosed by the callable that declares `v` and encloses a callable that
 * captures `v`.
 */
Callable getACapturingCallableAncestor(LocalVariable v) {
  result = v.getACapturingCallable()
  or
  exists(Callable mid | mid = getACapturingCallableAncestor(v) |
    result = mid.getEnclosingCallable() and
    not v.getEnclosingCallable() = result
  )
}

Expr getADelegateExpr(Callable c) {
  c = result.(CallableAccess).getTarget()
  or
  result = c.(AnonymousFunctionExpr)
}

/**
 * Holds if `c` is a call where any delegate argument is evaluated immediately.
 */
predicate nonEscapingCall(Call c) {
  exists(string name | c.getTarget().hasName(name) |
    name = "ForEach" or
    name = "Count" or
    name = "Any" or
    name = "All" or
    name = "Average" or
    name = "Aggregate" or
    name = "First" or
    name = "Last" or
    name = "FirstOrDefault" or
    name = "LastOrDefault" or
    name = "LongCount" or
    name = "Max" or
    name = "Single" or
    name = "SingleOrDefault" or
    name = "Sum"
  )
}

/**
 * Holds if `v` is a captured local variable, and one of the callables capturing
 * `v` may escape the local scope.
 */
predicate mayEscape(LocalVariable v) {
  exists(Callable c, Expr e, Expr succ | c = getACapturingCallableAncestor(v) |
    e = getADelegateExpr(c) and
    DataFlow::localExprFlow(e, succ) and
    not succ = any(DelegateCall dc).getDelegateExpr() and
    not succ = any(Cast cast).getExpr() and
    not succ = any(Call call | nonEscapingCall(call)).getAnArgument() and
    not succ = any(AssignableDefinition ad | ad.getTarget() instanceof LocalVariable).getSource()
  )
}

class RelevantDefinition extends AssignableDefinition {
  RelevantDefinition() {
    this instanceof AssignableDefinitions::AssignmentDefinition
    or
    this instanceof AssignableDefinitions::MutationDefinition
    or
    this instanceof AssignableDefinitions::TupleAssignmentDefinition
    or
    // Discards in out assignments are only possible from C# 7 (2017), so we disable this case
    // for now
    //or
    //this.(AssignableDefinitions::OutRefDefinition).getTargetAccess().isOutArgument()
    this.(AssignableDefinitions::LocalVariableDefinition).getDeclaration() =
      any(LocalVariableDeclExpr lvde |
        lvde = any(SpecificCatchClause scc).getVariableDeclExpr()
        or
        lvde = any(ForeachStmt fs).getVariableDeclExpr() and
        not lvde.getName() = "_"
      )
    or
    this instanceof AssignableDefinitions::PatternDefinition
  }

  /** Holds if this assignment may be live. */
  private predicate isMaybeLive() {
    exists(LocalVariable v | v = this.getTarget() |
      // SSA definitions are only created for live variables
      this = any(Ssa::ExplicitDefinition ssaDef).getADefinition()
      or
      mayEscape(v)
    )
  }

  /** Holds if this definition is a variable initializer, for example `string s = null`. */
  private predicate isInitializer() {
    this.getSource() = this.getTarget().(LocalVariable).getInitializer()
  }

  /**
   * Holds if this definition is a default-like variable initializer, for example
   * `string s = null` or `int i = 0`, but not `string s = "Hello"`.
   */
  private predicate isDefaultLikeInitializer() {
    this.isInitializer() and
    exists(Expr e | e = this.getSource().stripCasts() |
      exists(string val | val = e.getValue() |
        val = "0" or
        val = "-1" or
        val = "" or
        val = "false"
      )
      or
      e instanceof NullLiteral
      or
      e =
        any(Field f |
          f.isStatic() and
          (f.isReadOnly() or f.isConst())
        ).getAnAccess()
      or
      e instanceof DefaultValueExpr
      or
      e instanceof AnonymousObjectCreation
    )
  }

  /** Holds if this definition is dead and we want to report it. */
  predicate isDead() {
    // Ensure that the definition is not in dead code
    exists(this.getAControlFlowNode()) and
    not this.isMaybeLive() and
    (
      // Allow dead initializer assignments, such as `string s = string.Empty`, but only
      // if the initializer expression assigns a default-like value, and there exists another
      // definition of the same variable
      this.isInitializer()
      implies
      (
        not this.isDefaultLikeInitializer()
        or
        not exists(AssignableDefinition other | other.getTarget() = this.getTarget() |
          other != this
        )
      )
    )
  }
}

from RelevantDefinition def, LocalVariable v
where
  v = def.getTarget() and
  def.isDead()
select def, "This assignment to $@ is useless, since its value is never read.", v, v.getName()

A value is assigned to a local variable, but either that variable is never read later on, or its value is always overwritten before being read. This means that the original assignment has no effect, and could indicate a logic error or incomplete code.

Recommendation

Ensure that you check the program logic carefully. If a value is really not needed, consider omitting the assignment. Be careful, though: if the right-hand side has a side effect (like performing a method call), it is important to keep this to preserve the overall behavior.

Example

The following example shows six different types of assignments to local variables whose value is not read:

  • In ParseInt, the result of the call to int.TryParse is assigned directly to the unread local variable success.
  • In IsDouble, the out argument of the call to int.TryParse is assigned to the unread local variable i.
  • In ParseDouble, the exception thrown by the call to double.Parse in case of parse failure is assigned to the unread local variable e.
  • In Count, the elements of ss are assigned to the unread local foreach variable s.
  • In IsInt, o is assigned (in case o is an integer) to the unread local type test variable i.
  • In IsString, o is assigned (in case o is a string) to the unread local type case variable s.

using System;

class Bad
{
    double ParseInt(string s)
    {
        var success = int.TryParse(s, out int i);
        return i;
    }

    bool IsDouble(string s)
    {
        var success = double.TryParse(s, out double i);
        return success;
    }

    double ParseDouble(string s)
    {
        try
        {
            return double.Parse(s);
        }
        catch (FormatException e)
        {
            return double.NaN;
        }
    }

    int Count(string[] ss)
    {
        int count = 0;
        foreach (var s in ss)
            count++;
        return count;
    }

    string IsInt(object o)
    {
        if (o is int i)
            return "yes";
        else
            return "no";
    }

    string IsString(object o)
    {
        switch (o)
        {
            case string s:
                return "yes";
            default:
                return "no";
        }
    }
}

The revised example eliminates the unread assignments.

using System;

class Good
{
    double ParseInt(string s)
    {
        int.TryParse(s, out int i);
        return i;
    }

    bool IsDouble(string s)
    {
        var success = double.TryParse(s, out _);
        return success;
    }

    double ParseDouble(string s)
    {
        try
        {
            return double.Parse(s);
        }
        catch (FormatException)
        {
            return double.NaN;
        }
    }

    int Count(string[] ss)
    {
        return ss.Length;
    }

    string IsInt(object o)
    {
        if (o is int)
            return "yes";
        else
            return "no";
    }

    string IsString(object o)
    {
        switch (o)
        {
            case string _:
                return "yes";
            default:
                return "no";
        }
    }
}

References