Remove non-static fields from the initial store of a static method #216

AndrewShf · 2022-04-10T01:32:25Z

This PR is to fix issue #215

Below is the CFG graph produced by this branch.

As we can see, this.f has been removed from the initial store.

wmdietl

Thanks, just a small suggestion.

Separately, can you look whether store.insertValue can ever be called twice for the same field, first with the initializer and then with the declared type? The conditions don't seem to exclude this.

Should the first if also include varEle.getEnclosingElement().equals(classEle)? It would be more readable if that condition is extracted into another boolean with a more descriptive name.

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

AndrewShf · 2022-04-11T03:17:46Z

Thanks, just a small suggestion.

Separately, can you look whether store.insertValue can ever be called twice for the same field, first with the initializer and then with the declared type? The conditions don't seem to exclude this.

Should the first if also include varEle.getEnclosingElement().equals(classEle)? It would be more readable if that condition is extracted into another boolean with a more descriptive name.

For the first one, yes, the store.insertValue could be inserted twice. For example a field final String str = "hello"
(So, I guess I need to move my code before the first insertion)

For the second question, yes, I think so. But in what circumstance will the classEle not equal to varEle.getEnclosingElement()?

wmdietl · 2022-04-11T15:34:54Z

Re 1: Can you test what happens? I would assume the second insert overwrites the value from the first, making this rather pointless.

Re 2: Remove the check and see whether a test case fails. It might be related to nested classes and that the returned set of fields also contains fields from nested or enclosing classes that shouldn't be there. (Although, why shouldn't the store contain a field from an enclosing class? So maybe just nested classes should be excluded.)

AndrewShf · 2022-04-11T19:48:00Z

Re 1: Can you test what happens? I would assume the second insert overwrites the value from the first, making this rather pointless.

Re 2: Remove the check and see whether a test case fails. It might be related to nested classes and that the returned set of fields also contains fields from nested or enclosing classes that shouldn't be there. (Although, why shouldn't the store contain a field from an enclosing class? So maybe just nested classes should be excluded.)

Now I understand! the second insert with value declared is the Lhs and the first insert with value initializer is the Rhs. So if the second insert is triggered, then the second insert would be overwritten.
There's one scenario though: only the first insert is triggered, the second one in if and else branches are not triggered. That may due to this is a nested class (varEle.getEnclosingElement().equals(classEle)). Or the isNotFullyInitializedReceiver returns true in the if branch.

AndrewShf · 2022-04-18T01:49:45Z

After removing varEle.getEnclosingElement().equals(classEle), two nullness checks fail, one is issue354.java, another is issue904.java.
typetools#904
typetools#354

Is this because, this version inserts the field from enclosing class in store as initialized? (suppose to be under initialization)

wmdietl

Thanks!

wmdietl · 2022-04-20T01:28:02Z

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

        TypeElement classEle = TreeUtils.elementFromDeclaration(classTree);
        for (FieldInitialValue<V> fieldInitialValue : analysis.getFieldInitialValues()) {
            VariableElement varEle = fieldInitialValue.fieldDecl.getField();
+            boolean fieldNotFromEnclosingClass = varEle.getEnclosingElement().equals(classEle);


Should the Not be removed from the variable name?

If varEle.getEnclosingElement().equals(classEle) is true, varEle is in classEle, then this field is not from enclosing class right?

I'm not sure where the confusion is here. The enclosing element of varEle is some class. If that class is equal to classEle, then the field is directly within classEle.
What element do you mean with "the enclosing class"?

Meaning classEle is the nested class. The "enclosing class", encloses classEle. So if the field is directly within classEle, it's not from the enclosing class.
Yeah, the naming is a bit ambiguous. How about fieldNotFromOuterClass ?

It's usually easier to avoid negations. How about isFieldOfCurrentClass?

Yes, I agree!

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

Co-authored-by: Werner Dietl <wdietl@gmail.com> Co-authored-by: zcai <z44cai@uwaterloo.ca>

…sop#218)

wmdietl · 2022-04-21T00:32:22Z

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

        TypeElement classEle = TreeUtils.elementFromDeclaration(classTree);
        for (FieldInitialValue<V> fieldInitialValue : analysis.getFieldInitialValues()) {
            VariableElement varEle = fieldInitialValue.fieldDecl.getField();
+            boolean fieldNotFromEnclosingClass = varEle.getEnclosingElement().equals(classEle);


It's usually easier to avoid negations. How about isFieldOfCurrentClass?

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java

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

AndrewShf · 2022-04-21T20:07:58Z

I think this pull request is nearly done. During the discussion, we found one false negative, and I opened a new issue for this.
#226

wmdietl

Thanks for the improvements and cleanups!

fix issue 215 in eisop

545439e

AndrewShf changed the title ~~fix issue 215 in eisop~~ Remove non-static variable from initial store of a static method Apr 10, 2022

AndrewShf changed the title ~~Remove non-static variable from initial store of a static method~~ Remove non-static field variable from initial store of a static method Apr 10, 2022

AndrewShf changed the title ~~Remove non-static field variable from initial store of a static method~~ Remove non-static field variable from the initial store of a static method Apr 10, 2022

AndrewShf requested a review from wmdietl April 10, 2022 13:44

wmdietl reviewed Apr 10, 2022

View reviewed changes

framework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.java Outdated Show resolved Hide resolved

wmdietl changed the title ~~Remove non-static field variable from the initial store of a static method~~ Remove non-static fields from the initial store of a static method Apr 10, 2022

remove condition varEle.getEnclosingElement().equals(classEle)

9f8feb4

add varEle.getEnclosingElement().equals(classEle) back

250df1e

wmdietl reviewed Apr 20, 2022

View reviewed changes

AndrewShf and others added 7 commits April 20, 2022 17:18

Add UnusedAbstractValue and improve LiveVarAnalysis (eisop#203)

5334f0d

Co-authored-by: Werner Dietl <wdietl@gmail.com> Co-authored-by: zcai <z44cai@uwaterloo.ca>

Set -Anomsgtext by default for all tests (eisop#212)

2bd122b

Improve nullness value presentation (eisop#217)

58f5a90

Gradle task fastAssemble for executing assemble without Javadoc (ei…

96a591d

…sop#218)

reformat

7ab3e62

revert last change

4c89fe4

Merge branch 'master' into fix_initial_store

921bce0

wmdietl reviewed Apr 21, 2022

View reviewed changes

AndrewShf and others added 3 commits April 20, 2022 20:44

Improve documentation

319d09f

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

refine code style

fc43053

rename variable

d5a12d8

wmdietl approved these changes Apr 22, 2022

View reviewed changes

wmdietl merged commit 92d53e8 into eisop:master Apr 22, 2022

AndrewShf deleted the fix_initial_store branch September 17, 2022 23:29

wmdietl mentioned this pull request Feb 22, 2023

Dataflow Analysis adds "this.fieldValue" to initial store of a static method #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove non-static fields from the initial store of a static method #216

Remove non-static fields from the initial store of a static method #216

AndrewShf commented Apr 10, 2022 •

edited

Loading

wmdietl left a comment

AndrewShf commented Apr 11, 2022

wmdietl commented Apr 11, 2022

AndrewShf commented Apr 11, 2022 •

edited

Loading

AndrewShf commented Apr 18, 2022

wmdietl left a comment

wmdietl Apr 20, 2022

AndrewShf Apr 20, 2022

wmdietl Apr 20, 2022

AndrewShf Apr 20, 2022 •

edited

Loading

wmdietl Apr 21, 2022

AndrewShf Apr 21, 2022

wmdietl Apr 21, 2022

AndrewShf commented Apr 21, 2022

wmdietl left a comment

Remove non-static fields from the initial store of a static method #216

Remove non-static fields from the initial store of a static method #216

Conversation

AndrewShf commented Apr 10, 2022 • edited Loading

wmdietl left a comment

Choose a reason for hiding this comment

AndrewShf commented Apr 11, 2022

wmdietl commented Apr 11, 2022

AndrewShf commented Apr 11, 2022 • edited Loading

AndrewShf commented Apr 18, 2022

wmdietl left a comment

Choose a reason for hiding this comment

wmdietl Apr 20, 2022

Choose a reason for hiding this comment

AndrewShf Apr 20, 2022

Choose a reason for hiding this comment

wmdietl Apr 20, 2022

Choose a reason for hiding this comment

AndrewShf Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

wmdietl Apr 21, 2022

Choose a reason for hiding this comment

AndrewShf Apr 21, 2022

Choose a reason for hiding this comment

wmdietl Apr 21, 2022

Choose a reason for hiding this comment

AndrewShf commented Apr 21, 2022

wmdietl left a comment

Choose a reason for hiding this comment

AndrewShf commented Apr 10, 2022 •

edited

Loading

AndrewShf commented Apr 11, 2022 •

edited

Loading

AndrewShf Apr 20, 2022 •

edited

Loading