Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding notcheckdeadcode option #633

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,11 @@ protected boolean commonAssignmentCheck(
/** Case 1: Check for null dereferencing. */
@Override
public Void visitMemberSelect(MemberSelectTree tree, Void p) {
// if (atypeFactory.isUnreachable(tree)) {
// return super.visitMemberSelect(tree, p);
// }
if (ignoreCheckDeadCode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make these two separate 'if's?

if (atypeFactory.isUnreachable(tree)) {
return super.visitMemberSelect(tree, p);
}
}
Element e = TreeUtils.elementFromUse(tree);
if (e.getKind() == ElementKind.CLASS) {
if (atypeFactory.containsNullnessAnnotation(null, tree.getExpression())) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.checkerframework.checker.test.junit;

import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest;
import org.junit.runners.Parameterized;

import java.io.File;
import java.util.List;

/**
* JUnit tests for the Nullness checker when -AignoreCheckDeadCode command-line argument is used.
*/
public class NullnessDeadBranchTest extends CheckerFrameworkPerDirectoryTest {
/**
* Create a NullnessNullMarkedTest.
*
* @param testFiles the files containing test code, which will be type-checked
*/
public NullnessDeadBranchTest(List<File> testFiles) {
super(
testFiles,
org.checkerframework.checker.nullness.NullnessChecker.class,
"nullness-deadbranch",
"-AignoreCheckDeadCode");
}

@Parameterized.Parameters
public static String[] getTestDirs() {
return new String[] {"nullness-deadbranch"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about nullness-ignoredeadcode to name the directory after the option?

}
}
31 changes: 31 additions & 0 deletions checker/tests/nullness-deadbranch/DeadBranchNPE.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// @skip-test until the bug is fixed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these tests not work?


class DeadBranchNPE {
void test1() {
Object obj = null;
if (true) {
// :: error: (dereference.of.nullable)
obj.toString();
} else {
obj.toString();
}
}

void test2() {
Object obj = null;
// :: error: (dereference.of.nullable)
obj.toString();
for (int i = 0; i < 0; i++) {
obj.toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment to each dead branch why they are dead.

}
}

void test3() {
Object obj = null;
// :: error: (dereference.of.nullable)
obj.toString();
while (obj != null) {
obj.toString();
}
}
}
3 changes: 3 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ Version 3.40.0-eisop3 (November ??, 2023)

**User-visible changes:**

Add a new command-line argument `-AignoreCheckDeadCode` disables the checker for code in dead expression.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you update the branch, make sure to move this to the next upcoming release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And try to make this sentence easier to read.

This option is not enabled by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add documentation in the manual, where we have a short paragraph for each option.


**Implementation details:**

**Closed issues:**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ public class BaseTypeVisitor<Factory extends GenericAnnotatedTypeFactory<?, ?, ?
/** True if "-AcheckEnclosingExpr" was passed on the command line. */
private final boolean checkEnclosingExpr;

/** True if "-AigoreCheckDeadCode" was passed on the command line. */
protected final boolean ignoreCheckDeadCode;

/** The tree of the enclosing method that is currently being visited. */
protected @Nullable MethodTree methodTree = null;

Expand Down Expand Up @@ -346,6 +349,7 @@ protected BaseTypeVisitor(BaseTypeChecker checker, @Nullable Factory typeFactory
checkCastElementType = checker.hasOption("checkCastElementType");
conservativeUninferredTypeArguments =
checker.hasOption("conservativeUninferredTypeArguments");
ignoreCheckDeadCode = checker.hasOption("ignoreCheckDeadCode");
}

/** An array containing just {@code BaseTypeChecker.class}. */
Expand Down Expand Up @@ -5181,10 +5185,12 @@ protected TypeValidator createTypeValidator() {
* @return true if checker should not test exprTree
*/
protected final boolean shouldSkipUses(ExpressionTree exprTree) {
// System.out.printf("shouldSkipUses: %s: %s%n", exprTree.getClass(), exprTree);
// if (atypeFactory.isUnreachable(exprTree)) {
// return true;
// }
if (ignoreCheckDeadCode) {
System.out.printf("shouldSkipUses: %s: %s%n", exprTree.getClass(), exprTree);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is debugging output and should remain a comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the debugging output shouldn't be guarded by the new condition.
You can combine the two 'if's into one check.

if (atypeFactory.isUnreachable(exprTree)) {
return true;
}
}
Element elm = TreeUtils.elementFromTree(exprTree);
return checker.shouldSkipUses(elm);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@
// org.checkerframework.framework.source.SourceChecker.report
"warns",

// Make checker ignore the expression in dead branch
// org.checkerframework.framework.common.basetype.BaseTypeVisitor.shouldSkipUses
"ignoreCheckDeadCode",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in #633 (comment) ignoreDeadCode would seem simpler.


///
/// More sound (strict checking): enable errors that are disabled by default
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,9 @@ public abstract class GenericAnnotatedTypeFactory<
*/
public final boolean hasOrIsSubchecker;

/** True if "-AigoreCheckDeadCode" was passed on the command line. */
protected final boolean ignoreCheckDeadCode;

/** An empty store. */
// Set in postInit only
protected Store emptyStore;
Expand Down Expand Up @@ -394,6 +397,7 @@ protected GenericAnnotatedTypeFactory(BaseTypeChecker checker, boolean useFlow)
hasOrIsSubchecker =
!this.getChecker().getSubcheckers().isEmpty()
|| this.getChecker().getParentChecker() != null;
ignoreCheckDeadCode = checker.hasOption("ignoreCheckDeadCode");

// Every subclass must call postInit, but it must be called after
// all other initialization is finished.
Expand Down Expand Up @@ -458,7 +462,9 @@ public void setRoot(@Nullable CompilationUnitTree root) {

super.setRoot(root);
this.scannedClasses.clear();
// this.reachableNodes.clear();
if (ignoreCheckDeadCode) {
this.reachableNodes.clear();
}
this.flowResult = null;
this.regularExitStores.clear();
this.exceptionalExitStores.clear();
Expand Down Expand Up @@ -1071,13 +1077,13 @@ public IPair<JavaExpression, String> getExpressionAndOffsetFromJavaExpressionStr
return value != null ? value.getAnnotations().iterator().next() : null;
}

/*
/**
* Returns true if the {@code exprTree} is unreachable. This is a conservative estimate and may
* return {@code false} even though the {@code exprTree} is unreachable.
*
* @param exprTree an expression tree
* @return true if the {@code exprTree} is unreachable
*
*/
public boolean isUnreachable(ExpressionTree exprTree) {
if (!everUseFlow) {
return false;
Expand All @@ -1096,7 +1102,6 @@ public boolean isUnreachable(ExpressionTree exprTree) {
// None of the corresponding nodes is reachable, so this tree is dead.
return true;
}
*/

/**
* Track the state of org.checkerframework.dataflow analysis scanning for each class tree in the
Expand All @@ -1112,15 +1117,15 @@ protected enum ScanState {
/** Map from ClassTree to their dataflow analysis state. */
protected final Map<ClassTree, ScanState> scannedClasses = new HashMap<>();

/*
/**
* A set of trees whose corresponding nodes are reachable. This is not an exhaustive set of
* reachable trees. Use {@link #isUnreachable(ExpressionTree)} instead of this set directly.
*
* <p>This cannot be a set of Nodes, because two LocalVariableNodes are equal if they have the
* same name but represent different uses of the variable. So instead of storing Nodes, it
* stores the result of {@code Node#getTree}.
*/
// private final Set<Tree> reachableNodes = new HashSet<>();
private final Set<Tree> reachableNodes = new HashSet<>();

/**
* The result of the flow analysis. Invariant:
Expand Down Expand Up @@ -1598,15 +1603,15 @@ protected void analyze(
boolean isStatic,
@Nullable Store capturedStore) {
ControlFlowGraph cfg = CFCFGBuilder.build(root, ast, checker, this, processingEnv);
/*
cfg.getAllNodes(this::isIgnoredExceptionType)
.forEach(
node -> {
if (node.getTree() != null) {
reachableNodes.add(node.getTree());
}
});
*/
if (ignoreCheckDeadCode) {
cfg.getAllNodes(this::isIgnoredExceptionType)
.forEach(
node -> {
if (node.getTree() != null) {
reachableNodes.add(node.getTree());
}
});
}
if (isInitializationCode) {
Store initStore = !isStatic ? initializationStore : initializationStaticStore;
if (initStore != null) {
Expand Down
Loading