[Discussion] Reconsider the use of `assert` for input validation in our codebase

[zhangt2333]

Background

Java's assert keyword has specific intended use cases and runtime behavior that may not align with our current usage patterns in this repository.

Java assert Conventions and Behavior

According to Java best practices and Oracle documentation:

• Intended use: Internal invariant checking, debugging, and development-time verification
• Runtime behavior: Assertions are disabled by default and require explicit JVM flags (-ea/-enableassertions) to activate
• Purpose: Help developers catch bugs during development and testing, not for production error handling
• Expected outcome: When disabled (default), assert statements are completely ignored and have zero performance impact

Current Issues in Our Codebase

I've identified several instances where we're using assert for input validation and precondition checking in public APIs. This approach has several significant drawbacks:

1. Inconsistent behavior: Same input produces different results depending on JVM assertion settings
2. Silent failures: Invalid inputs may be silently accepted when assertions are disabled (default behavior)
3. Unclear error handling: Users don't know what exceptions to expect from our APIs
4. Testing gaps: Our tests might pass in development (with -ea) but fail silently in production

Examples in Our Codebase

Tai-e/src/main/java/pascal/taie/util/collection/AbstractHybridSet.java

Lines 167 to 169 in 12e7c43

	assert singleton == null;
	E e = CollectionUtils.getOne(c);
	singleton = Objects.requireNonNull(e, NULL_MESSAGE);

Tai-e/src/main/java/pascal/taie/analysis/bugfinder/nullpointer/IsNullConditionDecision.java

Lines 42 to 47 in 12e7c43

	public IsNullConditionDecision(
	If stmt, Var varTested,
	IsNullValue ifTrueDecision, IsNullValue ifFalseDecision) {
	assert varTested.getType() instanceof ReferenceType;
	assert !(ifTrueDecision == null && ifFalseDecision == null);
	this.conditionStmt = stmt;

Tai-e/src/main/java/pascal/taie/ir/exp/BitwiseExp.java

Lines 58 to 62 in 12e7c43

	@Override
	protected void validate() {
	assert (Exps.holdsInt(operand1) && Exps.holdsInt((operand2)) ||
	(Exps.holdsLong(operand1) && Exps.holdsLong(operand2)));
	}

Tai-e/src/main/java/pascal/taie/analysis/pta/plugin/reflection/MetaObjHelper.java

Lines 141 to 147 in 12e7c43

	Obj getMetaObjArray(Invoke invoke) {
	assert Set.of("getConstructors", "getDeclaredConstructors",
	"getMethods", "getDeclaredMethods",
	"getFields", "getDeclaredFields").contains(invoke.getMethodRef().getName());
	return heapModel.getMockObj(REFLECTION_ARRAY_DESC, invoke,
	invoke.getMethodRef().getReturnType(), invoke.getContainer());
	}

... more than 50 assert statements.

Proposed Solution

I suggest we replace assertion-based validation with explicit exception throwing:

Before (Current Approach)

public void processData(String input) {
    assert input != null : "Input cannot be null";
    assert !input.isEmpty() : "Input cannot be empty";
    // processing logic
}

After (Recommended Approach)

public void processData(String input) {
    if (input == null) {
        throw new IllegalArgumentException("Input cannot be null");
    }
    if (input.isEmpty()) {
        throw new IllegalArgumentException("Input cannot be empty");
    }
    // processing logic
}

Alternative: Utility Methods

public void processData(String input) {
    Objects.requireNonNull(input, "Input cannot be null");
    if (input.isEmpty()) {
        throw new IllegalArgumentException("Input cannot be empty");
    }
    // processing logic
}

Real-World Best Practices

• Spring Framework: Created its own org.springframework.util.Assert utility class that throws explicit exceptions (IllegalArgumentException, IllegalStateException) instead of using native assert keyword

• Google Guava: Developed Preconditions class with methods like checkArgument() and checkState() that throw IllegalArgumentException and IllegalStateException respectively, completely avoiding the native assert keyword

• Apache Commons: Provides Validate.isTrue() and similar methods that throw IllegalArgumentException for input validation, explicitly designed for cases where assertions would be inappropriate due to their ability to be disabled at runtime

Benefits

• ✅ Consistent behavior regardless of JVM settings
• ✅ Clear API contracts with documented exceptions
• ✅ Better error messages for library users
• ✅ Proper separation between debugging aids and production error handling

Discussion Points

• Should we establish coding guidelines or checkstyle rules about when to use assert vs explicit validation?
• Do we want to introduce a utility class (like Guava's Preconditions) for common validations?
• Are there any performance-critical paths where we might want to keep assertions for internal checks?

[zhangt2333]

@jjppp @silverbullettt

What are your thoughts on this approach? I'd love to hear your perspectives on how we should handle this refactoring.

Looking forward to the discussion! 🚀

[jjppp]

The idea sounds great.
I believe the proposed two styles can be done in a mixed manner: we can both (a) provide dedicated utilities for specific checks (such as simple checks for arguments or nullity), and (b) allow users to write their own checks (the validate() method for BitWiseExp).

[silverbullettt]

Insightful issue. Actually I wrote some assertions to prevent IDE warnings... Are you suggesting that we should replace all asserts for input validation with explicit assertions?

[zhangt2333]

Are you suggesting that we should replace all asserts for input validation with explicit assertions?

Yes, as I proposed in the issue description. However, I don't think this is a pressing issue. We can prioritize it after the new frontend to minimize merge conflicts when accepting PRs.