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

Name: Field is never assigned a non-null value

Description: A field that is never assigned a value (except possibly 'null') just returns the default value when it is read.

ID: java/unassigned-field

Kind: problem

Severity: warning

Precision: low

Query: NonAssignedFields.ql
/**
 * @name Field is never assigned a non-null value
 * @description A field that is never assigned a value (except possibly 'null') just returns the
 *              default value when it is read.
 * @kind problem
 * @problem.severity warning
 * @precision low
 * @id java/unassigned-field
 * @tags reliability
 *       maintainability
 *       useless-code
 *       external/cwe/cwe-457
 */

import java
import semmle.code.java.Reflection

/**
 * Holds if `c` is of the form `Class<T>`, where `t` represents `T`.
 */
predicate isClassOf(ParameterizedClass c, RefType t) {
  c.getGenericType() instanceof TypeClass and
  c.getATypeArgument().getSourceDeclaration() = t.getSourceDeclaration()
}

/**
 * Holds if field `f` is potentially accessed by an `AtomicReferenceFieldUpdater`.
 */
predicate subjectToAtomicReferenceFieldUpdater(Field f) {
  exists(Class arfu, Method newUpdater, MethodAccess c |
    arfu.hasQualifiedName("java.util.concurrent.atomic", "AtomicReferenceFieldUpdater") and
    newUpdater = arfu.getAMethod() and
    newUpdater.hasName("newUpdater") and
    c.getMethod().getSourceDeclaration() = newUpdater and
    isClassOf(c.getArgument(0).getType(), f.getDeclaringType()) and
    isClassOf(c.getArgument(1).getType(), f.getType()) and
    c.getArgument(2).(StringLiteral).getRepresentedString() = f.getName()
  )
}

/**
 * Holds if `f` is ever looked up reflectively.
 */
predicate lookedUpReflectively(Field f) {
  exists(MethodAccess getDeclaredField |
    isClassOf(getDeclaredField.getQualifier().getType(), f.getDeclaringType()) and
    getDeclaredField.getMethod().hasName("getDeclaredField") and
    getDeclaredField.getArgument(0).(StringLiteral).getRepresentedString() = f.getName()
  )
}

/**
 * Holds if `rt` registers a VM observer in its static initialiser.
 */
predicate isVMObserver(RefType rt) {
  exists(Method register |
    register.getDeclaringType().hasQualifiedName("sun.jvm.hotspot.runtime", "VM") and
    register.hasName("registerVMInitializedObserver") and
    register.getAReference().getEnclosingCallable().(StaticInitializer).getDeclaringType() = rt
  )
}

from Field f, FieldRead fr
where
  f.fromSource() and
  fr.getField().getSourceDeclaration() = f and
  not f.getDeclaringType() instanceof EnumType and
  forall(Assignment ae, Field g | ae.getDest() = g.getAnAccess() and g.getSourceDeclaration() = f |
    ae.getSource() instanceof NullLiteral
  ) and
  not exists(UnaryAssignExpr ua, Field g |
    ua.getExpr() = g.getAnAccess() and
    g.getSourceDeclaration() = f
  ) and
  not f.isFinal() and
  // Exclude fields that may be accessed reflectively.
  not reflectivelyWritten(f) and
  not lookedUpReflectively(f) and
  not subjectToAtomicReferenceFieldUpdater(f) and
  // If an object containing `f` is, or may be, passed to a native method,
  // assume it initializes the field.
  not exists(Callable c | c.isNative() |
    c.getAParameter().getType() = f.getDeclaringType() or
    c.getAReference().getAnArgument().getType() = f.getDeclaringType() or
    c.getDeclaringType() = f.getDeclaringType()
  ) and
  // Exclude special VM classes.
  not isVMObserver(f.getDeclaringType())
select f, "Field " + f.getName() + " never assigned non-null value, yet it is read at $@.", fr,
  fr.getFile().getStem() + ".java:" + fr.getLocation().getStartLine()

It is good practice to initialize every field in a constructor explicitly. A field that is never assigned any value (except possibly null) just returns the default value when it is read, or throws a NullPointerException.

Recommendation

A field whose value is always null (or the corresponding default value for primitive types, for example 0) is not particularly useful. Ensure that the code contains an assignment or initialization for each field. To help satisfy this rule, it is good practice to explicitly initialize every field in the constructor, even if the default value is acceptable.

If the field is genuinely never expected to hold a non-default value, check the statements that read the field and ensure that they are not making incorrect assumptions about the value of the field. Consider completely removing the field and rewriting the statements that read it, as appropriate.

Example

In the following example, the private field name is not initialized in the constructor (and thus is implicitly set to null), but there is a getter method to access it.

class Person {
    private String name;
    private int age;

    public Person(String name, int age) {
        this.age = age;
    }

    public String getName() {
        return name;
    }

    public int getAge() {
        return age;
    }
}

Therefore, the following code throws a NullPointerException:

Person p = new Person("Arthur Dent", 30);
int l = p.getName().length();

To fix the code, name should be initialized in the constructor by adding the following line: this.name = name;