Add ignoreDeadCode option
#633
Conversation
There was a problem hiding this comment.
Thanks for starting work on this issue!
At the moment this isn't tested at all, so please do add a new test directory that enables the option.
When I tried it before, there was a problem with the Nullness Checker, so do use that.
Write tests e.g. like the one in the issue and things like an empty try block where an impossible catch block does something bad.
| @@ -150,6 +150,9 @@ | |||
| // org.checkerframework.framework.source.SourceChecker.report | |||
| "warns", | |||
|
|
|||
| // Make checker ignore the expression in dead branch | |||
| "notCheckDeadCode", | |||
There was a problem hiding this comment.
How about ignoreDeadCode to have more symmetry with other ignoreXXX options?
There was a problem hiding this comment.
By convention, for each option we list the main point where that option is used. Please add such a reference.
There was a problem hiding this comment.
By convention, for each option we list the main point where that option is used. Please add such a reference.
Thanks, already addressed the comment in 1d1c348
There was a problem hiding this comment.
How about
ignoreDeadCodeto have more symmetry with otherignoreXXXoptions?
addressed this comment in 58f4773
| } | ||
| }); | ||
| */ | ||
| if (checker.hasOption("notCheckDeadCode")) { |
There was a problem hiding this comment.
Here, and probably in the BaseTypeVisitor, put the option into a protected variable, so that we don't look up the option multiple times. See how other options are handled.
There was a problem hiding this comment.
Here, and probably in the
BaseTypeVisitor, put the option into a protected variable, so that we don't look up the option multiple times. See how other options are handled.
addressed this comment in 2540cd3
Thanks, I will add test cases later. |
There was a problem hiding this comment.
@Ao-senXiong Were there more tests you wanted to add? Can you wrap up this PR?
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
Outdated
Show resolved
Hide resolved
checker/src/main/java/org/checkerframework/checker/nullness/NullnessNoInitVisitor.java
Outdated
Show resolved
Hide resolved
checker/src/test/java/org/checkerframework/checker/test/junit/NullnessDeadBranchTest.java
Outdated
Show resolved
Hide resolved
checker/src/test/java/org/checkerframework/checker/test/junit/NullnessIgnoreDeadCodeTest.java
Outdated
Show resolved
Hide resolved
checker/src/test/java/org/checkerframework/checker/test/junit/NullnessIgnoreDeadCodeTest.java
Outdated
Show resolved
Hide resolved
| // :: error: (dereference.of.nullable) | ||
| obj.toString(); | ||
| } else { | ||
| // TODO: This is a dead branch should not issue error, the currently it does |
There was a problem hiding this comment.
As this is the point of the PR, we should fix this issue before merging.
framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java
Outdated
Show resolved
Hide resolved
framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
Outdated
Show resolved
Hide resolved
| obj.toString(); | ||
| // The following loop is dead code because the loop condition is false. | ||
| for (int i = 0; i < 0; i++) { | ||
| obj.toString(); |
There was a problem hiding this comment.
Also note that this also doesn't produce a warning before this PR - so does this PR do anything?
The deref on obj before is remembered and then we don't produce multiple errors. So, here and below, don't use the same variable outside the loop and inside.
There was a problem hiding this comment.
You should add the same test to the normal Nullness Checker tests and show the actual difference in behavior between the two.
There was a problem hiding this comment.
I see. I thought we want to enable the check first. I will write dead branch analysis in this PR. I think Jenny's thesis contains case study for dead branch analysis but not sure why it is not in the dataflow framework.
There was a problem hiding this comment.
What do you mean? The GenericAnnotatedTypeFactory#reachableNodes should already contain whether a node is reachable or not, right? So what additional analysis do you want to add? In #627 it sounds like all the infrastructure is already there, we just wanted to guard it by an option.
There was a problem hiding this comment.
I think this issue comes from when we fetch upstream changes. They only implement reachablenodes to determine whether it includes exception node and its Successors. See typetool#6246
There was a problem hiding this comment.
That upstream change is already in eisop. Are you saying there are other changes that have not been merged yet?
There was a problem hiding this comment.
Sorry, I am trying to say the upstream change is not enough to check dead branch for if else and while loop.
|
This PR opprop#153 seems quite relavent. |
Fixes #627
Do you need more test cases on this option?