Java: Add MemberType

[Marcono1234]

Resolves #3708

Adds the class MemberType extending NestedType which models a member class or member interface as specified by JLS 16 §8.5.

This might also require some follow-up work for predicates which currently only consider Field or Callable as member, see also #5596.

(Though the documentation changes might need some tweaking when #5564 is handled.)

[smowton]

Perhaps your warning about inner classes should mention that there can be more than one layer of nesting -- for example, what might in source be written Outer.Middle.Inner would have name Outer$Middle$Inner. This is the same binary name format described at https://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.1

[Marcono1234]

The documentation comments about $ were mainly to give a hint to the user indicating that $ instead of . is used.
I have now force-pushed changes which at least mention that the enclosing type might be nested as well. Explicitly mentioning that the name of the enclosing nested type also uses $ might be a little bit verbose.

Do you think it suffices like this?

[smowton]

The docs look ok to me. Bear in mind though that there is QL code that we don't control, e.g. codebase-specific customisations made by customers. We would need a solution here that doesn't change the meaning of the existing type Member.

[Marcono1234]

We would need a solution here that doesn't change the meaning of the existing type Member.

The effective behavior of Member does not change (see also #3708):

import java

from NestedType n
where n instanceof Member
select n

Query Console link

Due to how the characteristics predicate of Member is defined, it was already matching member NestedTypes before. However there was no explicit class which made this fact obvious.

I am a little bit skeptical about a "solution here that doesn't change the meaning of the existing type Member". The JLS section referenced in the description of this pull request (as well as other JLS sections) explicitly call these nested classes "members". Though feel free to propose your solution.

[smowton]

Problem with this:

If we have class Outer { class Inner { } }, previously Inner.getDeclaringType() returned Outer via route getDeclaringType() -> declaresMember -> enclInRefType [primitive] and-not anonymous [primitive] and-not local [primitive].

However now you have getDeclaringType() -> declaresMember -> (instanceof MemberType) -> (instanceof Member) -> declaresMember. The circular dependency will prevent any NestedType from being considered a Member.

[Marcono1234]

Thanks for pointing that out and sorry for the trouble! My previous changes indeed completely broke matching member types at all. I have reverted the changes to declaresMember and it now seems to work as expected.

[smowton]

Test suite failures:

--- expected
+++ actual
@@ -1,4 +1,3 @@
 | Resource.async_completionstage |
-| Resource.async_promise |
 | Resource.index |
 | Resource.session_redirect_me |
Error: [5/8] [2/108 comp 23s eval 1.2s] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/java/ql/test/library-tests/frameworks/play/PlayControllerActionMethod.ql
--- expected
+++ actual
@@ -1,3 +1,2 @@
 | resources/Resource.java:19:37:19:46 | uri |
-| resources/Resource.java:24:42:24:53 | token |
 | resources/Resource.java:28:56:28:65 | uri |
Error: [5/8] [3/108 comp 10.9s eval 597ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/java/ql/test/library-tests/frameworks/play/PlayActionMethodQueryParameter.ql

--- expected
+++ actual
@@ -27,9 +27,6 @@
 | PlayResource.java:20:18:20:48 | getQueryString(...) | ../../../stubs/playframework-2.6.x/play/mvc/Results.java:634:33:634:42 | url |
 | PlayResource.java:20:18:20:48 | getQueryString(...) | PlayResource.java:20:18:20:48 | getQueryString(...) |
 | PlayResource.java:20:18:20:48 | getQueryString(...) | PlayResource.java:21:21:21:23 | url |
-| PlayResource.java:24:42:24:53 | token | ../../../stubs/playframework-2.6.x/play/mvc/Results.java:94:27:94:40 | content |
-| PlayResource.java:24:42:24:53 | token | PlayResource.java:24:42:24:53 | token |
-| PlayResource.java:24:42:24:53 | token | PlayResource.java:25:30:25:34 | token |
 | PlayResource.java:28:56:28:65 | uri | PlayResource.java:28:56:28:65 | uri |
 | RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:4:30:4:40 | path |
 | RmiFlowImpl.java:4:30:4:40 | path | RmiFlowImpl.java:5:20:5:31 | ... + ... |
Error: [4/8] [53/107 comp 16.1s eval 4.2s] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/java/ql/test/library-tests/dataflow/taintsources/remote.ql

These should be reproducible with a local codeql test run java/ql/test/library-tests/dataflow/taintsources java/ql/test/library-tests/frameworks/play

[Marcono1234]

The problem was that PlayAsyncResultPromise was matching a member type but was using Member.getQualifiedName() (which is now overridden by these changes). Performing a quick text search I did not find other occurrences of such a usage, but hopefully it does not cause issues for user code.

I have also modified the documentation for the qualified name predicates to explain in more detail which structure the qualified name has.

[Marcono1234]

As example for a qualified name of a member type I chose java.util.Map.Entry since it is probably one of the most well-known ones.
However, the qualified name I used in the QLDoc (java.util.Map$Entry) does not actually work due to #5672.
Should I use a different example instead?

[smowton]

Added change note; changed example to Thread$State