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.

Sign up for GitHub

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

8310905: [lw5] addressing review comments on null restricted types #880

Closed
wants to merge 8 commits into from
+493 −103
Closed

8310905: [lw5] addressing review comments on null restricted types #880

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f28c4d0
8310905: [lw5] addressing review comments on null restricted types
vicente-romero-oracle Jun 26, 2023
dd3fccc
addressing more review comments
vicente-romero-oracle Jun 28, 2023
b605e9c
adding more methods to NullabilityComparator visitor, adding more tes…
vicente-romero-oracle Jun 30, 2023
c6f88ac
adding more regression tests
vicente-romero-oracle Jul 5, 2023
054b548
addressing another round of review comments
vicente-romero-oracle Jul 12, 2023
32be32c
dont generate warnings for literals or constructor invocations
vicente-romero-oracle Jul 13, 2023
0c168ed
addressing additional review comments
vicente-romero-oracle Jul 13, 2023
da16b3b
addressing another round of review comments
vicente-romero-oracle Jul 15, 2023
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
94 changes: 47 additions & 47 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
View file
Original file line number Diff line number Diff line change
@@ -1037,33 +1037,38 @@ public boolean isSubtypeUnchecked(Type t, Type s, Warner warn) {
}
//where
private boolean isSubtypeUncheckedInternal(Type t, Type s, boolean capture, Warner warn) {
if (t.hasTag(ARRAY) && s.hasTag(ARRAY)) {
if (((ArrayType)t).elemtype.isPrimitive()) {
return isSameType(elemtype(t), elemtype(s));
} else {
// if T.ref <: S, then T[] <: S[]
Type es = elemtype(s);
Type et = elemtype(t);
if (!isSubtypeUncheckedInternal(et, es, false, warn))
return false;
return true;
}
} else if (isSubtype(t, s, capture, warn)) {
return true;
} else if (t.hasTag(TYPEVAR)) {
return isSubtypeUncheckedInternal(t.getUpperBound(), s, false, warn);
} else if (!s.isRaw()) {
Type t2 = asSuper(t, s.tsym);
if (t2 != null && t2.isRaw()) {
if (isReifiable(s)) {
warn.silentWarn(LintCategory.UNCHECKED);
try {
nullabilityComparator.setWarner(warn);
if (t.hasTag(ARRAY) && s.hasTag(ARRAY)) {
if (((ArrayType)t).elemtype.isPrimitive()) {
return isSameType(elemtype(t), elemtype(s));
} else {
warn.warn(LintCategory.UNCHECKED);
// if T.ref <: S, then T[] <: S[]
Type es = elemtype(s);
Type et = elemtype(t);
if (!isSubtypeUncheckedInternal(et, es, false, warn))
return false;
return true;
}
} else if (isSubtype(t, s, capture)) {
return true;
} else if (t.hasTag(TYPEVAR)) {
return isSubtypeUncheckedInternal(t.getUpperBound(), s, false, warn);
} else if (!s.isRaw()) {
Type t2 = asSuper(t, s.tsym);
if (t2 != null && t2.isRaw()) {
if (isReifiable(s)) {
warn.silentWarn(LintCategory.UNCHECKED);
} else {
warn.warn(LintCategory.UNCHECKED);
}
return true;
}
}
return false;
} finally {
nullabilityComparator.clearWarner();
}
return false;
}

private void checkUnsafeVarargsConversion(Type t, Type s, Warner warn) {
@@ -1099,12 +1104,8 @@ public final boolean isSubtypeNoCapture(Type t, Type s) {
return isSubtype(t, s, false);
}
public boolean isSubtype(Type t, Type s, boolean capture) {
return isSubtype(t, s, capture, noWarnings);
}
public boolean isSubtype(Type t, Type s, boolean capture, Warner warn) {
if (t.equalsIgnoreMetadata(s)) {
nullabilityComparator.reset((t1, t2) -> t1.hasNarrowerNullabilityThan(t2),
!warnStack.isEmpty() ? warnStack.head : warn).visit(s, t);
nullabilityComparator.reset((t1, t2) -> t1.hasNarrowerNullabilityThan(t2)).visit(s, t);
return true;
}
if (s.isPartial())
@@ -1127,20 +1128,7 @@ public boolean isSubtype(Type t, Type s, boolean capture, Warner warn) {
if (s != lower && !lower.hasTag(BOT))
return isSubtype(capture ? capture(t) : t, lower, false);
}

if (warn == warnStack.head ||
// if warn is noWarnings, then we should be reentering this method while computing the subtype of a,
// possibly, compound type, so keep the current top of the warnStack
(!warnStack.isEmpty() && warn == noWarnings)) {
return isSubtype.visit(capture ? capture(t) : t, s);
} else {
try {
warnStack = warnStack.prepend(warn);
return isSubtype.visit(capture ? capture(t) : t, s);
} finally {
warnStack = warnStack.tail;
}
}
return isSubtype.visit(capture ? capture(t) : t, s);
}
// where
private IsSubtype isSubtype = new IsSubtype();
@@ -1237,7 +1225,7 @@ public Boolean visitClassType(ClassType t, Type s) {
&& isSubtypeNoCapture(sup.getEnclosingType(),
s.getEnclosingType());
if (result) {
nullabilityComparator.reset((t1, t2) -> t1.hasNarrowerNullabilityThan(t2), warnStack.head).visit(s, t);
nullabilityComparator.reset((t1, t2) -> t1.hasNarrowerNullabilityThan(t2)).visit(s, t);
}
return result;
}
@@ -1286,17 +1274,29 @@ public Boolean visitErrorType(ErrorType t, Type s) {
}
}

private NullabilityComparator nullabilityComparator = new NullabilityComparator();
class NullabilityComparator extends TypeRelation {
public NullabilityComparator nullabilityComparator = new NullabilityComparator();
public class NullabilityComparator extends TypeRelation {
BiFunction<Type, Type, Boolean> differentNullability;
Warner warner;

NullabilityComparator reset(BiFunction<Type, Type, Boolean> differentNullability, Warner warner) {
NullabilityComparator reset(BiFunction<Type, Type, Boolean> differentNullability) {
this.differentNullability = differentNullability;
if (this.warner == null || this.warner == noWarnings) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow what's happening here. We have a warner stack in Types already. But this class seems to somehow override whatever warner is in the stack (if a client called setWarner on the nullability comparator). Shouldn't we try to reuse the warnStack as a way to set warners, and then tweak nullability comparator to use whatever warner is on the stack? E.g. I'm suspicious about the set/clearWarner methods, as they seem to duplicate functionality we already have in other forms.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did this because Check::checkExtends invokes Types::isSubtype, in a previous iteration I think you suggested not to add a Warner argument to Types::isSubtype, so this is an effort to circumvent that. I will give it another try

this.warner = !warnStack.isEmpty() ? warnStack.head : noWarnings;
}
return this;
}

public NullabilityComparator setWarner(Warner warner) {
this.warner = warner;
return this;
}

public NullabilityComparator clearWarner() {
this.warner = null;
return this;
}

@Override
public Boolean visitType(Type t, Type s) {
if (differentNullability.apply(t, s)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this truly a recursive visitor? I'm wondering, because if you don't need the visitor capabilities, then you could just drop the use of lambdas which will make the code a bit cleaner. E.g. instead of:

nullabilityComparator.reset((t1, t2) -> !t1.sameNullabilityAs(t2))
                            .visit(s, t);

do:

if (!s.sameNullabilityAs(t)) {
     nullabilityComparator.warn(...);
}

In which case, maybe NullabilityComparator is just NullabilityWarner?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Jun 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes the visitor is needed, I found other test cases for which this approach is not enough, I will be adding another iteration with more test cases

@@ -1497,7 +1497,7 @@ public Boolean visitClassType(ClassType t, Type s) {
&& visit(t.getEnclosingType(), s.getEnclosingType())
&& containsTypeEquivalent(t.getTypeArguments(), s.getTypeArguments());
if (equal) {
nullabilityComparator.reset((t1, t2) -> !t1.sameNullabilityAs(t2), !warnStack.isEmpty() ? warnStack.head : noWarnings)
nullabilityComparator.reset((t1, t2) -> !t1.sameNullabilityAs(t2))
.visit(s, t);
}
return equal;
@@ -4380,7 +4380,7 @@ public boolean returnTypeSubstitutable(Type r1,
return covariantReturnType(r1.getReturnType(), r2res, warner);
if (isSubtypeUnchecked(r1.getReturnType(), r2res, warner))
return true;
if (!isSubtype(r1.getReturnType(), erasure(r2res), false, warner))
if (!isSubtype(r1.getReturnType(), erasure(r2res), false))
return false;
warner.warn(LintCategory.UNCHECKED);
return true;
14 changes: 11 additions & 3 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java
View file
Original file line number Diff line number Diff line change
@@ -725,9 +725,17 @@ private boolean checkExtends(JCTree pos, Type a, Type bound) {
return true;
} else if (!a.hasTag(WILDCARD)) {
a = types.cvarUpperBound(a);
return pos != null ?
types.isSubtype(a, bound, true, new NullnessWarner(pos)) :
types.isSubtype(a, bound, true);
try {
if (pos != null) {
types.nullabilityComparator.setWarner(new NullnessWarner(pos));
}
return types.isSubtype(a, bound, true);
} finally {
if (pos != null) {
types.nullabilityComparator.clearWarner();
}
}

} else if (a.isExtendsBound()) {
return types.isCastable(bound, types.wildUpperBound(a), types.noWarnings);
} else if (a.isSuperBound()) {