Skip to content

Java: Fix ClassInstanceExpr::isDiamond not working for anonymous classes #15429

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 2 commits into
base: main
Choose a base branch
from
Open

Java: Fix ClassInstanceExpr::isDiamond not working for anonymous classes #15429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b784ecf
Java: Fix `ClassInstanceExpr::isDiamond` not working for anonymous cl…
Marcono1234 Jan 24, 2024
22d98ae
Merge branch 'main' into marcono1234/anonymous-isDiamond
Marcono1234 Feb 19, 2024
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
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2024-01-24-anonymous-diamond.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: fix
---
* Fixed `ClassInstanceExpr::isDiamond` not working for anonymous classes.
9 changes: 8 additions & 1 deletion java/ql/lib/semmle/code/java/Expr.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,7 +1243,14 @@ class ClassInstanceExpr extends Expr, ConstructorCall, @classinstancexpr {
* empty type argument list of the form `<>`.
*/
predicate isDiamond() {
this.getType() instanceof ParameterizedClass and
(
this.getType() instanceof ParameterizedClass
or
this.getAnonymousClass().getASupertype() instanceof ParameterizedType and
// Ignore Kotlin code; otherwise seems to erroneously match `object` expression with explicit
// type arguments, possibly because extractor is not properly extracting the type args?
not this.getCompilationUnit().isKotlinSourceFile()

@smowton smowton Feb 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@igfoo is there a quick fix for the Kotlin case rather than hack Kotlin source out like this? @Marcono1234 for clarity could you show a Kotlin snippet that otherwise behaves badly?

@Marcono1234 Marcono1234 Feb 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I omit that exclusion of Kotlin source files and then run the following query on the database for https://github.com/Kotlin/kotlinx.coroutines (latest database from GitHub was built with CodeQL CLI 2.16.1)

import java

from ClassInstanceExpr e
where
  e.isDiamond() and
  e.getCompilationUnit().isKotlinSourceFile()
select e

it finds for example this expression with an explicit type argument:
https://github.com/Kotlin/kotlinx.coroutines/blob/8c516f5ab1fcc39629d2838489598135eedd7b80/kotlinx-coroutines-core/common/test/AbstractCoroutineTest.kt#L13C25-L13C59

Though maybe the isDiamond predicate does not make sense for Kotlin in the first place; at least there is (to my knowledge) no <> syntax but instead the type arguments are simply omitted if they can be inferred by the compiler.
Or at least the name isDiamond might be a bit confusing then.

@igfoo igfoo Feb 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, that looks like a bug in our Kotlin support at the moment. Excluding like this looks fine for now. I've made an internal ticket to remind us to come back to this.

) and
not exists(this.getATypeArgument())
}

Expand Down
25 changes: 22 additions & 3 deletions java/ql/test/library-tests/java7/Diamond/Diamond.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,32 @@
public class Diamond {
// normal parameterized class instance expressions
List<Integer> list = new ArrayList<Integer>();
Map<String, Object> map = new HashMap<String, Object>();
Map<String, Object> map = new HashMap<String, Object>();

// same, but with diamond
List<Integer> diamond_list = new ArrayList<>();
Map<String, Object> diamond_map = new HashMap<>();


// anonymous without diamond
Object obj = new Object() {};
Runnable runnable = new Runnable() {
@Override
public void run() {
}
};
List<Integer> list2 = new ArrayList<Integer>() {};

// anonymous with diamond (supported since Java 9)
List<Integer> list3 = new ArrayList<>() {};
Map<String, Object> map2 = new HashMap<>() {
@Override
public String toString() {
return "custom map";
}
};

// other class instance expressions
List l = new ArrayList();
List l2 = new ArrayList() {};
Error e = new Error();
}
2 changes: 2 additions & 0 deletions java/ql/test/library-tests/java7/Diamond/Diamonds.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
| Diamond.java:14:31:14:47 | new ArrayList<Integer>(...) |
| Diamond.java:15:36:15:50 | new HashMap<String,Object>(...) |
| Diamond.java:27:24:27:43 | new (...) |
| Diamond.java:28:29:33:2 | new (...) |
63 changes: 55 additions & 8 deletions java/ql/test/library-tests/java7/Diamond/PrintAst.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,58 @@ Diamond.java:
# 15| 1: [TypeAccess] Object
# 15| 0: [ClassInstanceExpr] new HashMap<String,Object>(...)
# 15| -3: [TypeAccess] HashMap<String,Object>
# 18| 7: [FieldDeclaration] List<> l;
# 18| -1: [TypeAccess] List<>
# 18| 0: [ClassInstanceExpr] new ArrayList<>(...)
# 18| -3: [TypeAccess] ArrayList<>
# 19| 8: [FieldDeclaration] Error e;
# 19| -1: [TypeAccess] Error
# 19| 0: [ClassInstanceExpr] new Error(...)
# 19| -3: [TypeAccess] Error
# 18| 7: [FieldDeclaration] Object obj;
# 18| -1: [TypeAccess] Object
# 18| 0: [ClassInstanceExpr] new (...)
# 18| -4: [AnonymousClass] new Object(...) { ... }
# 18| -3: [TypeAccess] Object
# 19| 8: [FieldDeclaration] Runnable runnable;
# 19| -1: [TypeAccess] Runnable
# 19| 0: [ClassInstanceExpr] new (...)
# 19| -4: [AnonymousClass] new Runnable(...) { ... }
# 21| 2: [Method] run
#-----| 1: (Annotations)
# 20| 1: [Annotation] Override
# 21| 3: [TypeAccess] void
# 21| 5: [BlockStmt] { ... }
# 19| -3: [TypeAccess] Runnable
# 24| 9: [FieldDeclaration] List<Integer> list2;
# 24| -1: [TypeAccess] List<Integer>
# 24| 0: [TypeAccess] Integer
# 24| 0: [ClassInstanceExpr] new (...)
# 24| -4: [AnonymousClass] new ArrayList<Integer>(...) { ... }
# 24| -3: [TypeAccess] ArrayList<Integer>
# 24| 0: [TypeAccess] Integer
# 27| 10: [FieldDeclaration] List<Integer> list3;
# 27| -1: [TypeAccess] List<Integer>
# 27| 0: [TypeAccess] Integer
# 27| 0: [ClassInstanceExpr] new (...)
# 27| -4: [AnonymousClass] new ArrayList<Integer>(...) { ... }
# 27| -3: [TypeAccess] ArrayList<Integer>
# 28| 11: [FieldDeclaration] Map<String,Object> map2;
# 28| -1: [TypeAccess] Map<String,Object>
# 28| 0: [TypeAccess] String
# 28| 1: [TypeAccess] Object
# 28| 0: [ClassInstanceExpr] new (...)
# 28| -4: [AnonymousClass] new HashMap<String,Object>(...) { ... }
# 30| 2: [Method] toString
#-----| 1: (Annotations)
# 29| 1: [Annotation] Override
# 30| 3: [TypeAccess] String
# 30| 5: [BlockStmt] { ... }
# 31| 0: [ReturnStmt] return ...
# 31| 0: [StringLiteral] "custom map"
# 28| -3: [TypeAccess] HashMap<String,Object>
# 36| 12: [FieldDeclaration] List<> l;
# 36| -1: [TypeAccess] List<>
# 36| 0: [ClassInstanceExpr] new ArrayList<>(...)
# 36| -3: [TypeAccess] ArrayList<>
# 37| 13: [FieldDeclaration] List<> l2;
# 37| -1: [TypeAccess] List<>
# 37| 0: [ClassInstanceExpr] new (...)
# 37| -4: [AnonymousClass] new ArrayList<>(...) { ... }
# 37| -3: [TypeAccess] ArrayList<>
# 38| 14: [FieldDeclaration] Error e;
# 38| -1: [TypeAccess] Error
# 38| 0: [ClassInstanceExpr] new Error(...)
# 38| -3: [TypeAccess] Error
1 change: 1 addition & 0 deletions java/ql/test/library-tests/java7/Diamond/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 9 -target 9