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

JSpecify: handle intersection type in one place #1015

Merged
merged 9 commits into from
Aug 19, 2024
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,6 +28,9 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
// TODO Handle wildcard types
return true;
}
if (lhsType.isIntersection()) {
return handleIntersectionType((Type.IntersectionClassType) lhsType, rhsType);
}
Types types = state.getTypes();
// The base type of rhsType may be a subtype of lhsType's base type. In such cases, we must
// compare lhsType against the supertype of rhsType with a matching base type.
Expand Down Expand Up @@ -64,6 +67,13 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
return lhsType.getEnclosingType().accept(this, rhsType.getEnclosingType());
}

/** Check identical nullability for every type in the intersection */
private Boolean handleIntersectionType(
Type.IntersectionClassType intersectionType, Type rhsType) {
return intersectionType.getBounds().stream()
.allMatch(type -> ((Type) type).accept(this, rhsType));
}

@Override
public Boolean visitArrayType(Type.ArrayType lhsType, Type rhsType) {
if (rhsType instanceof NullType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ public String visitWildcardType(Type.WildcardType t, Void unused) {

@Override
public String visitClassType(Type.ClassType t, Void s) {
if (t.isIntersection()) {
return prettyIntersectionType((Type.IntersectionClassType) t);
}
StringBuilder sb = new StringBuilder();
Type enclosingType = t.getEnclosingType();
if (!ASTHelpers.isSameType(enclosingType, Type.noType, state)) {
Expand All @@ -57,6 +60,12 @@ public String visitClassType(Type.ClassType t, Void s) {
return sb.toString();
}

private String prettyIntersectionType(Type.IntersectionClassType t) {
return t.getBounds().stream()
.map(type -> ((Type) type).accept(this, null))
.collect(joining(" & "));
}

@Override
public String visitCapturedType(Type.CapturedType t, Void s) {
return t.wildcard.accept(this, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1856,6 +1856,56 @@ public void issue1008() {
.doTest();
}

@Test
public void intersectionTypeFromConditionalExprInStringConcat() {
makeHelper()
.addSourceLines(
"ServiceExtraInfo.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"public class ServiceExtraInfo {",
" private java.util.@Nullable List<Object> relativeServices;",
" private String getStr() {",
" return (relativeServices == null ? \"relativeServices == null\" : relativeServices.size()) + \"\";",
" }",
" private String getStr2(boolean b, java.util.List<Object> l) {",
" return (b ? (b ? l.size() : \"hello\") : 3) + \"\";",
" }",
"}")
.doTest();
}

@Test
public void intersectionTypeInvalidAssign() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import java.io.Serializable;",
"public class Test {",
" interface A<T extends @Nullable Object> {}",
" static class B implements A<@Nullable String>, Serializable {}",
" static class C implements A<String>, Serializable {}",
" static void test1(Object o) {",
" var x = (A<String> & Serializable) o;",
" // BUG: Diagnostic contains: Cannot assign from type B to type A<String> & Serializable",
" x = new B();",
" // ok",
" x = new C();",
" }",
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, the case where we have:

            "    var x = (A<@Nullable String> & Serializable) o;",
            "    x = new B();",

Should have no error, right? Should we encode that test case just to make sure we never regress that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right, and i think we will get a false positive on that test for now (since for some reason var-declared variables don't get types with all the right annotations). i'll add tests and open an issue for outstanding tasks, but will fix in a subsequent PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 5a8bdd6 along with link to issue #1022

" static void test2(Object o) {",
" var x = (A<@Nullable String> & Serializable) o;",
// TODO: should _not_ be an error, see https://github.com/uber/NullAway/issues/1022
" // BUG: Diagnostic contains: Cannot assign from type B to type A<String> & Serializable",
" x = new B();",
// TODO: _should_ be an error, see https://github.com/uber/NullAway/issues/1022
" x = new C();",
" }",
"}")
.doTest();
}

@Test
public void issue1014() {
makeHelper()
Expand Down