Add `ignoreDeadCode` option by aosen-xiong · Pull Request #633 · eisop/checker-framework

aosen-xiong · 2023-11-25T14:00:27Z

Fixes #627

Do you need more test cases on this option?

wmdietl

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.

wmdietl · 2023-11-25T15:03:52Z

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

    "warns",

+    // Make checker ignore the expression in dead branch
+    "notCheckDeadCode",


How about ignoreDeadCode to have more symmetry with other ignoreXXX options?

By convention, for each option we list the main point where that option is used. Please add such a reference.

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

How about ignoreDeadCode to have more symmetry with other ignoreXXX options?

addressed this comment in 58f4773

wmdietl · 2023-11-25T15:05:31Z

framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java

-                                 }
-                             });
-        */
+        if (checker.hasOption("notCheckDeadCode")) {


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.

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

aosen-xiong · 2023-11-25T18:54:03Z

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.

Thanks, I will add test cases later.

wmdietl

@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

checker/tests/nullness-deadbranch/DeadBranchNPE.java

docs/CHANGELOG.md

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java

checker/src/main/java/org/checkerframework/checker/nullness/NullnessNoInitVisitor.java

checker/src/test/java/org/checkerframework/checker/test/junit/NullnessDeadBranchTest.java

docs/CHANGELOG.md

wmdietl

Thanks for the updates!

checker/src/test/java/org/checkerframework/checker/test/junit/NullnessIgnoreDeadCodeTest.java

wmdietl · 2024-12-31T18:02:08Z

checker/tests/nullness-ignoredeadcode/DeadBranchNPE.java

+            // :: error: (dereference.of.nullable)
+            obj.toString();
+        } else {
+            // TODO: This is a dead branch should not issue error, the currently it does


As this is the point of the PR, we should fix this issue before merging.

docs/manual/introduction.tex

framework/src/main/java/org/checkerframework/framework/source/SourceChecker.java

framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java

wmdietl · 2024-12-31T18:48:08Z

checker/tests/nullness-ignoredeadcode/DeadBranchNPE.java

+        obj.toString();
+        // The following loop is dead code because the loop condition is false.
+        for (int i = 0; i < 0; i++) {
+            obj.toString();


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.

You should add the same test to the normal Nullness Checker tests and show the actual difference in behavior between the two.

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.

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.

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

That upstream change is already in eisop. Are you saying there are other changes that have not been merged yet?

Sorry, I am trying to say the upstream change is not enough to check dead branch for if else and while loop.

Co-authored-by: Werner Dietl <wdietl@gmail.com>

aosen-xiong · 2024-12-31T21:53:39Z

This PR opprop#153 seems quite relavent.

wmdietl · 2025-01-05T21:58:33Z

So the only difference would be for this test.
It would make sense to have a copy of this test with this PR (and cross-reference the two files).

I'm not sure this option is particularly valuable at the moment... that TryCatch example does not seem worth adding an option for.

How about we convert this PR to a draft until it is more useful, by adding an actual dead code analysis?

wmdietl · 2026-03-23T13:57:22Z

A comparison to the approach in #101 would be good. I guess we only need one of the two PRs.

aosen-xiong and others added 2 commits November 25, 2023 08:58

initial commit for adding notcheckdeadcode option

7102456

Merge branch 'master' into add-not-check-dead-code-option

bd2e5e7

wmdietl requested changes Nov 25, 2023

View reviewed changes

wmdietl assigned aosen-xiong Nov 25, 2023

aosen-xiong and others added 5 commits November 25, 2023 13:27

change option name to ignoreCheckDeadCode

58f4773

add comment for entry point of the option

1d1c348

add protect variable of checker option

2540cd3

add * make javadoc happy

02c69a8

Merge branch 'master' into add-not-check-dead-code-option

a3e4013

add deadbranch test case and skip the test first

73de62f

wmdietl requested changes Jun 26, 2024

View reviewed changes

aosen-xiong and others added 6 commits December 16, 2024 17:59

Merge branch 'master' into add-not-check-dead-code-option

30638eb

Address all comments

6392013

Rename Junit test file

decd355

Use getter and revert access modifier for root

f7b1bb5

Comment first...

a93748b

Javadoc

1576672

aosen-xiong requested a review from wmdietl December 17, 2024 15:30

aosen-xiong assigned wmdietl and unassigned aosen-xiong Dec 17, 2024

Merge branch 'master' into add-not-check-dead-code-option

3571c39

wmdietl changed the title ~~Adding notcheckdeadcode option~~ Add ignoreDeadCode option Dec 31, 2024

Update CHANGELOG.md

fa183ec

wmdietl requested changes Dec 31, 2024

View reviewed changes

wmdietl assigned aosen-xiong and unassigned wmdietl Dec 31, 2024

wmdietl reviewed Dec 31, 2024

View reviewed changes

Wording

f678255

Co-authored-by: Werner Dietl <wdietl@gmail.com>

aosen-xiong added 5 commits January 4, 2025 21:53

Skip the test util we add dead branch analysis

2117ffc

Add test case for normal nullness checker

aff7485

Merge branch 'master' into add-not-check-dead-code-option

1508ca6

Fixes the test cases

bf1652c

Extra warning

85d3a9a

aosen-xiong requested a review from wmdietl January 5, 2025 04:28

aosen-xiong assigned wmdietl and unassigned aosen-xiong Jan 5, 2025

wmdietl assigned aosen-xiong and unassigned wmdietl Jan 5, 2025

aosen-xiong marked this pull request as draft January 5, 2025 22:01

aosen-xiong removed their assignment Jan 5, 2025

Conversation

aosen-xiong commented Nov 25, 2023

Uh oh!

wmdietl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aosen-xiong commented Nov 25, 2023

Uh oh!

wmdietl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wmdietl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aosen-xiong commented Dec 31, 2024

Uh oh!

wmdietl commented Jan 5, 2025

Uh oh!

wmdietl commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants