Address review comments #2

owen-mc · owen-mc · commit d2ca1fb2eb40 · 2021-12-08T11:20:29.000-05:00
diff --git a/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll b/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll
@@ -147,7 +147,7 @@ module ControlFlow {
     }
 
     /**
-     * Holds if this node sets the value of element `idx` on `base` (or its implicit dereference)
+     * Holds if this node sets the value of element `index` on `base` (or its implicit dereference)
      * to `rhs`.
      *
      * For example, for the assignment `xs[i] = v`, `base` is either the data-flow node
diff --git a/ql/lib/semmle/go/dataflow/ExternalFlow.qll b/ql/lib/semmle/go/dataflow/ExternalFlow.qll
@@ -190,38 +190,38 @@ predicate summaryModel(
 }
 
 /** Holds if `package` have CSV framework coverage. */
-private predicate relevantPackage(string package) {
+private predicate packageHasCsvCoverage(string package) {
   sourceModel(package, _, _, _, _, _, _, _) or
   sinkModel(package, _, _, _, _, _, _, _) or
   summaryModel(package, _, _, _, _, _, _, _, _)
 }
 
 /**
- * Holds if `shortpkg` and `longpkg` have CSV framework coverage and `shortpkg`
- * is a subpackage of `longpkg`.
+ * Holds if `package` and `subpkg` have CSV framework coverage and `subpkg`
+ * is a subpackage of `package`.
  */
-private predicate packageLink(string shortpkg, string longpkg) {
-  relevantPackage(shortpkg) and
-  relevantPackage(longpkg) and
-  longpkg.prefix(longpkg.indexOf(".")) = shortpkg
+private predicate packageHasASubpackage(string package, string subpkg) {
+  packageHasCsvCoverage(package) and
+  packageHasCsvCoverage(subpkg) and
+  subpkg.prefix(subpkg.indexOf(".")) = package
 }
 
 /**
  * Holds if `package` has CSV framework coverage and it is not a subpackage of
  * any other package with CSV framework coverage.
  */
 private predicate canonicalPackage(string package) {
-  relevantPackage(package) and not packageLink(_, package)
+  packageHasCsvCoverage(package) and not packageHasASubpackage(_, package)
 }
 
 /**
  * Holds if `package` and `subpkg` have CSV framework coverage, `subpkg` is a
  * subpackage of `package` (or they are the same), and `package` is not a
  * subpackage of any other package with CSV framework coverage.
  */
-private predicate canonicalPkgLink(string package, string subpkg) {
+private predicate canonicalPackageHasASubpackage(string package, string subpkg) {
   canonicalPackage(package) and
-  (subpkg = package or packageLink(package, subpkg))
+  (subpkg = package or packageHasASubpackage(package, subpkg))
 }
 
 /**
@@ -230,29 +230,29 @@ private predicate canonicalPkgLink(string package, string subpkg) {
  * which have CSV framework coverage (including `package` itself).
  */
 predicate modelCoverage(string package, int pkgs, string kind, string part, int n) {
-  pkgs = strictcount(string subpkg | canonicalPkgLink(package, subpkg)) and
+  pkgs = strictcount(string subpkg | canonicalPackageHasASubpackage(package, subpkg)) and
   (
     part = "source" and
     n =
       strictcount(string subpkg, string type, boolean subtypes, string name, string signature,
         string ext, string output |
-        canonicalPkgLink(package, subpkg) and
+        canonicalPackageHasASubpackage(package, subpkg) and
         sourceModel(subpkg, type, subtypes, name, signature, ext, output, kind)
       )
     or
     part = "sink" and
     n =
       strictcount(string subpkg, string type, boolean subtypes, string name, string signature,
         string ext, string input |
-        canonicalPkgLink(package, subpkg) and
+        canonicalPackageHasASubpackage(package, subpkg) and
         sinkModel(subpkg, type, subtypes, name, signature, ext, input, kind)
       )
     or
     part = "summary" and
     n =
       strictcount(string subpkg, string type, boolean subtypes, string name, string signature,
         string ext, string input, string output |
-        canonicalPkgLink(package, subpkg) and
+        canonicalPackageHasASubpackage(package, subpkg) and
         summaryModel(subpkg, type, subtypes, name, signature, ext, input, output, kind)
       )
   )
diff --git a/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll b/ql/lib/semmle/go/dataflow/internal/DataFlowNodes.qll
@@ -545,6 +545,10 @@ module Public {
     abstract predicate isParameterOf(DataFlowCallable c, int i);
   }
 
+  /**
+   * A summary node which represents a parameter in a function which doesn't
+   * already have a parameter nodes.
+   */
   class SummarizedParameterNode extends ParameterNode, MkSummarizedParameterNode {
     DataFlowCallable c;
     int i;
@@ -1085,18 +1089,20 @@ module Public {
    * A data-flow node representing an index of an array, map, slice or string defined from `range` statement.
    *
    * Example: in `i, _ := range y { ... }`, this represents the `Node` that extracts the index from the
-   * range statement, which will flow to `x`.
+   * range statement, which will flow to `i`.
    */
   class RangeIndexNode extends Node {
     DataFlow::Node base;
 
     RangeIndexNode() {
+      // when there is a comma, as in `i, x := range y { ... }`
       exists(IR::ExtractTupleElementInstruction extract |
         this.asInstruction() = extract and
         extract.extractsElement(_, 0) and
         extract.getBase().(IR::GetNextEntryInstruction).getDomain() = base.asInstruction()
       )
       or
+      // when there is no comma, as in `i := range y { ... }`
       not exists(IR::ExtractTupleElementInstruction extract |
         extract.getBase() = this.asInstruction()
       ) and

Original file line number	Diff line number	Diff line change
`@@ -147,7 +147,7 @@ module ControlFlow {`
`147`	`147`	`}`
`148`	`148`
`149`	`149`	`/**`
`150`		- * Holds if this node sets the value of element `idx` on `base` (or its implicit dereference)
	`150`	+ * Holds if this node sets the value of element `index` on `base` (or its implicit dereference)
`151`	`151`	* to `rhs`.
`152`	`152`	`*`
`153`	`153`	* For example, for the assignment `xs[i] = v`, `base` is either the data-flow node