Merge pull request #10191 from smowton/smowton/admin/java-implicit-this-type-tests

Java: Add test regarding the type of an implicit `this` expression

Author: aschackmull

java/ql/src/Advisory/Deprecated Code/AvoidDeprecatedCallableAccess.ql

@@ -20,7 +20,7 @@ private predicate isDeprecatedCallable(Callable c) {
 
 from Call ca, Callable c
 where
-  ca.getCallee() = c and
+  ca.getCallee().getSourceDeclaration() = c and
   isDeprecatedCallable(c) and
   // Exclude deprecated calls from within deprecated code.
   not isDeprecatedCallable(ca.getCaller())

java/ql/src/Likely Bugs/Collections/IteratorRemoveMayFail.ql

@@ -36,11 +36,11 @@ predicate containsSpecialCollection(Expr e, SpecialCollectionCreation origin) {
   or
   exists(Call c, int i |
     containsSpecialCollection(c.getArgument(i), origin) and
-    e = c.getCallee().getParameter(i).getAnAccess()
+    e = c.getCallee().getSourceDeclaration().getParameter(i).getAnAccess()
   )
   or
   exists(Call c, ReturnStmt r | e = c |
-    r.getEnclosingCallable() = c.getCallee() and
+    r.getEnclosingCallable() = c.getCallee().getSourceDeclaration() and
     containsSpecialCollection(r.getResult(), origin)
   )
 }
@@ -58,11 +58,11 @@ predicate iterOfSpecialCollection(Expr e, SpecialCollectionCreation origin) {
   or
   exists(Call c, int i |
     iterOfSpecialCollection(c.getArgument(i), origin) and
-    e = c.getCallee().getParameter(i).getAnAccess()
+    e = c.getCallee().getSourceDeclaration().getParameter(i).getAnAccess()
   )
   or
   exists(Call c, ReturnStmt r | e = c |
-    r.getEnclosingCallable() = c.getCallee() and
+    r.getEnclosingCallable() = c.getCallee().getSourceDeclaration() and
     iterOfSpecialCollection(r.getResult(), origin)
   )
 }

java/ql/src/Performance/InnerClassCouldBeStatic.ql

@@ -78,7 +78,7 @@ RefType enclosingInstanceAccess(Expr expr) {
       result = ma.getMethod().getDeclaringType() and
       not exists(ma.getQualifier()) and
       not ma.getMethod().isStatic() and
-      not exists(Method m | m.getSourceDeclaration() = ma.getMethod() | enclosing.inherits(m))
+      not enclosing.inherits(ma.getMethod())
     )
   )
 }

java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql

@@ -72,7 +72,7 @@ predicate mayWriteToArray(Expr modified) {
   // return __array__;    ...  method()[1] = 0
   exists(ReturnStmt rs | modified = rs.getResult() and relevantType(modified.getType()) |
     exists(Callable enclosing, MethodAccess ma |
-      enclosing = rs.getEnclosingCallable() and ma.getMethod() = enclosing
+      enclosing = rs.getEnclosingCallable() and ma.getMethod().getSourceDeclaration() = enclosing
     |
       mayWriteToArray(ma)
     )
@@ -100,7 +100,7 @@ VarAccess varPassedInto(Callable c, int i) {
 predicate exposesByReturn(Callable c, Field f, Expr why, string whyText) {
   returnsArray(c, f) and
   exists(MethodAccess ma |
-    ma.getMethod() = c and ma.getCompilationUnit() != c.getCompilationUnit()
+    ma.getMethod().getSourceDeclaration() = c and ma.getCompilationUnit() != c.getCompilationUnit()
   |
     mayWriteToArray(ma) and
     why = ma and

java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql

@@ -27,7 +27,7 @@ predicate callToInheritedMethod(RefType lexicalScope, MethodAccess ma, string si
   not ma.getMethod().isStatic() and
   not ma.hasQualifier() and
   ma.getEnclosingCallable().getDeclaringType() = lexicalScope and
-  nestedSupertypePlus(lexicalScope).getAMethod() = ma.getMethod() and
+  nestedSupertypePlus(lexicalScope).getAMethod() = ma.getMethod().getSourceDeclaration() and
   signature = ma.getMethod().getSignature()
 }
 

java/ql/src/change-notes/2022-09-15-implicit-this-generic-method.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The Java extractor now populates the `Method` relating to a `MethodAccess` consistently for calls using an explicit and implicit `this` qualifier. Previously if the method `foo` was inherited from a specialised generic type `ParentType<String>`, then an explicit call `this.foo()` would yield a `MethodAccess` whose `getMethod()` accessor returned the bound method `ParentType<String>.foo`, whereas an implicitly-qualified `foo()` `MethodAccess`'s `getMethod()` would return the unbound method `ParentType.foo`. Now both scenarios produce a bound method. This means that all data-flow queries may return more results where a relevant path transits a call to such an implicitly-qualified call to a member method with a bound generic type, while queries that inspect the result of `MethodAccess.getMethod()` may need to tolerate bound generic methods in more circumstances. The queries `java/iterator-remove-failure`, `java/non-static-nested-class`, `java/internal-representation-exposure`, `java/subtle-inherited-call` and `java/deprecated-call` have been amended to properly handle calls to bound generic methods, and in some instances may now produce more results in the explicit-`this` case as well.

java/ql/test/library-tests/implicit-this-type/Test.java

@@ -0,0 +1,83 @@
+class Gen<T> {
+  void m(T t) { }
+}
+
+class SubSpec extends Gen<String> {
+  void foo() {
+    m("direct implicit this");
+    this.m("direct explicit this");
+  }
+
+  class Inner {
+    void bar() {
+      m("direct implicit this from inner");
+      SubSpec.this.m("direct explicit this from inner");
+    }
+  }
+
+  void hasLocal() {
+    class Local {
+      void baz() {
+        m("direct implicit this from local");
+        SubSpec.this.m("direct explicit this from local");
+      }
+    }
+  }
+}
+
+class SubGen<S> extends Gen<S> {
+  void foo() {
+    m((S)"direct implicit this (generic sub)");
+    this.m((S)"direct explicit this (generic sub)");
+  }
+
+  class Inner {
+    void bar() {
+      m((S)"direct implicit this from inner (generic sub)");
+      SubGen.this.m((S)"direct explicit this from inner (generic sub)");
+    }
+  }
+}
+
+class Intermediate<S> extends Gen<S> { }
+
+class GrandchildSpec extends Intermediate<String> {
+  void foo() {
+    m("indirect implicit this");
+    this.m("indirect explicit this");
+  }
+
+  class Inner {
+    void bar() {
+      m("indirect implicit this from inner");
+      GrandchildSpec.this.m("indirect explicit this from inner");
+    }
+  }
+}
+
+class GrandchildGen<R> extends Intermediate<R> {
+  void foo() {
+    m((R)"indirect implicit this (generic sub)");
+    this.m((R)"indirect explicit this (generic sub)");
+  }
+
+  class Inner {
+    void bar() {
+      m((R)"indirect implicit this from inner (generic sub)");
+      GrandchildGen.this.m((R)"indirect explicit this from inner (generic sub)");
+    }
+  }
+}
+
+class UninvolvedOuter {
+  class InnerGen<T> {
+    void m(T t) { }
+  }
+  
+  class InnerSpec extends InnerGen<String> {
+    void foo() {
+      m("direct implicit this, from inner to inner");
+      this.m("direct explicit this, from inner to inner");
+    }
+  }
+}

java/ql/test/library-tests/implicit-this-type/test.expected

@@ -0,0 +1,20 @@
+| Test.java:7:5:7:29 | m(...) | Test.java:7:7:7:28 | "direct implicit this" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:8:5:8:34 | m(...) | Test.java:8:12:8:33 | "direct explicit this" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:13:7:13:42 | m(...) | Test.java:13:9:13:41 | "direct implicit this from inner" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:14:7:14:55 | m(...) | Test.java:14:22:14:54 | "direct explicit this from inner" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:21:9:21:44 | m(...) | Test.java:21:11:21:43 | "direct implicit this from local" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:22:9:22:57 | m(...) | Test.java:22:24:22:56 | "direct explicit this from local" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:30:5:30:46 | m(...) | Test.java:30:10:30:45 | "direct implicit this (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<S> |
+| Test.java:31:5:31:51 | m(...) | Test.java:31:15:31:50 | "direct explicit this (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<S> |
+| Test.java:36:7:36:59 | m(...) | Test.java:36:12:36:58 | "direct implicit this from inner (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<S> |
+| Test.java:37:7:37:71 | m(...) | Test.java:37:24:37:70 | "direct explicit this from inner (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<S> |
+| Test.java:46:5:46:31 | m(...) | Test.java:46:7:46:30 | "indirect implicit this" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:47:5:47:36 | m(...) | Test.java:47:12:47:35 | "indirect explicit this" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:52:7:52:44 | m(...) | Test.java:52:9:52:43 | "indirect implicit this from inner" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:53:7:53:64 | m(...) | Test.java:53:29:53:63 | "indirect explicit this from inner" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<String> |
+| Test.java:60:5:60:48 | m(...) | Test.java:60:10:60:47 | "indirect implicit this (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<R> |
+| Test.java:61:5:61:53 | m(...) | Test.java:61:15:61:52 | "indirect explicit this (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<R> |
+| Test.java:66:7:66:61 | m(...) | Test.java:66:12:66:60 | "indirect implicit this from inner (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<R> |
+| Test.java:67:7:67:80 | m(...) | Test.java:67:31:67:79 | "indirect explicit this from inner (generic sub)" | Gen.class:0:0:0:0 | m | Gen.class:0:0:0:0 | Gen<R> |
+| Test.java:79:7:79:52 | m(...) | Test.java:79:9:79:51 | "direct implicit this, from inner to inner" | UninvolvedOuter$InnerGen.class:0:0:0:0 | m | UninvolvedOuter$InnerGen.class:0:0:0:0 | InnerGen<String> |
+| Test.java:80:7:80:57 | m(...) | Test.java:80:14:80:56 | "direct explicit this, from inner to inner" | UninvolvedOuter$InnerGen.class:0:0:0:0 | m | UninvolvedOuter$InnerGen.class:0:0:0:0 | InnerGen<String> |

java/ql/test/library-tests/implicit-this-type/test.ql

@@ -0,0 +1,5 @@
+import java
+
+from MethodAccess ma
+select ma, ma.getAnArgument().getAChildExpr*().(StringLiteral), ma.getCallee(),
+  ma.getCallee().getDeclaringType()

java/ql/test/query-tests/AmbiguousOuterSuper/AmbiguousOuterSuper.expected

@@ -0,0 +1,2 @@
+| GenericTest.java:14:7:14:9 | f(...) | A $@ is called instead of a $@. | GenericTest.class:0:0:0:0 | f | method declared in a superclass | GenericTest.java:9:8:9:8 | f | method with the same signature in an enclosing class |
+| Test.java:14:7:14:9 | f(...) | A $@ is called instead of a $@. | Test.java:3:8:3:8 | f | method declared in a superclass | Test.java:9:8:9:8 | f | method with the same signature in an enclosing class |

java/ql/test/query-tests/AmbiguousOuterSuper/AmbiguousOuterSuper.qlref

@@ -0,0 +1 @@
+Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql

java/ql/test/query-tests/AmbiguousOuterSuper/GenericTest.java

@@ -0,0 +1,19 @@
+public class GenericTest<T> {
+
+  void f() { }
+
+}
+
+class Outer2 {
+
+  void f() { }
+
+  class Inner<T> extends GenericTest<T> {
+
+    public void test() {
+      f();
+    }
+
+  }
+
+}

java/ql/test/query-tests/AmbiguousOuterSuper/Test.java

@@ -0,0 +1,19 @@
+public class Test {
+
+  void f() { }
+
+}
+
+class Outer {
+
+  void f() { }
+
+  class Inner extends Test {
+
+    public void test() {
+      f();
+    }
+
+  }
+
+}

java/ql/test/query-tests/ExposeRepresentation/ExposeRepresentation.expected

@@ -0,0 +1,7 @@
+| ExposesRep.java:11:19:11:28 | getStrings | getStrings exposes the internal representation stored in field strings. The value may be modified $@. | User.java:5:5:5:19 | User.java:5:5:5:19 | after this call to getStrings |
+| ExposesRep.java:11:19:11:28 | getStrings | getStrings exposes the internal representation stored in field strings. The value may be modified $@. | User.java:13:12:13:26 | User.java:13:12:13:26 | after this call to getStrings |
+| ExposesRep.java:11:19:11:28 | getStrings | getStrings exposes the internal representation stored in field strings. The value may be modified $@. | User.java:38:12:38:26 | User.java:38:12:38:26 | after this call to getStrings |
+| ExposesRep.java:13:30:13:41 | getStringMap | getStringMap exposes the internal representation stored in field stringMap. The value may be modified $@. | User.java:9:5:9:21 | User.java:9:5:9:21 | after this call to getStringMap |
+| ExposesRep.java:17:15:17:24 | setStrings | setStrings exposes the internal representation stored in field strings. The value may be modified $@. | User.java:22:5:22:6 | User.java:22:5:22:6 | through the variable ss |
+| ExposesRep.java:21:15:21:26 | setStringMap | setStringMap exposes the internal representation stored in field stringMap. The value may be modified $@. | User.java:27:5:27:5 | User.java:27:5:27:5 | through the variable m |
+| ExposesRep.java:29:14:29:21 | getArray | getArray exposes the internal representation stored in field array. The value may be modified $@. | User.java:31:5:31:18 | User.java:31:5:31:18 | after this call to getArray |

java/ql/test/query-tests/ExposeRepresentation/ExposeRepresentation.qlref

@@ -0,0 +1 @@
+Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql

java/ql/test/query-tests/ExposeRepresentation/ExposesRep.java

@@ -0,0 +1,30 @@
+import java.util.Map;
+
+public class ExposesRep {
+  private String[] strings;
+  private Map<String, String> stringMap;
+
+  public ExposesRep() {
+    strings = new String[1];
+  }
+
+  public String[] getStrings() { return strings; }
+
+  public Map<String, String> getStringMap() {
+    return stringMap;
+  }
+
+  public void setStrings(String[] ss) {
+    this.strings = ss;
+  }
+
+  public void setStringMap(Map<String, String> m) {
+    this.stringMap = m;
+  }
+}
+
+class GenericExposesRep<T> {
+  private T[] array;
+
+  public T[] getArray() { return array; }
+}

java/ql/test/query-tests/ExposeRepresentation/User.java

@@ -0,0 +1,45 @@
+import java.util.Map;
+
+public class User {
+  public static void test1(ExposesRep er) {
+    er.getStrings()[0] = "Hello world";
+  }
+
+  public static void test2(ExposesRep er) {
+    er.getStringMap().put("Hello", "world");
+  }
+
+  public String[] indirectGetStrings(ExposesRep er) {
+    return er.getStrings();
+  }
+
+  public void test3(ExposesRep er) {
+    indirectGetStrings(er)[0] = "Hello world";
+  }
+
+  public static void test4(ExposesRep er, String[] ss) {
+    er.setStrings(ss);
+    ss[0] = "Hello world";
+  }
+
+  public static void test5(ExposesRep er, Map<String, String> m) {
+    er.setStringMap(m);
+    m.put("Hello", "world");
+  }
+
+  public static void test6(GenericExposesRep<String> ger) {
+    ger.getArray()[0] = "Hello world";
+  }
+}
+
+class GenericUser<T> {
+
+  public String[] indirectGetStrings(ExposesRep er) {
+    return er.getStrings();
+  }
+
+  public static void test1(ExposesRep er, GenericUser<String> gu) {
+    gu.indirectGetStrings(er)[0] = "Hello world";
+  }
+
+}

java/ql/test/query-tests/IteratorRemoveMayFail/IteratorRemoveMayFail.expected

@@ -1,2 +1,3 @@
 | Test.java:16:5:16:17 | remove(...) | This call may fail when iterating over the collection created $@, since it does not support element removal. | Test.java:29:16:29:32 | asList(...) | here |
 | Test.java:16:5:16:17 | remove(...) | This call may fail when iterating over the collection created $@, since it does not support element removal. | Test.java:33:16:33:43 | singletonList(...) | here |
+| Test.java:44:3:44:23 | remove(...) | This call may fail when iterating over the collection created $@, since it does not support element removal. | Test.java:52:15:52:31 | asList(...) | here |

java/ql/test/query-tests/IteratorRemoveMayFail/Test.java

@@ -36,4 +36,20 @@ public static A mkA2(int i) {
 	public List<Integer> getL() {
 		return l;
 	}
+}
+
+class Parent<T> {
+
+	public void removeFirst(List<T> l) {
+		l.iterator().remove();
+	}
+
+}
+
+class Child extends Parent<String> {
+
+	public void test(String... ss) {
+		removeFirst(Arrays.asList(ss));
+	}
+
 }