Java: Model more of Apache Commons Collections#6370
Conversation
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,762,,,,,,,,
- Totals,,84,2465,296,13,6,6,107,33,1,66
+ Totals,,84,3029,296,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,381,,,,,,,,,,,,,,,10,371
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,381,,,,,,,,,,,,,,,10,371 |
67eeff1 to
af0dab9
Compare
java/ql/test/library-tests/frameworks/apache-collections/Test.java
Outdated
Show resolved
Hide resolved
java/ql/test/library-tests/frameworks/apache-collections/Test.java
Outdated
Show resolved
Hide resolved
java/ql/test/library-tests/frameworks/apache-collections/test.ql
Outdated
Show resolved
Hide resolved
java/ql/test/library-tests/frameworks/apache-collections/Test.java
Outdated
Show resolved
Hide resolved
|
There are btw. also a number of casts that could probably be removed from the generated test. |
af0dab9 to
0a39995
Compare
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,762,,,,,,,,
- Totals,,84,2705,360,13,6,6,107,33,1,66
+ Totals,,84,3269,360,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,381,,,,,,,,,,,,,,,,,,10,371
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,381,,,,,,,,,,,,,,,,,,10,371 |
0a39995 to
7862d4a
Compare
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,760,,,,,,,,
- Totals,,84,2705,398,13,6,6,107,33,1,66
+ Totals,,84,3267,398,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,380,,,,,,,,,,,,,,,,,,,10,370
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,380,,,,,,,,,,,,,,,,,,,10,370 |
|
@atorralba Feel free to take the review from here and merge when you're happy. |
atorralba
left a comment
There was a problem hiding this comment.
I added some inline comments (some may be dismissible because of my lack of knowledge), otherwise LGTM.
The package name was org.apache.commons.collection until release 4.0.
7862d4a to
1997dfb
Compare
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,784,,,,,,,,
- Totals,,84,2705,398,13,6,6,107,33,1,66
+ Totals,,84,3291,398,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,392,,,,,,,,,,,,,,,,,,,16,376
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,392,,,,,,,,,,,,,,,,,,,16,376 |
smowton
left a comment
There was a problem hiding this comment.
Reviewed the collections themselves; various Utils classes to do.
| // Note that when lambdas are supported we should model things relating to Closure, Factory, Transformer, FluentIterable.forEach, FluentIterable.transform | ||
| "org.apache.commons.collections4;ArrayStack;true;peek;;;Element of Argument[-1];ReturnValue;value", | ||
| "org.apache.commons.collections4;ArrayStack;true;pop;;;Element of Argument[-1];ReturnValue;value", | ||
| "org.apache.commons.collections4;ArrayStack;true;push;;;Argument[0];Element of Argument[-1];value", |
There was a problem hiding this comment.
| "org.apache.commons.collections4;ArrayStack;true;push;;;Argument[0];Element of Argument[-1];value", | |
| "org.apache.commons.collections4;ArrayStack;true;push;;;Argument[0];Element of Argument[-1];value", | |
| "org.apache.commons.collections4;ArrayStack;true;push;;;Argument[0];ReturnValue;value", |
| "org.apache.commons.collections4;FluentIterable;true;skip;;;Element of Argument[-1];Element of ReturnValue;value", | ||
| "org.apache.commons.collections4;FluentIterable;true;toArray;;;Element of Argument[-1];ArrayElement of ReturnValue;value", | ||
| "org.apache.commons.collections4;FluentIterable;true;toList;;;Element of Argument[-1];Element of ReturnValue;value", | ||
| "org.apache.commons.collections4;FluentIterable;true;toString;;;Element of Argument[-1];ReturnValue;taint", |
There was a problem hiding this comment.
Might have missed a discussion on this, but I think we're still not routinely propagating taint across stringification
| "org.apache.commons.collections4;MapIterator;true;setValue;;;Argument[0];MapValue of Argument[-1];value", | ||
| "org.apache.commons.collections4;MultiMap;true;get;;;Element of MapValue of Argument[-1];Element of ReturnValue;value", | ||
| "org.apache.commons.collections4;MultiMap;true;put;;;Argument[0];MapKey of Argument[-1];value", | ||
| "org.apache.commons.collections4;MultiMap;true;put;;;Argument[1];Element of MapValue of Argument[-1];value", |
There was a problem hiding this comment.
| "org.apache.commons.collections4;MultiMap;true;put;;;Argument[1];Element of MapValue of Argument[-1];value", | |
| "org.apache.commons.collections4;MultiMap;true;put;;;Argument[1];Element of MapValue of Argument[-1];value", | |
| "org.apache.commons.collections4;MultiMap;true;remove;;;Element of MapValue of Argument[-1];Element of ReturnValue;value", |
There was a problem hiding this comment.
This is inherited from Get and Map. Or do you think that the extra Element of components are needed?
There was a problem hiding this comment.
I see, no problem then. I don't think the extra Element of is needed (though check -- no need to use the test generator, just add a little test method that adds something to a MultiMap and then removes it and see if taint is tracked)
There was a problem hiding this comment.
I checked and it worked.
| "org.apache.commons.collections4;MultiSet;true;uniqueSet;;;Element of Argument[-1];Element of ReturnValue;value", | ||
| "org.apache.commons.collections4;MultiSet;true;entrySet;;;Element of Argument[-1];Element of Element of ReturnValue;value", | ||
| "org.apache.commons.collections4;MultiValuedMap;true;asMap;;;MapKey of Argument[-1];MapKey of ReturnValue;value", | ||
| "org.apache.commons.collections4;MultiValuedMap;true;asMap;;;Element of MapValue of Argument[-1];Element of MapValue of ReturnValue;value", |
| "org.apache.commons.collections4;SortedBag;true;last;;;Element of Argument[-1];ReturnValue;value", | ||
| "org.apache.commons.collections4;Trie;true;prefixMap;;;MapKey of Argument[-1];MapKey of ReturnValue;value", | ||
| "org.apache.commons.collections4;Trie;true;prefixMap;;;MapValue of Argument[-1];MapValue of ReturnValue;value", | ||
| "org.apache.commons.collections;ArrayStack;true;peek;;;Element of Argument[-1];ReturnValue;value", |
There was a problem hiding this comment.
Only reviewed the collections4 models. Are the two sets of models identical or supposed to be? If so use something like "org.apache.commons.collections" + ["", "4"] + ";" + ["ArrayStack;true;peek;;;Element of Argument[-1];ReturnValue;value", ...]
This way we can at least guard against the models drifting apart by accident.
There was a problem hiding this comment.
My review assumed both sets were identical too. +1 to this suggestion.
We do not do this for other overrides of toString
And corresponding tests
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,776,,,,,,,,
- Totals,,84,2711,398,13,6,6,107,33,1,66
+ Totals,,84,3289,398,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,388,,,,,,,,,,,,,,,,,,,9,379
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,388,,,,,,,,,,,,,,,,,,,9,379 |
|
@smowton I've addressed all review comments. |
smowton
left a comment
There was a problem hiding this comment.
Comments on CollectionUtils
| ";CollectionUtils;true;select;;;Element of Argument[0];Element of Argument[2];value", | ||
| ";CollectionUtils;true;select;;;Element of Argument[0];Element of Argument[3];value", |
There was a problem hiding this comment.
Note these two also return their Argument[2] (distinct from the element-transfer to return value case which only applies to the one-arg overload)
There was a problem hiding this comment.
I don't understand your comment in parentheses. Are you saying L293 should be limited to the (Iterable,Predicate)?
There was a problem hiding this comment.
Yes, the Iterable, Predicate version should be modelled elem-of-arg0 -> elem-of-ret, whereas the Iterable, Predicate, Collection[, Collection] overloads should be modelled elem-of-arg0 -> elem-of-arg2 and arg2 -> ret (cf elem of arg(0 or 2) -> elem of ret which would obscure the fact that arg2 and ret are the same object)
There was a problem hiding this comment.
I was worried that we wouldn't get elem-of-arg0 -> elem-of-ret because "arg2 -> ret" would consider "arg2" from before this line is executed. But I just tested it and it works fine 😄 .
There was a problem hiding this comment.
Yeah, this works for similar reasons to fluent methods, where we might have elem-of-arg -> elem-of-qualifier and also qualifier -> ret.
| ";CollectionUtils;true;select;;;Element of Argument[0];Element of Argument[2];value", | ||
| ";CollectionUtils;true;select;;;Element of Argument[0];Element of Argument[3];value", | ||
| ";CollectionUtils;true;selectRejected;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";CollectionUtils;true;selectRejected;;;Element of Argument[0];Element of Argument[2];value", |
There was a problem hiding this comment.
Similarly, model the overload that takes an existing container as propagating elements to it and returning arg2 by value.
| ";CollectionUtils;true;selectRejected;;;Element of Argument[0];Element of Argument[2];value", | ||
| ";CollectionUtils;true;subtract;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";CollectionUtils;true;synchronizedCollection;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";CollectionUtils;true;transformingCollection;;;Element of Argument[0];Element of ReturnValue;value", |
There was a problem hiding this comment.
We haven't modelled other collection operations that apply some lambda; for consistency we probably shouldn't model this one either
There was a problem hiding this comment.
But the lambda isn't applied by this function: Existing entries in the specified collection will not be transformed.
There was a problem hiding this comment.
I'll add a comment like you ask for below.
There was a problem hiding this comment.
Ah yes, comment will do the trick
smowton
left a comment
There was a problem hiding this comment.
Comments on CollectionUtils and ListUtils
| ";ListUtils;true;sum;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";ListUtils;true;sum;;;Element of Argument[1];Element of ReturnValue;value", | ||
| ";ListUtils;true;synchronizedList;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";ListUtils;true;transformedList;;;Element of Argument[0];Element of ReturnValue;value", |
There was a problem hiding this comment.
Probably worth a comment on this and lazyList that these models represent the perhaps-surprising behaviour that existing list elements are retained unaltered by both methods
| ";IteratorUtils;true;filteredListIterator;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";IteratorUtils;true;find;;;Element of Argument[0];ReturnValue;value", | ||
| ";IteratorUtils;true;first;;;Element of Argument[0];ReturnValue;value", | ||
| ";IteratorUtils;true;get;;;Element of Argument[0];ReturnValue;value", |
There was a problem hiding this comment.
forEachButLast gets the last element in an iterator by value
| ";IterableUtils;true;filteredIterable;;;Element of Argument[0];Element of ReturnValue;value", | ||
| ";IterableUtils;true;find;;;Element of Argument[0];ReturnValue;value", | ||
| ";IterableUtils;true;first;;;Element of Argument[0];ReturnValue;value", | ||
| ";IterableUtils;true;get;;;Element of Argument[0];ReturnValue;value", |
There was a problem hiding this comment.
forEachButLast exists here too
smowton
left a comment
There was a problem hiding this comment.
Done reviewing; this looks good to me with the given minor adjustments
| ["org.apache.commons.collections4", "org.apache.commons.collections"] + | ||
| [ | ||
| ";MultiMapUtils;true;emptyIfNull;;;Argument[0];ReturnValue;value", | ||
| ";MultiMapUtils;true;getCollection;;;MapValue of Argument[0];ReturnValue;value", |
There was a problem hiding this comment.
I think these need an Element of on both sides, because it's creating a new Collection/Bag/List/Set to hold whatever values the MultiValuedMap holds for the key
There was a problem hiding this comment.
I don't think so. The source code is literally map.get(key) behind a null check.
There was a problem hiding this comment.
Ah ok, I thought it stored them internally as something different.
There was a problem hiding this comment.
You are however right about getValuesAsBag and its friends, so 75% right.
| row = | ||
| [ | ||
| //"package;type;overrides;name;signature;ext;inputspec;outputspec;kind", | ||
| "org.apache.commons.collections4.iterators;IteratorEnumeration;true;IteratorEnumeration;;;Element of Argument[0];Element of Argument[-1];value", |
There was a problem hiding this comment.
Add a comment indicating why the test models this
javaGenerated file changes for java
- `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,198,,,,,,,,
+ `Apache Commons Collections <https://commons.apache.org/proper/commons-collections/>`_,"``org.apache.commons.collections``, ``org.apache.commons.collections4``",,788,,,,,,,,
- Totals,,84,2711,398,13,6,6,107,33,1,66
+ Totals,,84,3301,398,13,6,6,107,33,1,66
- org.apache.commons.collections,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections,,,394,,,,,,,,,,,,,,,,,,,9,385
- org.apache.commons.collections4,,,99,,,,,,,,,,,,,,,,,,,4,95
+ org.apache.commons.collections4,,,394,,,,,,,,,,,,,,,,,,,9,385 |
|
I have pushed a commit fixing two small mistakes from #6138 which were caught by @atorralba after it was merged. As this adds so many models we will do a performance analysis before merging it. I believe that is the last thing left. |
smowton
left a comment
There was a problem hiding this comment.
Performance evaluation finished, using Apache Flink and Apache Hive (both heavy Collections users) on top of several existing light users in our dist-compare suite. No noticeable performance impact.
Model everything that is not in a subpackage