Merge pull request #11294 from erik-krogh/fileDoc

erik-krogh · web-flow · commit 1eec067474c4 · 2022-11-23T22:23:36.000+01:00
QL: improve the "this block-comment should have been a QLDoc"-query
diff --git a/cpp/ql/lib/semmle/code/cpp/security/TaintTracking.qll b/cpp/ql/lib/semmle/code/cpp/security/TaintTracking.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * Support for tracking tainted data through the program. This is an alias for
  * `semmle.code.cpp.ir.dataflow.DefaultTaintTracking` provided for backwards
  * compatibility.
diff --git a/cpp/ql/src/jsf/4.09 Style/Naming.qll b/cpp/ql/src/jsf/4.09 Style/Naming.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * Common functions for implementing naming conventions
  *
  * Naming rules are the following:
diff --git a/csharp/ql/campaigns/Solorigate/lib/Solorigate.qll b/csharp/ql/campaigns/Solorigate/lib/Solorigate.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * Provides reusable predicates related to Solorigate
  */
 
diff --git a/csharp/ql/lib/experimental/code/csharp/Cryptography/NonCryptographicHashes.qll b/csharp/ql/lib/experimental/code/csharp/Cryptography/NonCryptographicHashes.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * Predicates that help detect potential non-cryptographic hash functions
  *
  * By themselves, non-cryptographic functions are common and not dangerous
diff --git a/java/ql/lib/semmle/code/java/frameworks/Properties.qll b/java/ql/lib/semmle/code/java/frameworks/Properties.qll
@@ -1,4 +1,5 @@
-/* Definitions related to `java.util.Properties`. */
+/** Definitions related to `java.util.Properties`. */
+
 import semmle.code.java.Type
 private import semmle.code.java.dataflow.FlowSteps
 
diff --git a/java/ql/lib/semmle/code/java/frameworks/Rmi.qll b/java/ql/lib/semmle/code/java/frameworks/Rmi.qll
@@ -1,4 +1,5 @@
-/* Remote Method Invocation. */
+/** Remote Method Invocation. */
+
 import java
 
 /** The interface `java.rmi.Remote`. */
diff --git a/java/ql/lib/semmle/code/java/frameworks/apache/Exec.qll b/java/ql/lib/semmle/code/java/frameworks/apache/Exec.qll
@@ -1,4 +1,5 @@
-/* Definitions related to the Apache Commons Exec library. */
+/** Definitions related to the Apache Commons Exec library. */
+
 import semmle.code.java.Type
 import semmle.code.java.security.ExternalProcess
 
diff --git a/java/ql/lib/semmle/code/java/security/ExternalProcess.qll b/java/ql/lib/semmle/code/java/security/ExternalProcess.qll
@@ -1,4 +1,5 @@
-/* Definitions related to external processes. */
+/** Definitions related to external processes. */
+
 import semmle.code.java.Member
 
 private module Instances {
diff --git a/java/ql/lib/semmle/code/java/security/RelativePaths.qll b/java/ql/lib/semmle/code/java/security/RelativePaths.qll
@@ -1,4 +1,5 @@
-/* Detection of strings and arrays of strings containing relative paths. */
+/** Detection of strings and arrays of strings containing relative paths. */
+
 import java
 
 /**
diff --git a/java/ql/lib/semmle/code/java/security/SqlUnescapedLib.qll b/java/ql/lib/semmle/code/java/security/SqlUnescapedLib.qll
@@ -1,4 +1,5 @@
-/* Definitions used by `SqlUnescaped.ql`. */
+/** Definitions used by `SqlUnescaped.ql`. */
+
 import semmle.code.java.security.ControlledString
 import semmle.code.java.dataflow.TaintTracking
 
diff --git a/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll b/java/ql/src/Violations of Best Practice/Dead Code/DeadLocals.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * Provides classes and predicates for "dead locals": which variables are used, which assignments are useless, etc.
  */
 
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Configures boosting for adaptive threat modeling (ATM).
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/AdaptiveThreatModeling.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/AdaptiveThreatModeling.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Provides information about the results of boosted queries for use in adaptive threat modeling (ATM).
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/BaseScoring.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/BaseScoring.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Provides shared scoring functionality for use in adaptive threat modeling (ATM).
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/CoreKnowledge.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Provides predicates that expose the knowledge of models
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointFeatures.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Extracts data about the database for use in adaptive threat modeling (ATM).
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointScoring.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointScoring.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Provides an implementation of scoring alerts for use in adaptive threat modeling (ATM).
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/FunctionBodyFeatures.qll b/javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/FunctionBodyFeatures.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * FunctionBodyFeatures.qll
  *
  * Contains logic relating to the `enclosingFunctionBody` and `enclosingFunctionName` features.
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/counting/CountAlertsAndSinks.qll b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/counting/CountAlertsAndSinks.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  *
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/Exclusions.qll b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/Exclusions.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Defines files that should be excluded from the evaluation of ML models.
diff --git a/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll b/javascript/ql/experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll
@@ -1,4 +1,4 @@
-/*
+/**
  * For internal use only.
  *
  * Extracts training data we can use to train ML models for ML-powered queries.
diff --git a/javascript/ql/test/tutorials/Validating RAML-based APIs/Osprey.qll b/javascript/ql/test/tutorials/Validating RAML-based APIs/Osprey.qll
@@ -1,4 +1,5 @@
-/* Model of Osprey API implementations. */
+/** Model of Osprey API implementations. */
+
 import javascript
 import HTTP
 
diff --git a/javascript/ql/test/tutorials/Validating RAML-based APIs/RAML.qll b/javascript/ql/test/tutorials/Validating RAML-based APIs/RAML.qll
@@ -1,4 +1,5 @@
-/* Model of RAML specifications. */
+/** Model of RAML specifications. */
+
 import javascript
 import HTTP
 
diff --git a/python/ql/lib/semmle/python/TestUtils.qll b/python/ql/lib/semmle/python/TestUtils.qll
@@ -1,4 +1,5 @@
-/* This file contains test-related utility functions */
+/** This file contains test-related utility functions */
+
 import python
 
 /** Removes everything up to the occurrence of `sub` in the string `str` */
diff --git a/ql/ql/src/codeql_ql/ast/Ast.qll b/ql/ql/src/codeql_ql/ast/Ast.qll
@@ -156,12 +156,24 @@ class TopLevel extends TTopLevel, AstNode {
   }
 
   QLDoc getQLDocFor(ModuleMember m) {
-    exists(int i | i > 0 and result = this.getMember(i) and m = this.getMember(i + 1))
+    exists(int i | result = this.getMember(i) and m = this.getMember(i + 1)) and
+    (
+      m instanceof ClasslessPredicate
+      or
+      m instanceof Class
+      or
+      m instanceof Module
+    )
   }
 
   override string getAPrimaryQlClass() { result = "TopLevel" }
 
-  override QLDoc getQLDoc() { result = this.getMember(0) }
+  override QLDoc getQLDoc() {
+    result = this.getMember(0) and
+    // it's not the QLDoc for a module member
+    not this.getQLDocFor(_) = result and
+    result.getLocation().getStartLine() = 1 // this might not hold if there is a block comment above, and that's the point.
+  }
 }
 
 abstract class Comment extends AstNode, TComment {
@@ -536,6 +548,12 @@ class ClasslessPredicate extends TClasslessPredicate, Predicate, ModuleDeclarati
 
   /** Holds if this classless predicate is a signature predicate with no body. */
   predicate isSignature() { not exists(this.getBody()) }
+
+  override QLDoc getQLDoc() {
+    result = any(TopLevel m).getQLDocFor(this)
+    or
+    result = any(Module m).getQLDocFor(this)
+  }
 }
 
 /**
diff --git a/ql/ql/src/queries/style/NonDocBlock.ql b/ql/ql/src/queries/style/NonDocBlock.ql
@@ -29,24 +29,59 @@ int getLineAboveNodeThatCouldHaveDoc(File file) {
 
 pragma[noinline]
 BlockComment getACommentThatCouldBeQLDoc(File file) {
-  file = result.getLocation().getFile() and
-  result.getLocation().getEndLine() = getLineAboveNodeThatCouldHaveDoc(file) and
-  result.getLocation().getFile().getExtension() = "qll" and
-  not result.getContents().matches("/**%")
+  exists(Location loc | loc = result.getLocation() |
+    file = loc.getFile() and
+    loc.getFile().getExtension() = "qll" and
+    not result.getContents().matches("/**%") and
+    not [loc.getStartLine(), loc.getEndLine()] = getLinesWithNonComment(file) and
+    (
+      // above something that can be commented.
+      loc.getEndLine() = getLineAboveNodeThatCouldHaveDoc(file)
+      or
+      // toplevel in file.
+      loc.getStartLine() = 1 and
+      loc.getStartColumn() = 1
+    )
+  )
+}
+
+pragma[noinline]
+int getLinesWithNonComment(File f) {
+  exists(AstNode n, Location loc |
+    not n instanceof Comment and
+    not n instanceof TopLevel and
+    loc = n.getLocation() and
+    loc.getFile() = f
+  |
+    result = [loc.getEndLine(), loc.getStartLine()]
+  )
 }
 
 pragma[noinline]
-BlockComment getCommentAt(File file, int endLine) {
+BlockComment getCommentAtEnd(File file, int endLine) {
   result = getACommentThatCouldBeQLDoc(file) and
   result.getLocation().getEndLine() = endLine
 }
 
-from AstNode node, BlockComment comment
+pragma[noinline]
+BlockComment getCommentAtStart(File file, int startLine) {
+  result = getACommentThatCouldBeQLDoc(file) and
+  result.getLocation().getStartLine() = startLine
+}
+
+from AstNode node, BlockComment comment, string nodeDescrip
 where
-  canHaveQLDoc(node) and
+  (
+    canHaveQLDoc(node) and
+    comment = getCommentAtEnd(node.getLocation().getFile(), node.getLocation().getStartLine() - 1) and
+    nodeDescrip = "the below code"
+    or
+    node instanceof TopLevel and
+    comment = getCommentAtStart(node.getLocation().getFile(), 1) and
+    nodeDescrip = "the file"
+  ) and
   not exists(node.getQLDoc()) and
   not node.(ClassPredicate).isOverride() and // ignore override predicates
   not node.hasAnnotation("deprecated") and // ignore deprecated
-  not node.hasAnnotation("private") and // ignore private
-  comment = getCommentAt(node.getLocation().getFile(), node.getLocation().getStartLine() - 1)
-select comment, "Block comment could be QLDoc for $@.", node, "the below code"
+  not node.hasAnnotation("private") // ignore private
+select comment, "Block comment could be QLDoc for $@.", node, nodeDescrip
diff --git a/ql/ql/test/queries/style/NonDocBlock/Foo.qll b/ql/ql/test/queries/style/NonDocBlock/Foo.qll
@@ -0,0 +1,22 @@
+/*
+ * This should be QLDoc.
+ */
+
+/**
+ * this is fine
+ */
+predicate foo() { any() }
+
+/* Note: this is bad. */
+class Foo extends string {
+  Foo() { this = "FOo" }
+}
+
+/**
+ * This is also fine.
+ */
+/*abstract*/ class Bar extends string {
+  string getMergeRaw() { none() } // <- fine. The abstract comment is fine, it doesn't need to be QLDoc.
+
+  Bar() { this = "bar" }
+}
diff --git a/ql/ql/test/queries/style/NonDocBlock/NonDocBlock.expected b/ql/ql/test/queries/style/NonDocBlock/NonDocBlock.expected
@@ -0,0 +1,2 @@
+| Foo.qll:1:1:3:3 | BlockComment | Block comment could be QLDoc for $@. | Foo.qll:1:1:22:2 | TopLevel | the file |
+| Foo.qll:10:1:10:24 | BlockComment | Block comment could be QLDoc for $@. | Foo.qll:11:7:11:9 | Class Foo | the below code |
diff --git a/ql/ql/test/queries/style/NonDocBlock/NonDocBlock.qlref b/ql/ql/test/queries/style/NonDocBlock/NonDocBlock.qlref
@@ -0,0 +1 @@
+queries/style/NonDocBlock.ql

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-/*`
	`1`	`+/**`
`2`	`2`	`* Support for tracking tainted data through the program. This is an alias for`
`3`	`3`	* `semmle.code.cpp.ir.dataflow.DefaultTaintTracking` provided for backwards
`4`	`4`	`* compatibility.`