Skip to content

QL: improve the "this block-comment should have been a QLDoc"-query #11294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cpp/ql/lib/semmle/code/cpp/security/TaintTracking.qll
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/src/jsf/4.09 Style/Naming.qll
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* Common functions for implementing naming conventions
*
* Naming rules are the following:
Expand Down
2 changes: 1 addition & 1 deletion csharp/ql/campaigns/Solorigate/lib/Solorigate.qll
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* Provides reusable predicates related to Solorigate
*/

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* Predicates that help detect potential non-cryptographic hash functions
*
* By themselves, non-cryptographic functions are common and not dangerous
Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/frameworks/Properties.qll
Original file line number Diff line number Diff line change
@@ -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

Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/frameworks/Rmi.qll
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Remote Method Invocation. */
/** Remote Method Invocation. */

import java

/** The interface `java.rmi.Remote`. */
Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/frameworks/apache/Exec.qll
Original file line number Diff line number Diff line change
@@ -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

Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/security/ExternalProcess.qll
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Definitions related to external processes. */
/** Definitions related to external processes. */

import semmle.code.java.Member

private module Instances {
Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/security/RelativePaths.qll
Original file line number Diff line number Diff line change
@@ -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

/**
Expand Down
3 changes: 2 additions & 1 deletion java/ql/lib/semmle/code/java/security/SqlUnescapedLib.qll
Original file line number Diff line number Diff line change
@@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* Provides classes and predicates for "dead locals": which variables are used, which assignments are useless, etc.
*/

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Configures boosting for adaptive threat modeling (ATM).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Provides information about the results of boosted queries for use in adaptive threat modeling (ATM).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Provides shared scoring functionality for use in adaptive threat modeling (ATM).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Provides predicates that expose the knowledge of models
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Extracts data about the database for use in adaptive threat modeling (ATM).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Provides an implementation of scoring alerts for use in adaptive threat modeling (ATM).
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* FunctionBodyFeatures.qll
*
* Contains logic relating to the `enclosingFunctionBody` and `enclosingFunctionName` features.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Defines files that should be excluded from the evaluation of ML models.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/**
* For internal use only.
*
* Extracts training data we can use to train ML models for ML-powered queries.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Model of Osprey API implementations. */
/** Model of Osprey API implementations. */

import javascript
import HTTP

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* Model of RAML specifications. */
/** Model of RAML specifications. */

import javascript
import HTTP

Expand Down
3 changes: 2 additions & 1 deletion python/ql/lib/semmle/python/TestUtils.qll
Original file line number Diff line number Diff line change
@@ -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` */
Expand Down
22 changes: 20 additions & 2 deletions ql/ql/src/codeql_ql/ast/Ast.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
}

/**
Expand Down
55 changes: 45 additions & 10 deletions ql/ql/src/queries/style/NonDocBlock.ql
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we could remove some redundant logic here by adding a reason parameter to getACommentThatCouldBeQLDoc returning "the below code" or "the file".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that could save some duplication.
But that sounds like something for a followup, this seems good enough to merge.

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
22 changes: 22 additions & 0 deletions ql/ql/test/queries/style/NonDocBlock/Foo.qll
Original file line number Diff line number Diff line change
@@ -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" }
}
2 changes: 2 additions & 0 deletions ql/ql/test/queries/style/NonDocBlock/NonDocBlock.expected
Original file line number Diff line number Diff line change
@@ -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 |
1 change: 1 addition & 0 deletions ql/ql/test/queries/style/NonDocBlock/NonDocBlock.qlref
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
queries/style/NonDocBlock.ql