ruby: clean up and simplify query

- remove PluckCall, this only removes 1 out of 196 results
- add and clean up comments

Author: yoff

ruby/ql/src/queries/performance/CouldBeHoisted.ql

@@ -8,12 +8,15 @@
  * @tags performance
  */
 
-// Consider also Associations.
-// Associations are lazy-loading by default, so something like
-// in a loop over article do
-//    article.book
-// if you have 1000 articles it will do a 1000 calls to book.
-// If you already did article includes book, there should be no problem.
+// Possible Improvements;
+// - Consider also Associations.
+//   Associations are lazy-loading by default, so something like
+//   in a loop over `article` do
+//      `article.book`
+//   if you have 1000 articles it will do a 1000 calls to `book`.
+//   If you already did `article includes book`, there should be no problem.
+// - Consider instances of ActiveRecordInstanceMethodCall, for instance
+//   calls to `pluck`.
 import ruby
 private import codeql.ruby.AST
 import codeql.ruby.ast.internal.Constant
@@ -54,18 +57,6 @@ predicate happensInInnermostLoop(LoopingCall loop, DataFlow::CallNode e) {
   not happensInOuterLoop(loop, e)
 }
 
-private class PluckCall extends ActiveRecordInstanceMethodCall {
-  PluckCall() { this.getMethodName() in ["pluck"] }
-
-  ActiveRecordInstance chaines() { result = getChain(this) }
-}
-
-private ActiveRecordInstance getChain(ActiveRecordInstanceMethodCall c) {
-  result = c.getInstance()
-  or
-  result = getChain(c.getInstance())
-}
-
 // The ActiveRecord instance is used to potentially control the loop
 predicate usedInLoopControlGuard(ActiveRecordInstance ar, DataFlow::Node guard) {
   TaintTracking::localTaint(ar, guard) and
@@ -85,14 +76,15 @@ DataFlow::Node guardForLoopControl(ConditionalExpr cond, Stmt control) {
 
 from LoopingCall loop, DataFlow::CallNode call
 where
-  // Filter loops over constants
+  // Disregard loops over constants
   not isArrayConstant(loop.getReceiver().asExpr(), _) and
-  // Filter tests
+  // Disregard tests
   not call.getLocation().getFile().getAbsolutePath().matches("%test%") and
-  not call = any(PluckCall p).chaines() and
+  // Disregard cases where the looping is influenced by the query result
   not usedInLoopControlGuard(call, _) and
+  // Only report the inner most loop
   happensInInnermostLoop(loop, call) and
-  (call instanceof ActiveRecordModelFinderCall or call instanceof PluckCall) and
-  not call.getMethodName() in ["new", "create"] and
-  call.getLocation().getFile().getAbsolutePath().matches(["%app/models/s%", "%app/models/e%"])
+  // Only report calls that are likely to be expensive
+  call instanceof ActiveRecordModelFinderCall and
+  not call.getMethodName() in ["new", "create"]
 select call, "This call happens inside $@, and could be hoisted.", loop, "this loop"