Merge pull request #8374 from erik-krogh/nonDocBlock

QL: add query detecting block comments in a position where a QLDoc should be

Author: erik-krogh

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -431,7 +431,7 @@ class SsaPhiNode extends Node, TSsaPhiNode {
 
   SsaPhiNode() { this = TSsaPhiNode(phi) }
 
-  /* Get the phi node associated with this node. */
+  /** Gets the phi node associated with this node. */
   Ssa::PhiNode getPhiNode() { result = phi }
 
   override Declaration getEnclosingCallable() { result = this.getFunction() }

csharp/ql/lib/semmle/code/csharp/exprs/Creation.qll

@@ -435,10 +435,10 @@ class AnonymousFunctionExpr extends Expr, Callable, Modifiable, @anonymous_funct
  * A lambda expression, for example `(int x) => x + 1`.
  */
 class LambdaExpr extends AnonymousFunctionExpr, @lambda_expr {
-  /* Holds if this lambda expression has explicit return type. */
+  /** Holds if this lambda expression has explicit return type. */
   predicate hasExplicitReturnType() { lambda_expr_return_type(this, _) }
 
-  /* Gets the explicit return type of this lambda expression, if any. */
+  /** Gets the explicit return type of this lambda expression, if any. */
   Type getExplicitReturnType() { lambda_expr_return_type(this, getTypeRef(result)) }
 
   override string toString() { result = "(...) => ..." }

python/ql/lib/semmle/python/Flow.qll

@@ -299,7 +299,7 @@ class ControlFlowNode extends @py_flow_node {
     exists(BasicBlock b, int i, int j | this = b.getNode(i) and other = b.getNode(j) and i < j)
   }
 
-  /* Holds if this CFG node is a branch */
+  /** Holds if this CFG node is a branch */
   predicate isBranch() { py_true_successors(this, _) or py_false_successors(this, _) }
 
   ControlFlowNode getAChild() { result = this.getExprChild(this.getBasicBlock()) }

python/ql/lib/semmle/python/dataflow/old/Implementation.qll

@@ -991,7 +991,7 @@ int iterable_unpacking_descent(SequenceNode left_parent, ControlFlowNode left_de
 }
 
 module Implementation {
-  /* A call that returns a copy (or similar) of the argument */
+  /** Holds if `tonode` is a call that returns a copy (or similar) of the argument `fromnode` */
   predicate copyCall(ControlFlowNode fromnode, CallNode tonode) {
     tonode.getFunction().(AttrNode).getObject("copy") = fromnode
     or

python/ql/lib/semmle/python/dataflow/old/Legacy.qll

@@ -2,7 +2,7 @@ import semmle.python.dataflow.TaintTracking
 private import semmle.python.objects.ObjectInternal
 import semmle.python.dataflow.Implementation
 
-/* Backwards compatibility with config-less taint-tracking */
+/** A configuration that provides backwards compatibility with config-less taint-tracking */
 private class LegacyConfiguration extends TaintTracking::Configuration {
   LegacyConfiguration() {
     /* A name that won't be accidentally chosen by users */

python/ql/lib/semmle/python/dependencies/Dependencies.qll

@@ -158,13 +158,13 @@ private predicate defn_of_instance_attribute(Assign asgn, Class c, string name)
   )
 }
 
-/* Whether asgn defines an attribute of a class */
+/** Holds if asgn defines an attribute of a class */
 private predicate defn_of_class_attribute(Assign asgn, Class c, string name) {
   asgn.getScope() = c and
   asgn.getATarget().(Name).getId() = name
 }
 
-/* Holds if `value` is a value assigned to the `name`d attribute of module `m`. */
+/** Holds if `value` is a value assigned to the `name`d attribute of module `m`. */
 private predicate defn_of_module_attribute(ControlFlowNode value, Module m, string name) {
   exists(DefinitionNode def |
     def.getScope() = m and

python/ql/lib/semmle/python/objects/ObjectAPI.qll

@@ -18,7 +18,7 @@ private import semmle.python.types.Builtins
 
 class ObjectSource = Object;
 
-/* Aliases for scopes */
+/** An alias for Function used for scopes */
 class FunctionScope = Function;
 
 class ClassScope = Class;

python/ql/lib/semmle/python/pointsto/Base.qll

@@ -120,7 +120,7 @@ int version_tuple_compare(Object t) {
   version_tuple_value(t) > major_minor() and result = 1
 }
 
-/* Holds if `cls` is a new-style class if it were to have no explicit base classes */
+/** Holds if `cls` is a new-style class if it were to have no explicit base classes */
 predicate baseless_is_new_style(ClassObject cls) {
   cls.isBuiltin()
   or

python/ql/lib/semmle/python/pointsto/PointsTo.qll

@@ -2395,7 +2395,7 @@ module Types {
     )
   }
 
-  /* Holds if type inference failed to compute the full class hierarchy for this class for the reason given. */
+  /** Holds if type inference failed to compute the full class hierarchy for this class for the reason given. */
   private predicate failedInference(ClassObjectInternal cls, string reason, int priority) {
     strictcount(cls.(PythonClassObjectInternal).getScope().getADecorator()) > 1 and
     reason = "Multiple decorators" and

python/ql/lib/semmle/python/security/strings/Common.qll

@@ -1,6 +1,6 @@
 import python
 
-/* A call that returns a copy (or similar) of the argument */
+/** A call that returns a copy (or similar) of the argument */
 deprecated predicate copy_call(ControlFlowNode fromnode, CallNode tonode) {
   tonode.getFunction().(AttrNode).getObject("copy") = fromnode
   or

python/ql/lib/semmle/python/types/ClassObject.qll

@@ -166,7 +166,7 @@ class ClassObject extends Object {
     not this.failedInference()
   }
 
-  /* Whether this class is abstract. */
+  /** Holds if this class is abstract. */
   predicate isAbstract() {
     this.getMetaClass() = theAbcMetaClassObject()
     or

python/ql/src/Lexical/CommentedOutCode.qll

@@ -158,11 +158,11 @@ private predicate commented_out_code_block(Comment start, Comment end) {
   not commented_out_code(non_empty_following(end))
 }
 
-/* A single line comment that appears to be commented out code */
+/** A single line comment that appears to be commented out code */
 class CommentedOutCodeLine extends Comment {
   CommentedOutCodeLine() { exists(CommentedOutCodeBlock b | b.contains(this)) }
 
-  /* Whether this commented-out code line is likely to be example code embedded in a larger comment. */
+  /** Holds if this commented-out code line is likely to be example code embedded in a larger comment. */
   predicate maybeExampleCode() {
     exists(CommentedOutCodeBlock block |
       block.contains(this) and

python/ql/src/Resources/FileOpen.qll

@@ -66,7 +66,7 @@ predicate passes_open_files(Variable v, ControlFlowNode test, boolean sense) {
   )
 }
 
-/* Helper for `def_is_open` to give better join order */
+// Helper for `def_is_open` to give better join order
 private predicate passes_open_files(PyEdgeRefinement refinement) {
   passes_open_files(refinement.getSourceVariable(), refinement.getPredecessor().getLastNode(),
     refinement.getSense())

ql/ql/src/codeql_ql/ast/Ast.qll

@@ -156,6 +156,16 @@ class QLDoc extends TQLDoc, AstNode {
   override string getAPrimaryQlClass() { result = "QLDoc" }
 }
 
+class BlockComment extends TBlockComment, AstNode {
+  QL::BlockComment comment;
+
+  BlockComment() { this = TBlockComment(comment) }
+
+  string getContents() { result = comment.getValue() }
+
+  override string getAPrimaryQlClass() { result = "BlockComment" }
+}
+
 /**
  * The `from, where, select` part of a QL query.
  */
@@ -1867,13 +1877,13 @@ class BinOpExpr extends TBinOpExpr, Expr {
   /** Gets the left operand of the binary expression. */
   Expr getLeftOperand() { none() } // overriden in subclasses
 
-  /* Gets the right operand of the binary expression. */
+  /** Gets the right operand of the binary expression. */
   Expr getRightOperand() { none() } // overriden in subclasses
 
   /** Gets the operator of the binary expression. */
   FunctionSymbol getOperator() { none() } // overriden in subclasses
 
-  /* Gets an operand of the binary expression. */
+  /** Gets an operand of the binary expression. */
   final Expr getAnOperand() { result = this.getLeftOperand() or result = this.getRightOperand() }
 }
 

ql/ql/src/codeql_ql/ast/internal/AstNodes.qll

@@ -6,6 +6,7 @@ cached
 newtype TAstNode =
   TTopLevel(QL::Ql file) or
   TQLDoc(QL::Qldoc qldoc) or
+  TBlockComment(QL::BlockComment comment) or
   TClasslessPredicate(QL::ClasslessPredicate pred) or
   TVarDecl(QL::VarDecl decl) or
   TFieldDecl(QL::Field field) or
@@ -146,6 +147,8 @@ QL::AstNode toQL(AST::AstNode n) {
   or
   n = TQLDoc(result)
   or
+  n = TBlockComment(result)
+  or
   n = TClasslessPredicate(result)
   or
   n = TVarDecl(result)

ql/ql/src/queries/style/NonDocBlock.ql

@@ -0,0 +1,52 @@
+/**
+ * @name Block comment that is not QLDoc
+ * @description Placing a block comment that could have been a QLDoc comment is an indication that it should have been a QLDoc comment.
+ * @kind problem
+ * @problem.severity warning
+ * @id ql/non-doc-block
+ * @tags maintainability
+ * @precision very-high
+ */
+
+import ql
+
+predicate canHaveQLDoc(AstNode node) {
+  node instanceof Class
+  or
+  node instanceof Module
+  or
+  node instanceof ClasslessPredicate
+  or
+  node instanceof ClassPredicate
+}
+
+pragma[noinline]
+int getLineAboveNodeThatCouldHaveDoc(File file) {
+  exists(AstNode node | canHaveQLDoc(node) |
+    result = node.getLocation().getStartLine() - 1 and file = node.getLocation().getFile()
+  )
+}
+
+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("/**%")
+}
+
+pragma[noinline]
+BlockComment getCommentAt(File file, int endLine) {
+  result = getACommentThatCouldBeQLDoc(file) and
+  result.getLocation().getEndLine() = endLine
+}
+
+from AstNode node, BlockComment comment
+where
+  canHaveQLDoc(node) 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"