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

Scan through members in InitializationFieldAccessVisitor #619

Merged
merged 7 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ protected InitializationAnalysis createFlowAnalysis() {
@Override
protected void performFlowAnalysis(ClassTree classTree) {
// Only perform the analysis if initialization checking is turned on.
if (!checker.hasOption("assumeInitialized")) {
if (!assumeInitialized) {
super.performFlowAnalysis(classTree);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
*/
public class InitializationFieldAccessTreeAnnotator extends TreeAnnotator {

/** The value of the assumeInitialized option. */
protected final boolean assumeInitialized;

/**
* Creates a new CommitmentFieldAccessTreeAnnotator.
*
Expand All @@ -34,6 +37,7 @@ public class InitializationFieldAccessTreeAnnotator extends TreeAnnotator {
public InitializationFieldAccessTreeAnnotator(
GenericAnnotatedTypeFactory<?, ?, ?, ?> atypeFactory) {
super(atypeFactory);
assumeInitialized = atypeFactory.getChecker().hasOption("assumeInitialized");
}

@Override
Expand Down Expand Up @@ -62,7 +66,7 @@ private void computeFieldAccessType(ExpressionTree tree, AnnotatedTypeMirror typ
(GenericAnnotatedTypeFactory<?, ?, ?, ?>) atypeFactory;

// Don't adapt anything if initialization checking is turned off.
if (factory.getChecker().hasOption("assumeInitialized")) {
if (assumeInitialized) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,17 @@
public class InitializationFieldAccessVisitor
extends BaseTypeVisitor<InitializationFieldAccessAnnotatedTypeFactory> {

/** The value of the assumeInitialized option. */
private final boolean assumeInitialized;

/**
* Create an InitializationFieldAccessVisitor.
*
* @param checker the initialization field-access checker
*/
public InitializationFieldAccessVisitor(BaseTypeChecker checker) {
super(checker);
assumeInitialized = checker.hasOption("assumeInitialized");
}

@Override
Expand All @@ -24,6 +28,9 @@ public void processClassTree(ClassTree classTree) {
// and InitializationChecker, this checker performs the flow analysis
// (which is handled in the BaseTypeVisitor), but does not perform
// any type checking.
// Thus, this method does nothing.
// Thus, this method does nothing but scan through the members.
if (!assumeInitialized) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@flo2702 This is the key change in this PR. Sorry for adding so many other changes while tracking this down.
I think I'll merge this PR now, to fix the exception. Can you open a new PR if you find a better fix?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. I'll keep looking, but haven't found anything better.

scan(classTree.getMembers(), null);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ public abstract class InitializationParentAnnotatedTypeFactory
/** The UnknownInitialization.value field/element. */
protected final ExecutableElement unknownInitializationValueElement;

/** The value of the assumeInitialized option. */
protected final boolean assumeInitialized;

/**
* Create a new InitializationParentAnnotatedTypeFactory.
*
Expand All @@ -131,6 +134,8 @@ public InitializationParentAnnotatedTypeFactory(BaseTypeChecker checker) {
TreeUtils.getMethod(UnderInitialization.class, "value", 0, processingEnv);
unknownInitializationValueElement =
TreeUtils.getMethod(UnknownInitialization.class, "value", 0, processingEnv);

assumeInitialized = checker.hasOption("assumeInitialized");
}

@Override
Expand Down Expand Up @@ -233,7 +238,7 @@ public AnnotationMirror createUnderInitializationAnnotation(TypeMirror typeFrame
public @Nullable AnnotatedDeclaredType getSelfType(Tree tree) {
AnnotatedDeclaredType selfType = super.getSelfType(tree);

if (checker.hasOption("assumeInitialized")) {
if (assumeInitialized) {
return selfType;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ public class InitializationVisitor extends BaseTypeVisitor<InitializationAnnotat
/** List of fields in the current compilation unit that have been initialized. */
protected final List<VariableTree> initializedFields;

/** The value of the assumeInitialized option. */
protected final boolean assumeInitialized;

/**
* Create an InitializationVisitor.
*
Expand All @@ -80,6 +83,7 @@ public class InitializationVisitor extends BaseTypeVisitor<InitializationAnnotat
public InitializationVisitor(BaseTypeChecker checker) {
super(checker);
initializedFields = new ArrayList<>();
assumeInitialized = checker.hasOption("assumeInitialized");
}

@Override
Expand All @@ -90,7 +94,7 @@ protected InitializationAnnotatedTypeFactory createTypeFactory() {
@Override
public void visit(TreePath path) {
// This visitor does nothing if init checking is turned off.
if (!checker.hasOption("assumeInitialized")) {
if (!assumeInitialized) {
super.visit(path);
}
}
Expand Down
48 changes: 24 additions & 24 deletions checker/tests/nullness-extra/issue5174/Issue5174.out
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,15 @@ Before: InitializationStore#19(
3 -> 0

2:
Before: NullnessNoInitStore#26(
Before: NullnessNoInitStore#27(

isPolyNullNonNull = false
isPolyNullNull = false)
~~~~~~~~~
<entry>

3:
Before: NullnessNoInitStore#26(
Before: NullnessNoInitStore#27(

isPolyNullNonNull = false
isPolyNullNull = false)
Expand All @@ -133,7 +133,7 @@ Before: NullnessNoInitStore#26(
(this).sf = "" [ Assignment ] > NV{@NonNull, String, poly nn/n=f/f}

0:
Before: NullnessNoInitStore#27(
Before: NullnessNoInitStore#28(
Issue5174Super<S>.sf > NV{@NonNull, String, poly nn/n=f/f}
isPolyNullNonNull = false
isPolyNullNull = false)
Expand All @@ -143,7 +143,7 @@ Before: NullnessNoInitStore#27(
8 -> 5

7:
Before: NullnessNoInitStore#31(
Before: NullnessNoInitStore#32(
in > NV{, S, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
this.f > NV{, S, poly nn/n=f/f}
Expand All @@ -153,7 +153,7 @@ Before: NullnessNoInitStore#31(
<entry>

8:
Before: NullnessNoInitStore#31(
Before: NullnessNoInitStore#32(
in > NV{, S, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
this.f > NV{, S, poly nn/n=f/f}
Expand All @@ -164,7 +164,7 @@ in [ LocalVariable ] > NV{, S, poly nn/n=f/f}
return in [ Return ] > NV{@NonNull, boolean, poly nn/n=f/f}

5:
Before: NullnessNoInitStore#32(
Before: NullnessNoInitStore#33(
in > NV{, S, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
this.f > NV{, S, poly nn/n=f/f}
Expand All @@ -180,7 +180,7 @@ Before: NullnessNoInitStore#32(
16 -> 11

13:
Before: NullnessNoInitStore#36(
Before: NullnessNoInitStore#37(
f > NV{, S, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
isPolyNullNonNull = false
Expand All @@ -189,7 +189,7 @@ Before: NullnessNoInitStore#36(
<entry>

14:
Before: NullnessNoInitStore#36(
Before: NullnessNoInitStore#37(
f > NV{, S, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
isPolyNullNonNull = false
Expand All @@ -199,7 +199,7 @@ Before: NullnessNoInitStore#36(
(this).<init> [ MethodAccess ] > NV{@NonNull, Issue5174Super, poly nn/n=f/f}

15:
Before: NullnessNoInitStore#37(
Before: NullnessNoInitStore#38(
f > NV{, S, poly nn/n=f/f}
this > NV{@NonNull, Issue5174Super, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
Expand All @@ -209,7 +209,7 @@ Before: NullnessNoInitStore#37(
(this).<init>() [ MethodInvocation ] > NV{@NonNull, Object, poly nn/n=f/f}

16:
Before: NullnessNoInitStore#38(
Before: NullnessNoInitStore#39(
f > NV{, S, poly nn/n=f/f}
this > NV{@NonNull, Issue5174Super, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
Expand All @@ -225,7 +225,7 @@ this.f = f [ Assignment ] > NV{, S, poly nn/n=f/f}
expression statement this.f = f [ ExpressionStatement ]

12:
Before: NullnessNoInitStore#39(
Before: NullnessNoInitStore#40(
f > NV{, S, poly nn/n=f/f}
this > NV{@NonNull, Issue5174Super, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
Expand All @@ -235,7 +235,7 @@ Before: NullnessNoInitStore#39(
<exceptional-exit>

11:
Before: NullnessNoInitStore#42(
Before: NullnessNoInitStore#43(
f > NV{, S, poly nn/n=f/f}
this > NV{@NonNull, Issue5174Super, poly nn/n=f/f}
Issue5174Super<S>.sf > NV{@NonNull, Object, poly nn/n=f/f}
Expand Down Expand Up @@ -413,12 +413,12 @@ Before: InitializationStore#93(
~~~~~~~~~
o [ VariableDeclaration ]
(this) [ ImplicitThis ]
(this).f [ FieldAccess ] > CFAV{@Initialized, Object}
o = (this).f [ Assignment ] > CFAV{@Initialized, Object}
(this).f [ FieldAccess ] > CFAV{, T}
Copy link
Member Author

Choose a reason for hiding this comment

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

@flo2702 These changes come a bit as a surprise, but look good to me - field f has type T, so the new types look more precise than before, where it used the erasure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like scanning through all members once in the visitor somehow allows more precise types to be used during the dataflow refinement. Probably something to consider while I look for a better fix, but I admit that I don't really understand how the visitor affects the refinement in this way.

o = (this).f [ Assignment ] > CFAV{, T}

39:
Before: InitializationStore#94(
o > CFAV{@Initialized, Object}
o > CFAV{, T}
initialized fields = [])
~~~~~~~~~
<exit>
Expand All @@ -437,12 +437,12 @@ Before: InitializationStore#98(
~~~~~~~~~
o [ VariableDeclaration ]
this [ ExplicitThis ] > CFAV{@Initialized, Issue5174Sub}
this.f [ FieldAccess ] > CFAV{@Initialized, Object}
o = this.f [ Assignment ] > CFAV{@Initialized, Object}
this.f [ FieldAccess ] > CFAV{, T}
o = this.f [ Assignment ] > CFAV{, T}

44:
Before: InitializationStore#99(
o > CFAV{@Initialized, Object}
o > CFAV{, T}
initialized fields = [])
~~~~~~~~~
<exit>
Expand Down Expand Up @@ -739,12 +739,12 @@ Before: InitializationStore#203(
~~~~~~~~~
o [ VariableDeclaration ]
(this) [ ImplicitThis ]
(this).f [ FieldAccess ] > CFAV{@Initialized, Object}
o = (this).f [ Assignment ] > CFAV{@Initialized, Object}
(this).f [ FieldAccess ] > CFAV{, T}
o = (this).f [ Assignment ] > CFAV{, T}

87:
Before: InitializationStore#204(
o > CFAV{@Initialized, Object}
o > CFAV{, T}
initialized fields = [])
~~~~~~~~~
<exit>
Expand Down Expand Up @@ -795,17 +795,17 @@ Before: InitializationStore#211(
Before: InitializationStore#218(
initialized fields = [])
~~~~~~~~~
Issue5174Sub.this.f [ FieldAccess ] > CFAV{@Initialized, Object}
Issue5174Sub.this.f [ FieldAccess ] > CFAV{, T}

101:
Before: InitializationStore#221(
initialized fields = [])
~~~~~~~~~
o = Issue5174Sub.this.f [ Assignment ] > CFAV{@Initialized, Object}
o = Issue5174Sub.this.f [ Assignment ] > CFAV{, T}

92:
Before: InitializationStore#224(
o > CFAV{@Initialized, Object}
o > CFAV{, T}
initialized fields = [])
~~~~~~~~~
<exit>
Expand Down
2 changes: 1 addition & 1 deletion docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ is an enhanced switch statement.

**Closed issues:**

eisop#609, eisop#610.
eisop#609, eisop#610, eisop#612.


Version 3.39.0-eisop1 (October 22, 2023)
Expand Down
20 changes: 20 additions & 0 deletions framework/tests/all-systems/EisopIssue612.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
interface Issue612A {}

@interface Issue612B {
Class<? extends Issue612A> value();
}

class Issue612C implements Issue612A {}

class Issue612E {}

@interface Issue612Z {
abstract class Issue612D implements Issue612A {

public Issue612D() {
g(new Issue612E());
}

static void g(Object... xs) {}
}
}
9 changes: 9 additions & 0 deletions framework/tests/all-systems/EisopIssue612Min.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@interface Issue612Min {
class D {
D() {
g(new Object());
}

static void g(Object... xs) {}
}
}