Skip to content

Java: do not generate models for toString and lambda flow methods #15772

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

Merged
owen-mc merged 5 commits into from
Mar 4, 2024
Merged

Java: do not generate models for toString and lambda flow methods #15772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bbf3fa7
do not generate models for toString
owen-mc Mar 1, 2024
bb97df1
do not generate models for lambda flow methods
owen-mc Mar 1, 2024
6e63df9
Accept test change: toString method no longer generated
owen-mc Mar 1, 2024
5399d88
Accept test change: slight change in gen vs man modelgen stats
owen-mc Mar 1, 2024
114c17a
Add more methods of `java.util.Comparator`
owen-mc Mar 2, 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
16 changes: 16 additions & 0 deletions java/ql/lib/semmle/code/java/dataflow/internal/ModelExclusions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,27 @@ private predicate isInternal(CompilationUnit cu) {
cu.getPackage().getName().matches("%internal%")
}

/** A method relating to lambda flow. */
private class LambdaFlowMethod extends Method {
LambdaFlowMethod() {
this.hasQualifiedName("java.lang", "Runnable", "run") or
this.hasQualifiedName("java.util", "Comparator",
["comparing", "comparingDouble", "comparingInt", "comparingLong"]) or
this.hasQualifiedName("java.util.function", "BiConsumer", "accept") or
this.hasQualifiedName("java.util.function", "BiFunction", "apply") or
this.hasQualifiedName("java.util.function", "Consumer", "accept") or
this.hasQualifiedName("java.util.function", "Function", "apply") or
this.hasQualifiedName("java.util.function", "Supplier", "get")
}
}

/** Holds if the given callable is not worth modeling. */
predicate isUninterestingForModels(Callable c) {
isInTestFile(c.getCompilationUnit().getFile()) or
isInternal(c.getCompilationUnit()) or
c instanceof MainMethod or
c instanceof ToStringMethod or
c instanceof LambdaFlowMethod or

@atorralba atorralba Mar 1, 2024 •
edited
Loading

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.

Would it make sense to use FunctionalInterface instead? i.e.

Suggested change
c instanceof LambdaFlowMethod or
c = any(FunctionalInterface f).getRunMethod() or

@atorralba atorralba Mar 1, 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.

Hm, nvm, some of those aren't functional interfaces.

@owen-mc owen-mc Mar 1, 2024

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.

Hmm, I might be able to use that to replace some of the hard-coded method names. I'll have a play around with it.

@owen-mc owen-mc Mar 2, 2024

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.

It gave almost 7000 results! I think it's best to just hard-code a few for now and if we find we want to do more in future then we can come up with a clever way to characterise them.

@atorralba atorralba Mar 4, 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.

Makes sense 👍

c instanceof StaticInitializer or
exists(FunctionalExpr funcExpr | c = funcExpr.asMethod()) or
c.getDeclaringType() instanceof TestLibrary or
Expand Down
14 changes: 7 additions & 7 deletions ...test/query-tests/Metrics/GeneratedVsManualCoverage/TopJdkApisTest/TopJdkApisTest.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
| java.awt | 0 | 0 | 2 | 1 | 3 | 0.6666666666666666 | 0.0 | 0.6666666666666666 | 0.0 | NaN | 0.3333333333333333 |
| java.io | 0 | 0 | 22 | 15 | 37 | 0.5945945945945946 | 0.0 | 0.5945945945945946 | 0.0 | NaN | 0.40540540540540543 |
| java.lang | 0 | 0 | 62 | 94 | 156 | 0.3974358974358974 | 0.0 | 0.3974358974358974 | 0.0 | NaN | 0.6025641025641025 |
| java.io | 0 | 0 | 21 | 15 | 36 | 0.5833333333333334 | 0.0 | 0.5833333333333334 | 0.0 | NaN | 0.4166666666666667 |
| java.lang | 0 | 0 | 57 | 88 | 145 | 0.3931034482758621 | 0.0 | 0.3931034482758621 | 0.0 | NaN | 0.6068965517241379 |
| java.lang.invoke | 0 | 0 | 0 | 1 | 1 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.lang.reflect | 0 | 0 | 0 | 4 | 4 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.math | 0 | 0 | 0 | 16 | 16 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.net | 0 | 0 | 5 | 0 | 5 | 1.0 | 0.0 | 1.0 | 0.0 | NaN | 0.0 |
| java.math | 0 | 0 | 0 | 15 | 15 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.net | 0 | 0 | 4 | 0 | 4 | 1.0 | 0.0 | 1.0 | 0.0 | NaN | 0.0 |
| java.nio | 0 | 0 | 2 | 3 | 5 | 0.4 | 0.0 | 0.4 | 0.0 | NaN | 0.6 |
| java.nio.charset | 0 | 0 | 0 | 1 | 1 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.nio.file | 0 | 0 | 7 | 1 | 8 | 0.875 | 0.0 | 0.875 | 0.0 | NaN | 0.125 |
| java.nio.file | 0 | 0 | 6 | 1 | 7 | 0.8571428571428571 | 0.0 | 0.8571428571428571 | 0.0 | NaN | 0.14285714285714285 |
| java.sql | 0 | 0 | 1 | 15 | 16 | 0.0625 | 0.0 | 0.0625 | 0.0 | NaN | 0.9375 |
| java.text | 0 | 0 | 0 | 5 | 5 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.time | 0 | 0 | 0 | 17 | 17 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.time.chrono | 0 | 0 | 0 | 1 | 1 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.time.format | 0 | 0 | 0 | 2 | 2 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.util | 0 | 0 | 86 | 66 | 152 | 0.5657894736842105 | 0.0 | 0.5657894736842105 | 0.0 | NaN | 0.4342105263157895 |
| java.util | 0 | 0 | 86 | 64 | 150 | 0.5733333333333334 | 0.0 | 0.5733333333333334 | 0.0 | NaN | 0.4266666666666667 |
| java.util.concurrent | 0 | 0 | 9 | 9 | 18 | 0.5 | 0.0 | 0.5 | 0.0 | NaN | 0.5 |
| java.util.concurrent.atomic | 0 | 0 | 2 | 11 | 13 | 0.15384615384615385 | 0.0 | 0.15384615384615385 | 0.0 | NaN | 0.8461538461538461 |
| java.util.concurrent.locks | 0 | 0 | 0 | 2 | 2 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.util.function | 0 | 0 | 0 | 6 | 6 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.util.function | 0 | 0 | 0 | 1 | 1 | 0.0 | 0.0 | 0.0 | NaN | NaN | 1.0 |
| java.util.logging | 0 | 0 | 1 | 1 | 2 | 0.5 | 0.0 | 0.5 | 0.0 | NaN | 0.5 |
| java.util.regex | 0 | 0 | 3 | 1 | 4 | 0.75 | 0.0 | 0.75 | 0.0 | NaN | 0.25 |
| java.util.stream | 0 | 0 | 18 | 8 | 26 | 0.6923076923076923 | 0.0 | 0.6923076923076923 | 0.0 | NaN | 0.3076923076923077 |
1 change: 0 additions & 1 deletion java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
| p;Joiner;false;merge;(Joiner);;Argument[this];ReturnValue;value;df-generated |
| p;Joiner;false;setEmptyValue;(CharSequence);;Argument[0];Argument[this];taint;df-generated |
| p;Joiner;false;setEmptyValue;(CharSequence);;Argument[this];ReturnValue;value;df-generated |
| p;Joiner;false;toString;();;Argument[this];ReturnValue;taint;df-generated |
| p;MultipleImpl2$IInterface;true;m;(Object);;Argument[0];ReturnValue;taint;df-generated |
| p;MultipleImpls$Strat2;true;getValue;();;Argument[this];ReturnValue;taint;df-generated |
| p;MultipleImpls$Strategy;true;doSomething;(String);;Argument[0];Argument[this];taint;df-generated |
Expand Down