Skip to content

Java: Improve RefType.inherits(Member) #8660

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

Open
Marcono1234 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Java: Improve RefType.inherits(Member) #8660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c82f1fe
Java: Add inheritance test
Marcono1234 Apr 4, 2022
9099b7f
Java: Consider constructors and member types as inherited
Marcono1234 Apr 4, 2022
719d87e
Java: Improve cross-package inheritance handling
Marcono1234 Apr 4, 2022
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
20 changes: 19 additions & 1 deletion java/ql/lib/semmle/code/java/Type.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,12 +568,30 @@ class RefType extends Type, Annotatable, Modifiable, @reftype {
exists(Field f | f = m |
f = this.getAField()
or
not f.isPrivate() and not this.declaresField(f.getName()) and this.getASupertype().inherits(f)
not f.isPrivate() and
(f.isPackageProtected() implies f.getDeclaringType().getPackage() = this.getPackage()) and
not this.declaresField(f.getName()) and
this.getASupertype().inherits(f)
or
this.getSourceDeclaration().inherits(f)
)
or
this.hasMethod(m, _)
or
// Constructors are not inherited, but this predicate also considers declared members
m.(Constructor).getDeclaringType() = this.getSourceDeclaration()
or
exists(MemberType memberType | memberType = m |
m.(MemberType).getDeclaringType() = this
or
not memberType.isPrivate() and
(memberType.isPackageProtected() implies memberType.getPackage() = this.getPackage()) and
// And member type is not hidden by another member type with the same name (regardless of visibility)
Copy link
Contributor

@smowton smowton Apr 7, 2022

Choose a reason for hiding this comment

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

There's also https://docs.oracle.com/javase/tutorial/java/IandI/hidevariables.html which would enable you to treat types and fields the same

Copy link
Contributor Author

@Marcono1234 Marcono1234 Apr 10, 2022

Choose a reason for hiding this comment

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

That seems to only be about fields though, isn't it? Do you mean fields could hide member types and member types could hide fields? I am not sure if that is possible, at least the JLS does not seem to mention it.

Or do you mean I should try to combine the common checks here for Field and MemberType?

Copy link
Contributor

@smowton smowton Apr 11, 2022

Choose a reason for hiding this comment

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

I meant fields hiding fields and types hiding types and forgot about the possibility of one hiding the other.

not this.getAMember().(MemberType).getName() = memberType.getName() and
this.getASupertype().inherits(memberType)
or
this.getSourceDeclaration().inherits(memberType)
)
}

/** Holds if this is a top-level type, which is not nested inside any other types. */
Expand Down
82 changes: 82 additions & 0 deletions java/ql/test/library-tests/inheritance/inhertiance.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
onlyInherited
| package_a/FieldInheritanceA.java:36:19:36:36 | InheritingClashing | package_a/FieldInheritanceA.java:25:13:25:19 | staticI |
| package_a/FieldInheritanceA.java:36:19:36:36 | InheritingClashing | package_a/FieldInheritanceA.java:28:13:28:19 | staticI |
| package_a/FieldInheritanceA.java:36:19:36:36 | InheritingClashing | package_a/FieldInheritanceA.java:31:20:31:20 | i |
| package_a/FieldInheritanceA.java:36:19:36:36 | InheritingClashing | package_a/FieldInheritanceA.java:32:27:32:33 | staticI |
| package_a/FieldInheritanceA.java:43:26:43:45 | ExtendingHidingField | package_a/FieldInheritanceA.java:40:27:40:27 | i |
| package_a/FieldInheritanceA.java:43:26:43:45 | ExtendingHidingField | package_a/FieldInheritanceA.java:41:20:41:26 | staticI |
| package_a/FieldInheritanceAB.java:8:14:8:31 | FieldInheritanceAB | package_a/FieldInheritanceA.java:12:19:12:32 | protectedField |
| package_a/FieldInheritanceAB.java:8:14:8:31 | FieldInheritanceAB | package_a/FieldInheritanceA.java:13:26:13:45 | protectedStaticField |
| package_a/FieldInheritanceAB.java:8:14:8:31 | FieldInheritanceAB | package_a/FieldInheritanceA.java:15:16:15:26 | publicField |
| package_a/FieldInheritanceAB.java:8:14:8:31 | FieldInheritanceAB | package_a/FieldInheritanceA.java:16:23:16:39 | publicStaticField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:9:9:9:27 | packagePrivateField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:10:16:10:40 | packagePrivateStaticField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:12:19:12:32 | protectedField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:13:26:13:45 | protectedStaticField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:15:16:15:26 | publicField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:16:23:16:39 | publicStaticField |
| package_a/FieldInheritanceOtherA.java:3:14:3:35 | FieldInheritanceOtherA | package_a/FieldInheritanceA.java:55:18:55:35 | PrivateHidingField |
| package_a/MemberTypeInheritanceA.java:25:23:25:46 | ExtendingPublicInterface | package_a/MemberTypeInheritanceA.java:22:29:22:34 | Nested |
| package_a/MemberTypeInheritanceA.java:26:23:26:55 | ExtendingExtendingPublicInterface | package_a/MemberTypeInheritanceA.java:22:29:22:34 | Nested |
| package_a/MemberTypeInheritanceA.java:27:19:27:45 | ImplementingPublicInterface | package_a/MemberTypeInheritanceA.java:22:29:22:34 | Nested |
| package_a/MemberTypeInheritanceA.java:36:26:36:43 | InheritingClashing | package_a/MemberTypeInheritanceA.java:22:29:22:34 | Nested |
| package_a/MemberTypeInheritanceA.java:36:26:36:43 | InheritingClashing | package_a/MemberTypeInheritanceA.java:30:29:30:34 | Nested |
| package_a/MemberTypeInheritanceA.java:36:26:36:43 | InheritingClashing | package_a/MemberTypeInheritanceA.java:33:29:33:34 | Nested |
| package_a/MemberTypeInheritanceA.java:41:23:41:48 | ExtendingHidingNestedClass | package_a/MemberTypeInheritanceA.java:39:26:39:31 | Nested |
| package_a/MemberTypeInheritanceA.java:66:19:66:41 | ExtendingWithLocalTypes | package_a/MemberTypeInheritanceA.java:58:36:58:36 | o |
| package_a/MemberTypeInheritanceA.java:66:19:66:41 | ExtendingWithLocalTypes | package_a/MemberTypeInheritanceA.java:60:21:60:21 | m |
| package_a/MemberTypeInheritanceAB.java:8:14:8:36 | MemberTypeInheritanceAB | package_a/MemberTypeInheritanceA.java:16:21:16:34 | ProtectedClass |
| package_a/MemberTypeInheritanceAB.java:8:14:8:36 | MemberTypeInheritanceAB | package_a/MemberTypeInheritanceA.java:17:25:17:42 | ProtectedInterface |
| package_a/MemberTypeInheritanceAB.java:8:14:8:36 | MemberTypeInheritanceAB | package_a/MemberTypeInheritanceA.java:19:18:19:28 | PublicClass |
| package_a/MemberTypeInheritanceAB.java:8:14:8:36 | MemberTypeInheritanceAB | package_a/MemberTypeInheritanceA.java:20:25:20:41 | PublicStaticClass |
| package_a/MemberTypeInheritanceAB.java:8:14:8:36 | MemberTypeInheritanceAB | package_a/MemberTypeInheritanceA.java:21:22:21:36 | PublicInterface |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:13:11:13:29 | PackagePrivateClass |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:14:15:14:37 | PackagePrivateInterface |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:16:21:16:34 | ProtectedClass |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:17:25:17:42 | ProtectedInterface |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:19:18:19:28 | PublicClass |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:20:25:20:41 | PublicStaticClass |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:21:22:21:36 | PublicInterface |
| package_a/MemberTypeInheritanceOtherA.java:3:14:3:40 | MemberTypeInheritanceOtherA | package_a/MemberTypeInheritanceA.java:47:18:47:41 | HidingNestedClassPrivate |
| package_a/MethodInheritanceA.java:49:23:49:40 | ExtendingInterface | package_a/MethodInheritanceA.java:46:22:46:22 | d |
| package_a/MethodInheritanceA.java:49:23:49:40 | ExtendingInterface | package_a/MethodInheritanceA.java:47:14:47:14 | a |
| package_a/MethodInheritanceA.java:66:26:66:49 | ExtendingClassWithPublic | package_a/MethodInheritanceA.java:64:21:64:21 | m |
| package_a/MethodInheritanceA.java:69:28:69:53 | AbstractInheritingClashing | package_a/MethodInheritanceA.java:61:30:61:30 | m |
| package_a/MethodInheritanceA.java:72:19:72:36 | InheritingClashing | package_a/MethodInheritanceA.java:64:21:64:21 | m |
| package_a/MethodInheritanceA.java:76:19:76:46 | InheritingClashingTransitive | package_a/MethodInheritanceA.java:64:21:64:21 | m |
| package_a/MethodInheritanceA.java:82:23:82:46 | ExtendingMultipleMethods | package_a/MethodInheritanceA.java:80:14:80:14 | m |
| package_a/MethodInheritanceA.java:102:26:102:40 | BaseAndOverride | package_a/MethodInheritanceA.java:96:22:96:22 | m |
| package_a/MethodInheritanceA.java:104:23:104:45 | ExtendingOverridingBase | package_a/MethodInheritanceA.java:96:22:96:22 | m |
| package_a/MethodInheritanceA.java:106:26:106:49 | BaseAndExtendingOverride | package_a/MethodInheritanceA.java:96:22:96:22 | m |
| package_a/MethodInheritanceA.java:117:19:117:40 | InheritingSubsignature | package_a/MethodInheritanceA.java:112:21:112:21 | m |
| package_a/MethodInheritanceAB.java:7:14:7:32 | MethodInheritanceAB | package_a/MethodInheritanceA.java:17:20:17:34 | protectedMethod |
| package_a/MethodInheritanceAB.java:7:14:7:32 | MethodInheritanceAB | package_a/MethodInheritanceA.java:18:27:18:47 | protectedStaticMethod |
| package_a/MethodInheritanceAB.java:7:14:7:32 | MethodInheritanceAB | package_a/MethodInheritanceA.java:20:17:20:28 | publicMethod |
| package_a/MethodInheritanceAB.java:7:14:7:32 | MethodInheritanceAB | package_a/MethodInheritanceA.java:21:24:21:41 | publicStaticMethod |
| package_b/FieldInheritanceB.java:5:14:5:30 | FieldInheritanceB | package_a/FieldInheritanceA.java:12:19:12:32 | protectedField |
| package_b/FieldInheritanceB.java:5:14:5:30 | FieldInheritanceB | package_a/FieldInheritanceA.java:13:26:13:45 | protectedStaticField |
| package_b/FieldInheritanceB.java:5:14:5:30 | FieldInheritanceB | package_a/FieldInheritanceA.java:15:16:15:26 | publicField |
| package_b/FieldInheritanceB.java:5:14:5:30 | FieldInheritanceB | package_a/FieldInheritanceA.java:16:23:16:39 | publicStaticField |
| package_b/FieldInheritanceBTransitive.java:3:14:3:40 | FieldInheritanceBTransitive | package_a/FieldInheritanceA.java:12:19:12:32 | protectedField |
| package_b/FieldInheritanceBTransitive.java:3:14:3:40 | FieldInheritanceBTransitive | package_a/FieldInheritanceA.java:13:26:13:45 | protectedStaticField |
| package_b/FieldInheritanceBTransitive.java:3:14:3:40 | FieldInheritanceBTransitive | package_a/FieldInheritanceA.java:15:16:15:26 | publicField |
| package_b/FieldInheritanceBTransitive.java:3:14:3:40 | FieldInheritanceBTransitive | package_a/FieldInheritanceA.java:16:23:16:39 | publicStaticField |
| package_b/MemberTypeInheritanceB.java:5:14:5:35 | MemberTypeInheritanceB | package_a/MemberTypeInheritanceA.java:16:21:16:34 | ProtectedClass |
| package_b/MemberTypeInheritanceB.java:5:14:5:35 | MemberTypeInheritanceB | package_a/MemberTypeInheritanceA.java:17:25:17:42 | ProtectedInterface |
| package_b/MemberTypeInheritanceB.java:5:14:5:35 | MemberTypeInheritanceB | package_a/MemberTypeInheritanceA.java:19:18:19:28 | PublicClass |
| package_b/MemberTypeInheritanceB.java:5:14:5:35 | MemberTypeInheritanceB | package_a/MemberTypeInheritanceA.java:20:25:20:41 | PublicStaticClass |
| package_b/MemberTypeInheritanceB.java:5:14:5:35 | MemberTypeInheritanceB | package_a/MemberTypeInheritanceA.java:21:22:21:36 | PublicInterface |
| package_b/MemberTypeInheritanceBTransitive.java:3:14:3:45 | MemberTypeInheritanceBTransitive | package_a/MemberTypeInheritanceA.java:16:21:16:34 | ProtectedClass |
| package_b/MemberTypeInheritanceBTransitive.java:3:14:3:45 | MemberTypeInheritanceBTransitive | package_a/MemberTypeInheritanceA.java:17:25:17:42 | ProtectedInterface |
| package_b/MemberTypeInheritanceBTransitive.java:3:14:3:45 | MemberTypeInheritanceBTransitive | package_a/MemberTypeInheritanceA.java:19:18:19:28 | PublicClass |
| package_b/MemberTypeInheritanceBTransitive.java:3:14:3:45 | MemberTypeInheritanceBTransitive | package_a/MemberTypeInheritanceA.java:20:25:20:41 | PublicStaticClass |
| package_b/MemberTypeInheritanceBTransitive.java:3:14:3:45 | MemberTypeInheritanceBTransitive | package_a/MemberTypeInheritanceA.java:21:22:21:36 | PublicInterface |
| package_b/MethodInheritanceB.java:5:14:5:31 | MethodInheritanceB | package_a/MethodInheritanceA.java:17:20:17:34 | protectedMethod |
| package_b/MethodInheritanceB.java:5:14:5:31 | MethodInheritanceB | package_a/MethodInheritanceA.java:18:27:18:47 | protectedStaticMethod |
| package_b/MethodInheritanceB.java:5:14:5:31 | MethodInheritanceB | package_a/MethodInheritanceA.java:20:17:20:28 | publicMethod |
| package_b/MethodInheritanceB.java:5:14:5:31 | MethodInheritanceB | package_a/MethodInheritanceA.java:21:24:21:41 | publicStaticMethod |
| package_b/MethodInheritanceBTransitive.java:3:14:3:41 | MethodInheritanceBTransitive | package_a/MethodInheritanceA.java:17:20:17:34 | protectedMethod |
| package_b/MethodInheritanceBTransitive.java:3:14:3:41 | MethodInheritanceBTransitive | package_a/MethodInheritanceA.java:18:27:18:47 | protectedStaticMethod |
| package_b/MethodInheritanceBTransitive.java:3:14:3:41 | MethodInheritanceBTransitive | package_a/MethodInheritanceA.java:20:17:20:28 | publicMethod |
| package_b/MethodInheritanceBTransitive.java:3:14:3:41 | MethodInheritanceBTransitive | package_a/MethodInheritanceA.java:21:24:21:41 | publicStaticMethod |
bugDeclaredButNotInherited
17 changes: 17 additions & 0 deletions java/ql/test/library-tests/inheritance/inhertiance.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import java

/** Matches members which are only inherited, but not declared by the type */
query Member onlyInherited(RefType t) {
t.fromSource() and
t.inherits(result) and
not result.getDeclaringType() instanceof TypeObject and
// Ignore declared members; query predicate below verifies that they are a subset
not result.getDeclaringType() = t
}

/** Verifies that declared members are a subset of inherited members */
query Member bugDeclaredButNotInherited(RefType t) {
t.fromSource() and
result.getDeclaringType() = t and
not t.inherits(result)
}
1 change: 1 addition & 0 deletions java/ql/test/library-tests/inheritance/options
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
//semmle-extractor-options: --javac-args -source 16 -target 16
59 changes: 59 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/FieldInheritanceA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// See https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.3

package package_a;

public class FieldInheritanceA {
private int privateField;
private static int privateStaticField;

int packagePrivateField;
static int packagePrivateStaticField;

protected int protectedField;
protected static int protectedStaticField;

public int publicField;
public static int publicStaticField;

private static class ClassWithPrivate {
private int i;
private static int staticI;
}
private class ExtendingClassWithPrivate extends ClassWithPrivate {}

private interface InterfaceWithPublic {
int staticI = 1;
}
private interface OtherInterfaceWithPublic {
int staticI = 1;
}
private static class ClassWithPublic {
public int i;
public static int staticI;
}

// Should inherit all fields called staticI
private class InheritingClashing extends ClassWithPublic implements InterfaceWithPublic, OtherInterfaceWithPublic {}

private static class HidingField extends ClassWithPublic {
// Whether fields are static or not does not make a difference
public static int i;
public int staticI;
}
private static class ExtendingHidingField extends HidingField {}
private static class HidingTransitiveField extends ExtendingHidingField {
public int i;
public static int staticI;
}

private static class HidingWithDifferentType extends ClassWithPublic {
private String i;
private static String staticI;
}

// Has subclass in FieldInheritanceOtherA
static class PrivateHidingField extends ClassWithPublic {
private String i;
private static String staticI;
}
}
9 changes: 9 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/FieldInheritanceAB.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package package_a;

import package_b.FieldInheritanceB;

// Tries to transitively inherit package-private fields which were not inherited by FieldInheritanceB
// These fields should not be considered inherited per JLS, even though the javac error message when
// trying to access them is misleading: "cannot be accessed from outside package"
public class FieldInheritanceAB extends FieldInheritanceB {
}
7 changes: 7 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/FieldInheritanceOtherA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package package_a;

public class FieldInheritanceOtherA extends FieldInheritanceA {
// Cannot inheriting original fields, due to fields in PrivateHidingField
// even though they are not accessible here
class ExtendingPrivateHidingField extends PrivateHidingField {}
}
67 changes: 67 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/MemberTypeInheritanceA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// See https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.5

package package_a;

public class MemberTypeInheritanceA {
private class PrivateClass {
private class Nested {}
}
private class ExtendingPrivateClass extends PrivateClass {}

private interface PrivateInterface {}

class PackagePrivateClass {}
interface PackagePrivateInterface {}

protected class ProtectedClass {}
protected interface ProtectedInterface {}

public class PublicClass {}
public static class PublicStaticClass {}
public interface PublicInterface {
public static class Nested {}
}

private interface ExtendingPublicInterface extends PublicInterface {}
private interface ExtendingExtendingPublicInterface extends ExtendingPublicInterface {}
private class ImplementingPublicInterface implements PublicInterface {}

private interface OtherInterface {
public static class Nested {}
}
private static class OtherClass {
public static class Nested {}
}
// Should inherit all classes called Nested
private static class InheritingClashing extends OtherClass implements PublicInterface, OtherInterface {}

private interface HidingNestedClass extends OtherInterface {
public interface Nested {}
}
private interface ExtendingHidingNestedClass extends HidingNestedClass {}
private interface HidingTransitiveNestedClass extends ExtendingPublicInterface {
public interface Nested {}
}

// Has subclass in MemberTypeInheritanceOtherA
static class HidingNestedClassPrivate implements OtherInterface {
private interface Nested {}
}

private class WithLocalTypes {
// Local and anonymous types are not inherited

{
class LocalClass {}
}

public static final Object o = new Object() {};

public void m() {
class LocalClass {}
interface LocalInterface {}
record LocalRecord() {}
}
}
private class ExtendingWithLocalTypes extends WithLocalTypes {}
}
9 changes: 9 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/MemberTypeInheritanceAB.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package package_a;

import package_b.MemberTypeInheritanceB;

// Tries to transitively inherit package-private member types which were not inherited by MemberTypeInheritanceB
// These member types should not be considered inherited per JLS, even though the javac error message when
// trying to access them is misleading: "cannot be accessed from outside package"
public class MemberTypeInheritanceAB extends MemberTypeInheritanceB {
}
7 changes: 7 additions & 0 deletions java/ql/test/library-tests/inheritance/package_a/MemberTypeInheritanceOtherA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package package_a;

public class MemberTypeInheritanceOtherA extends MemberTypeInheritanceA {
// Cannot inheriting original class, due to class in HidingNestedClassPrivate
// even though it is not accessible here
private static class ExtendendingHidingNestedClassPrivate extends HidingNestedClassPrivate {}
}
Loading